This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by filcab on Jan 12 2015, 6:59 PM.

Diff Detail

Event Timeline

filcab updated this revision to Diff 18067.Jan 12 2015, 6:59 PM
filcab retitled this revision from to Report fatal errors instead of segfaulting/asserting on a few invalid accesses while reading MachO files..
filcab updated this object.
filcab edited the test plan for this revision. (Show Details)
filcab added a reviewer: rafael.
filcab added a subscriber: Unknown Object (MLST).
majnemer added inline comments.
lib/Object/MachOObjectFile.cpp
302

Maybe "Segment load command size is too small."?

rafael edited edge metadata.Jan 13 2015, 9:17 AM

Optional bonus (can be another patch): include more information in the errors: Section index is XXX, but there are only YYY sections.

lib/Object/MachOObjectFile.cpp
45

There are no tests for this one. Please add one or convert this to an assert.

60

Nit. maybe avoid this bit of duplicated with a small helper template function that takes a MachO::segment_command_64 or a MachO::segment_command?

filcab updated this revision to Diff 18129.Jan 13 2015, 8:29 PM
filcab edited edge metadata.

Rephrase error pointed out by majnemer
Added a test requested by Rafael

Rafael: Extracting that part of the function yields code that's a bit too convoluted. Check which version you prefer (or suggest other changes).

rafael added inline comments.Jan 14 2015, 10:49 AM
lib/Object/MachOObjectFile.cpp
55

Maybe pass a "const SegmentCmd&"?

2233

Missing test.

2243

Missing test.

test/Object/macho-invalid.test
34

There are 4 errors that would match " Invalid section index": Please make sure they are all covered.

filcab updated this revision to Diff 18193.Jan 14 2015, 4:43 PM

Added some missing tests

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.

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.

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 ?

Cheers,
Benoit

Benoit Belley
Sr Principal Developer
M&E-Product Development Group
Autodesk, Inc.
www.autodesk.com http://www.autodesk.com/

emaste added a subscriber: emaste.Jan 15 2015, 6:52 AM

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.

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.

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 ?

Cheers,
Benoit

Benoit Belley
Sr Principal Developer
M&E-Product Development Group
Autodesk, Inc.
www.autodesk.com http://www.autodesk.com/

This review is for code that parses files and is primarily used by introspective tools like llvm-objdump and the like. The code you are referring is substantially different, I think that discussion belongs in a different thread from this one.

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

Hi Benoit,

rafael accepted this revision.Jan 15 2015, 1:53 PM
rafael edited edge metadata.

Thanks again for fuzzing llvm!

This revision is now accepted and ready to land.Jan 15 2015, 1:53 PM
This revision was automatically updated to reflect the committed changes.
test/Object/macho-invalid.test