This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Fix latent bug that allowed some Sections to be improperly cast to StringTableSections
ClosedPublic

Authored by jakehehrlich on Sep 27 2017, 12:59 PM.

Details

Summary

If a Section had Type SHT_STRTAB (which could happen if you had a .dynstr section) it was possible to cast Section to StringTableSection and get away with any operation that was supported by SectionBase without it being noticed. This change makes this bug easier to notice and fixes it where it occurred. It also made me realize that there was some duplication of efforts in the loop that calls ::initialize. These issues are all fixed by this change.

Diff Detail

Repository
rL LLVM

Event Timeline

jakehehrlich created this revision.Sep 27 2017, 12:59 PM
jhenderson edited edge metadata.Sep 28 2017, 1:19 AM

So, from what I can see, it is no longer a requirement for a dynamic section (or indeed any "SectionWithStrTab" instance) to refer to a section of type SHT_STRTAB? Is this desirable, because it sounds a bit odd to me? I could support a check in SectionWithStrtab::initialize that checks the actual type of the string table. Otherwise, this change looks reasonable to me.

tools/llvm-objcopy/Object.h
125

As this class is now only usable for non-alloc string table sections, I think that we need to explicitly say so in the comment, along with the reason why.

So, from what I can see, it is no longer a requirement for a dynamic section (or indeed any "SectionWithStrTab" instance) to refer to a section of type SHT_STRTAB? Is this desirable, because it sounds a bit odd to me? I could support a check in SectionWithStrtab::initialize that checks the actual type of the string table. Otherwise, this change looks reasonable to me.

I think a check is a good idea. In fact I thought I added that check; not sure why it didn't make it to review.

  1. Added comment explaining why classof works the way it does.
  2. Added check for SHT_STRTAB

I'm starting to notice a slightly non-trivial match up between classof and makeSection. Is there a way we could make this precise? Ideally I'd like to somehow contain the means of construction for a type within the class itself. I'd really like the following properties to be enforced by the library

  1. You can't construct a StringTableSection that dosn't pass StringTableSection::classof
  2. The checks for SHT_* and SHF_ALLOC shouldn't be duplicated in construction and classof. When one changes the other should change as well.
jhenderson accepted this revision.Oct 4 2017, 9:19 AM

LGTM with typo fixes.

I'm starting to notice a slightly non-trivial match up between classof and makeSection. Is there a way we could make this precise? Ideally I'd like to somehow contain the means of construction for a type within the class itself. I'd really like the following properties to be enforced by the library

  1. You can't construct a StringTableSection that dosn't pass StringTableSection::classof
  2. The checks for SHT_* and SHF_ALLOC shouldn't be duplicated in construction and classof. When one changes the other should change as well.

I agree with this, although I think it's a separate change. I need to think a bit more about what a good solution might be.

tools/llvm-objcopy/Object.h
119

addrsses -> addresses

124

the agrees -> then agrees

This revision is now accepted and ready to land.Oct 4 2017, 9:19 AM

Fixed typos

This revision was automatically updated to reflect the committed changes.