Page MenuHomePhabricator

[ARM] Fix lld crash introduced by r321154
AbandonedPublic

Authored by vit9696 on Jan 24 2018, 5:26 AM.

Details

Summary

Since SyntheticSection::getParent() may return null, dereferencing this pointer in ARMExidxSentinelSection::empty() call from removeUnusedSyntheticSections() results in crashes when linking ARM binaries.

A check against null is added to ARMExidxSentinelSection::empty() similarly to SyntheticSection::getVA(). Another check is added to removeUnusedSyntheticSections, because in other functions (e.g. applySynthetic) getParent is normally checked first. While either change will fix the code, both of them seem reasonable to me.

Please commit this for me after review, since I do not have commit access.

Diff Detail

Event Timeline

vit9696 created this revision.Jan 24 2018, 5:26 AM

Thanks for sending the patch. What I'd like to know first is in what circumstances the synthetic section has no parent and is this expected to be possible? If it isn't expected to be possible then it would be better to fix that and not try to account for the no parent case. The implementation has changed quite a bit since I last looked so I'm not sure of the intention.

Do you have a test case that you could add to your patch? For example find a case that currently faults without the patch and check that it links correctly with it. Perhaps checking that the output has no .ARM.exidx sections present.

Thanks for sending the patch. What I'd like to know first is in what circumstances the synthetic section has no parent and is this expected to be possible? If it isn't expected to be possible then it would be better to fix that and not try to account for the no parent case. The implementation has changed quite a bit since I last looked so I'm not sure of the intention.

Do you have a test case that you could add to your patch? For example find a case that currently faults without the patch and check that it links correctly with it. Perhaps checking that the output has no .ARM.exidx sections present.

Thanks for a quick reply. The reason why it has no parent (and crashes) is because we discard this section in our custom linker script, and do not include it anywhere.
It may be discussible whether it is fine to do, but lld should not crash regardless of that.
I could provide an example, but it is kind of obvious.

Thank you for the patch. I agree with Peter, the fix should be accomplished with a test.

I could provide an example, but it is kind of obvious.

The simpler, the better. Such tests help us avoid regressions in the future.

ELF/SyntheticSections.cpp
2595

Use early return here. It's better than increasing the level of nesting.

ELF/Writer.cpp
1312

I think this change is independent. It's more about style, so it should be applied separately.

vit9696 updated this revision to Diff 131280.Jan 24 2018, 8:39 AM

Added a test case and restyled the check like Igor suggested.

vit9696 added inline comments.Jan 24 2018, 8:43 AM
ELF/Writer.cpp
1312

Here I have to disagree. This avoids a potentially insecure code, which has already caused a crash.
Since a check is put here, and not before getParent, it is obvious that there exists some connection, so it is safer to firstly check for null.

Thanks for adding the test. I've added a small suggestion in an inline comment.

test/ELF/arm-exidx-discard.s
6

May I suggest adding a comment to explain what the test is for? Something like "// Check that the linker generated .ARM.exidx sentinel section is correctly removed. You then could use llvm-readobj to dump the Sections and use CHECK-NOT for .arm.exidx. That would make sure that we don't regress by crashing or by somehow keeping the linker generated sentinel.

vit9696 updated this revision to Diff 131295.Jan 24 2018, 9:30 AM

Updated with suggested test extension by Peter Smith.

Thanks for the change. I've no further comments.

vit9696 abandoned this revision.Jan 24 2018, 3:12 PM

This was committed in r323366, thanks to @rafael!