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

[Tobias Grosser]

On 10/30/2013 06:52 PM, Rafael Espíndola wrote:
>> Using fatal errors in the context of a library is unfortunate, as the fatal
>> error message on errs() may often not reach the user.
>
> Our fatal error implementation is more flexible than that. The tool
> could do pretty much anything with the error: log it, open a dialog,
> etc, as long as it remains fatal.

I see.

>> 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.
>
> I agree that that assumption is false. We should be able to handle
> corrupt files in release builds.

bitcode files are unversioned and there are most likely corruptions we 
can not detect. Still, if we actually were able to do so, we can
improve the user and debugging experience.

>> Even if we can not catch all cases of invalid bitcode files,
>> I believe we should try to avoid fatal errors if we can.
>
> Given the flexibility we give to the fatal error handling, the real
> question I think is: Do we expect any tool to handle this in a non
> fatal way? Right now the answer for in tree tools is no. Any such tool
> out of tree would also be in a very fragile situation given the
> existing asserts.

Yes, it would be in a fragile situation and I would not advice anybody 
to rely on the error handling in the bitcode reader.

On the other hand, the existing error reporting infrastructure seems
to be written exactly to support non-fatal errors. If we make fatal 
errors an official policy for the bitcode reader, it the interface would 
be unnecessary, no?

Regarding long running users, I can see e.g. OpenCL runtimes that use 
bitcode to cache binary programs and that may run due to some bugs into 
bitcode incompatibilities. If those runtimes have the chance to return 
an OpenCL error code, but to continue to work properly for other 
kernels, that seems to be beneficial.

>> 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?
>
> This would work for this particular case, but then how do we interface
> "rich" non fatal errors with APIs that use error_code? Do we extend
> error code to carry a std::string too?

$ grep error_code lib/Bitcode/* -R
$

This gives zero results. Which API are you talking about? Do those API's 
really need std::string? Or are you saying they are forwarding the
result of the Bitcode reader?

Another option would be to just drop the AttributeKind id, in case Nick 
is OK with this.

Cheers,
Tobias