[patch] Fix use after free in BitcodeReader.cpp error path

[Tobias Grosser]

Hi Raphael,

thanks for working on the bug I introduced.

Just for the history, my original patch did not include the attribute 
kind number, but I added it as Nick Lewycky asked for it in his review.
My implementation unfortunately introduced the bug then.

On 10/30/2013 05:53 PM, Rafael Espíndola wrote:
> CCing Tobias because he wrote the code being patched and Chris because
> he wrote most of the file and its asserts.
>
> I am trying to create an ObjectFile wrapper for bitcode files for use
> in llvm-nm, llvm-ar and lld. One issue is that ObjectFile uses
> error_code. The error handling at the bitcode libraries is a somewhat
> confusing mix. Some errors are returned via a std::string, some via a
> fixed list of "const char*" and some just use asserts.
>
> One particular case tries to use const char* but ends up accessing
> freed memory. What this patch does is convert that case to use a fatal
> error. Is that OK? I am assuming so since it is fairly easy to hit the
> asserts with a broken bitcode file (and they should probably be
> converted to a fatal error so we handle them correctly in release
> builds).
>
> If non fatal error is desired for this library, we probably have to
> find a way to extend error_code to hold a std::string so that we can
> use it to report "Unknown attribute kind (38)" instead of just
> "Unknown attribute kind".

Using fatal errors in the context of a library is unfortunate, as the 
fatal error message on errs() may often not reach the user. The 
asserts() seem to have been added under the assumption that invalid
bitcode files only happen for developers. One could claim that reading 
invalid bitcode files should not happen for users, but as I run into 
this and Nick asked for more debug output, it seems to not be such a 
rare case. Even if we can not catch all cases of invalid bitcode files,
I believe we should try to avoid fatal errors if we can.

Consequently I believe using or extending the already existing 'const 
char *' interface is the right thing to do. As the Error String is not 
in the hot path and as the bitcode reader does not seem a class that is
instantiated very often, what about making ErrorString a std::string?

Cheers,
Tobias