This is an archive of the discontinued LLVM Phabricator instance.

Do not ignore SizeOfOptionalHeader in COFF header even if PE header is not present.
ClosedPublic

Authored by milkovic on Jul 25 2016, 4:38 AM.

Details

Reviewers
ruiu
Summary

Attribute SizeOfOptionalHeader is ignored if no PE header is present in the file. This attribute should be ignored according to standard, however there are uses of this field even though it should not be used.

This change does not conform to PE/COFF standard, but there are several COFF files without PE header, where you had to add up SizeOfOptionalHeader in order to get proper section headers. Other tools and their own parsers do take this into account.

Diff Detail

Event Timeline

milkovic updated this revision to Diff 65330.Jul 25 2016, 4:38 AM
milkovic retitled this revision from to Do not ignore SizeOfOptionalHeader in COFF header even if PE header is not present..
milkovic updated this object.
milkovic added a reviewer: ruiu.
milkovic added a subscriber: llvm-commits.
ruiu edited edge metadata.Jul 25 2016, 6:54 PM

How can I create a COFF file that has no PE header but has an optional header? Do you give me information as to what tools are creating such files?

I can't really give you step by step guide on how to create the files like that, because this file was given to me to analyze, so I don't really know the source of the file. You can find the file in the attachment.

Other tools have no problems with this file, but when processing it with LLVM, I run into issues with wrong section headers. I have not yet found the file where this change would cause problems.

ruiu accepted this revision.Aug 4 2016, 1:58 PM
ruiu edited edge metadata.

Sorry for the belated response. I believe this is safe to land. LGTM.

This revision is now accepted and ready to land.Aug 4 2016, 1:58 PM

Thank you for approval. Would you please commit the change? I don't have rights to write to the repository.

ruiu added a comment.Aug 8 2016, 4:16 PM

Hi Marek,

I submitted this as r278066 but had to revert because it broke llvm/test/tools/llvm-readobj/bigobj.test test. If you still want me to submit this, please take a look and fix. Thanks!

milkovic updated this revision to Diff 67497.Aug 10 2016, 3:14 AM
milkovic edited edge metadata.

I have updated the diff with the new version. The new version checks for presence of COFFHeader because it may be nullified when bigobj COFF is detected. I have run all the tests and everything seems to be OK.

Thanks.

milkovic closed this revision.Aug 16 2016, 12:05 AM

Committed in r278429. Thanks!