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

[Benoit Belley]

Thanks for taking the time to answer. My comment was by no-mean implying that I was requesting any immediate code change. I completely agree, as I mentioned, in my original comment, that calling report_fatal_error() is infinitely better than crashing on segfault.

I was under the impression that the Object reader would be used when using/implementing an MCJIT object cache. Is that the case ?

Benoit

From: Filipe Cabecinhas <filcab+llvm.phabricator at gmail.com<mailto:filcab+llvm.phabricator at gmail.com>>
Date: jeudi 15 janvier 2015 14:17
To: "reviews+D6945+public+88af01f8557bf636 at reviews.llvm.org<mailto:reviews+D6945+public+88af01f8557bf636 at reviews.llvm.org>" <reviews+D6945+public+88af01f8557bf636 at reviews.llvm.org<mailto:reviews+D6945+public+88af01f8557bf636 at reviews.llvm.org>>
Cc: "rafael.espindola at gmail.com<mailto:rafael.espindola at gmail.com>" <rafael.espindola at gmail.com<mailto:rafael.espindola at gmail.com>>, Benoit Belley <benoit.belley at autodesk.com<mailto:benoit.belley at autodesk.com>>, "david.majnemer at gmail.com<mailto:david.majnemer at gmail.com>" <david.majnemer at gmail.com<mailto:david.majnemer at gmail.com>>, "llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>" <llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>>
Subject: Re: [PATCH] Report fatal errors instead of segfaulting/asserting on a few invalid accesses while reading MachO files.

Hi Benoit,

On Thu, Jan 15, 2015 at 5:22 AM, Benoit Belley <benoit.belley at autodesk.com<mailto: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://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/4e981ae1/attachment.html>