[llvm] r263742 - Bitcode: Error out instead of crashing on corrupt metadata

[Justin Bogner via llvm-commits]

Rafael Espíndola <rafael.espindola at gmail.com> writes:
>> I could commit an 800 byte binary blob where I generated some IR then
>> manually flipped a bit in a hex editor to hit one of the five error
>> checks I added here. I could conceivably figure out how to generate
>> similar tests for the other four checks with enough time and effort. Of
>> course, since the bitcode's invalid it won't be meaningfully upgradeable
>> and will probably end up hitting some other error path very soon after
>> and no longer test this at all. When it does start erroring for whatever
>> reason someone will find that it isn't doing anything interesting and
>> probably delete it. IMO, This kind of test is useless.
>>
>> The case where I hit this in the real world involved a truncated bitcode
>> file from an out of tree front end. This also wouldn't be terribly
>> useful to commit.
>>
>> I think if we want to test this kind of change meaningfully we probably
>> need to expose the BitcodeReader type to unit tests, then call
>> individual parse functions with blobs of data that are short enough for
>> a human to understand. This could end up being messy anyway though, I'm
>> not really sure.
>
> My take is that if it is worth coding a "fancy" error handling, then
> it is worth testing it.
>
> Checking the 800 byte binary would be just perfect for this I think.

I'd need to create another four binaries with manual bit flips for the
testing of this to be more than just lip service, and I really disagree
that such tests are at all useful.

If we want to make it a requirement that we can test the bitcode reader
in a reasonable way before it's worth fixing this kind of crash on bogus
data, fine. I can revert this change for now and just let it crash. I'm
not convinced the tests that I am capable of generating at this point
are worth the cost of maintaining them, so I'm against committing them.