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

[Chris Lattner]

On Oct 30, 2013, at 9:53 AM, Rafael Espíndola <rafael.espindola at gmail.com> 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".

I agree that the current handling is bogus, but I'd rather this not be a fatal error.  The bitcode reader isn't perfectly there, but it tries hard to return errors up with the "return Error" pattern.  It is unquestionable goodness to make bitcode reading capable of safely handling reading of bogus files.  I realize we're not there yet, but this seems like it is pushing us away from that goal, not moving us closer.

-Chris