Page MenuHomePhabricator

[lld][ELF] Check length of subsection in .ARM.attributes
ClosedPublic

Authored by evgeny777 on Jun 12 2019, 2:50 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

evgeny777 created this revision.Jun 12 2019, 2:50 AM
ruiu added inline comments.Jun 12 2019, 2:52 AM
tools/lld/test/ELF/bad-arm-attributes2.test
1 ↗(On Diff #204242)

We don't use yaml2obj unless code is not representable in assembly. Is there any way to rewrite this in assembly?

ruiu accepted this revision.Jun 13 2019, 1:55 AM

LGTM with the following changes.

tools/lld/test/ELF/bad-arm-attributes.s
1 ↗(On Diff #204265)

Please move this to tools/lld/test/ELF/invalid directory.

tools/lld/test/ELF/bad-arm-attributes2.s
1 ↗(On Diff #204265)

Ditto

This revision is now accepted and ready to land.Jun 13 2019, 1:55 AM
grimar accepted this revision.Jun 13 2019, 2:21 AM

LGTM with one nit and Rui's comments addressed.

tools/lld/test/ELF/bad-arm-attributes.s
1 ↗(On Diff #204265)

I think you also need to add # REQUIRES: arm

MaskRay added inline comments.Jun 13 2019, 2:23 AM
tools/lld/test/ELF/bad-arm-attributes2.s
2 ↗(On Diff #204265)

%tout -> /dev/null

MaskRay added inline comments.Jun 13 2019, 2:27 AM
tools/lld/test/ELF/bad-arm-attributes2.s
2 ↗(On Diff #204265)

Oh, wait.

// ELF/InputFiles.cpp

    Attributes.Parse(Contents, /*isLittle*/ Config->EKind == ELF32LEKind);

Parse doesn't return an error? That may be something that should be improved later..

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2019, 6:39 AM

Ironically bad-arm-attributes2.s test is failing on an Arm buildbot http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/4/steps/ninja%20check%201/logs/FAIL%3A%20lld%3A%3Abad-arm-attributes2.s

invalid subsection length 4294967040 at offset 0

The expected is:

# CHECK: invalid subsection length 4294967295 at offset 1

I've not worked out why yet, it could be a 32/64-bit host issue.

I've not worked out why yet, it could be a 32/64-bit host issue.

Yes it appears to be the case. The offset starts at 1 and when 0xffffffff is added on a 32-bit machine the offset overflows back to 0

evgeny777 added a comment.EditedJun 14 2019, 6:07 AM

Strange, I haven't got notification. Are buildbot notifications working?
Anyway I think making Offset uint64_t instead of size_t will fix the issue.

Strange, I haven't got notification. Are buildbot notifications working?
Anyway I think making Offset uint64_t instead of size_t will fix the issue.

I think this landed, when the lab.llvm.org was down, we're just catching up. One possible drawback of making Offset uint64_t is that the .ARM.attributes should only be seen in a 32-bit ELF file so there is never a legal attributes section size that large, but I don't think it matters that much. An alternative would be to drop the size field to 0xFE, 0xFF, 0xFF, 0xFF.

would be to drop the size field to 0xFE, 0xFF, 0xFF, 0xFF.

This would cause problems on arm-be, no?

would be to drop the size field to 0xFE, 0xFF, 0xFF, 0xFF.

This would cause problems on arm-be, no?

That would give a different (smaller I think) size on BE yes, although LLD doesn't support big-endian at the moment. I kind of picked the first value that came into my head anyway, so by all means pick something else if you want to change the test.