[PATCH] Report fatal errors instead of segfaulting/asserting on a few invalid accesses while reading MachO files.

[Filipe Cabecinhas]

Hi Benoit,

On Thu, Jan 15, 2015 at 5:22 AM, Benoit Belley <benoit.belley at autodesk.com>
wrote:

> Hi Everyone,
>
> My understanding is that the ELF writer is also used by the MCJIT engine
> when filling an object buffer with jitted code. In that context, I am
> wondering if the call to llvm::report_fatal_error() is the appropriate way
> to report the error. It has the effect of killing the entire host
> application simply because one LLVM module couldn¹t be jitted.
>
I'm only doing changes for Mach-O, but future similar changes might hit the
ELF reader.

In any case, this impacts the readers, not the writers. And, more
importantly, this is covering over assertions and segfaults for bad user
input. I'm not calling report_fatal_error in places where our behavior was
well-defined (like when we “recovered” from bad user input in some way).
I think you'll agree that report_fatal_error is preferable to any assert or
segfault, which aren't recoverable (in general). :-)


> Of course, fatal errors and assertions should never occur! ;-) And, if
> they never occur, then there isn¹t any issue. Moreover, it is much better
> to exit the application after a proper error message has been emitted, then
> crashing on a segfault! So, this patch is definitely a step in the right
> direction.
>
:-)

On the other hand, report_fatal_error() has the unfortunate consequence of
> killing our application before our users have any chance of saving their
> work.
>
We can make libObject recover from this. But that will force us to change a
lot os libObject's internals (and API), since most of it isn't really ready
for recovering (even if recovering means just signalling an error occurred
to the caller) from bad input.
I have no real proposal to change the API to one that is resilient to bad
input, and there are other libObject criticisms that could be addressed in
a possible rewrite of the API. We can start doing it, but I'm not an expert
(or even very knowledgeable) on most libObject uses, so I can't propose a
better API right now. I can participate in the discussion if you want to
propose one, though. ;-)

Thus, I am wondering if there would be a way to indicate to the MCJIT
> engine that an internal error occurred while jitting a particular LLVM
> module. The compilation of that module would abort, but the application
> would not exit.
>
> Is that a legitimate concern ? Comments ?
>
I don't really think this changes anything for the current users of
libObject (whatever hits report_fatal_error was either reading bad memory,
asserting, or screwing up the file's semantics in a way that was most
likely not even compatible with whatever other tools exist (dynamic
loaders, otool, etc), which made it useless to try to keep reading the
file).


If you still have issues, or think we didn't account for something, let me
know.

Regards,

  Filipe


>
> Cheers,
> Benoit
>
> Benoit Belley
> Sr Principal Developer
> M&E-Product Development Group
> Autodesk, Inc.
> www.autodesk.com http://www.autodesk.com/
>
>
> http://reviews.llvm.org/D6945
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150115/ab1237a9/attachment.html>