This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] - Program headers: add an additional check for `Offset`
ClosedPublic

Authored by grimar on Apr 16 2020, 8:30 AM.

Details

Summary

The Offset field is used to set the file offset of a program header.
In a normal object it should not be greater than the minimal offset
of sections included into segment.

This patch adds a check for that and adds tests.

Diff Detail

Event Timeline

grimar created this revision.Apr 16 2020, 8:30 AM
grimar marked an inline comment as done.Apr 17 2020, 12:54 AM
grimar added inline comments.
llvm/test/tools/llvm-objcopy/ELF/preserve-segment-contents.test
348 ↗(On Diff #258062)

I think for empty segments we can allow any "Offset". Then similar changes will go away.

grimar updated this revision to Diff 258276.Apr 17 2020, 3:16 AM
  • Allow any "Offset" for empty segments.

Not looked at the detail of this change yet, but I'm wondering why we need it? One way of looking at things is that a segment is just a range. It doesn't have any literal contents in the sense of things it strictly owns, unlike sections which need to have contents placed at specific locations, which cannot overlap other contents of the file. Consequently, we essentially need two offset fields for sections: 1) where the contents are placed (derived implicitly), and 2) what the sh_offset field should be (allowing it to be placed in places where contents otherwise couldn't be written).

Can't the regular Offset field be used for this instead? Yes, that might mean that the offset is greater than the minimum section offset, but I think that's okay?

Can't the regular Offset field be used for this instead? Yes, that might mean that the offset is greater than the minimum section offset, but I think that's okay?

That is what I was not sure is OK and why this patch appeared. We have a few test cases like:

Sections:
  - Name: .dynamic
    Type: SHT_DYNAMIC
    Entries:
      - Tag:   DT_NULL
        Value: 0
ProgramHeaders:
  - Type:    PT_DYNAMIC
    Offset: 0xffff0000
    Sections:
      - Section: .dynamic

I read it as (knowing the internal logic): "take the list of 'Sections` and assign the proper memory size, file size and the offset.
But since the "Offset" is specified, it itself reads as "and change the offset to something", what can be valid because we might need the offset to be lower than the minimum section offset.
But when it is not lower its meaning intersects with the section list. It looks like an issue because it is not clear what the correct logic should be probably? E.g. which memory and file size we want to set?
Using of "POffset" here would say: "take the list of sections, calculate everything as usual, but override the final offset"

Does it make sense?

jhenderson added a comment.EditedApr 20 2020, 5:34 AM

Does it make sense?

I think the problem here is that we have two competing sources of information for how to layout a program header - the Sections list and the Offset key. Possibly me mental model is outdated, but I thought the layout of the program headers had no effect on that of the sections? In other words, I thought that a section offset was based purely on its alignment and the positions and sizes of any sections/fillers/headers before it? Assuming that my model is correct, a program header's offset and size are determined by the contents, if unspecified, but a forced Offset greater than the start offset of some section seems invalid, and should therefore generate an error?

But when it is not lower its meaning intersects with the section list. It looks like an issue because it is not clear what the correct logic should be probably? E.g. which memory and file size we want to set?

In other words, don't try to derive a memory and file size and just emit an error in this case. It doesn't need to be allowed to create a broken ELF in this instance, because you can always specify the sizes explicitly.

The alternative of course is to completely change yaml2obj's model to lead with program headers, i.e. sections listed in program headers should be placed based on the offset of the program header. I don't think this is necessarily a good change to make though.

There is some misunderstanding it seems:

Does it make sense?

I think the problem here is that we have two competing sources of information for how to layout a program header - the Sections list and the Offset key. Possibly me mental model is outdated, but I thought the layout of the program headers had no effect on that of the sections? In other words, I thought that a section offset was based purely on its alignment and the positions and sizes of any sections/fillers/headers before it?

It is correct.

but a forced Offset greater than the start offset of some section seems invalid, and should therefore generate an error?

This is what this patch does. It introduces an error when the program header Offset forced is greater than the minimal section offset.
But since we need a way to set an arbitrary value for some tests (to override p_offset), I've introduced POffset, which does not affect on the properties of a program header, it just overrides the p_offset field without any checks.

So I think the behavior you described is exactly what this patch does, or not ?

This is what this patch does. It introduces an error when the program header Offset forced is greater than the minimal section offset.
But since we need a way to set an arbitrary value for some tests (to override p_offset), I've introduced POffset, which does not affect on the properties of a program header, it just overrides the p_offset field without any checks.

So I think the behavior you described is exactly what this patch does, or not ?

The bit I don't understand is the need for the POffset field. In such instances, just don't list the sections and use explicit Offset + FileSize/MemSize. The sizes wouldn't make sense anyway since the program header would be at a weird location.

grimar added a comment.EditedApr 20 2020, 6:53 AM

This is what this patch does. It introduces an error when the program header Offset forced is greater than the minimal section offset.
But since we need a way to set an arbitrary value for some tests (to override p_offset), I've introduced POffset, which does not affect on the properties of a program header, it just overrides the p_offset field without any checks.

So I think the behavior you described is exactly what this patch does, or not ?

The bit I don't understand is the need for the POffset field. In such instances, just don't list the sections and use explicit Offset + FileSize/MemSize. The sizes wouldn't make sense anyway since the program header would be at a weird location.

OK, great! That what I did it initially in D78005 (in tests) and it sounds good to me. Then I'll remove the "POffset" field from this patch. So it will just introduce the offset validation check and adopt existent test cases.

grimar updated this revision to Diff 258752.Apr 20 2020, 8:21 AM
grimar retitled this revision from [yaml2obj] - Program headers: introduce the `POffset` and add an additional check for `Offset` to [yaml2obj] - Program headers: and add an additional check for `Offset`.
grimar edited the summary of this revision. (Show Details)
  • Remove "POffset".
jhenderson added inline comments.Apr 21 2020, 12:25 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
760

Should this ignore SHT_NOBITS?

764

I think it's okay to slightly simplify to make the error message slightly less verbose:

"'Offset' for segment with index"

766

I think this needs rewording:

" must be less than or equal to the minimum file offset of all included sections (0x123456)"

llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml
95

This comment is stale and needs updating.

113

minimal section offset -> minimum offset

(I would generally prefer minimum to minimal as minimal can also just mean "really small", so could occasionally be ambiguous)

143

FWIW, I wouldn't indent PT_LOAD here or below. I don't consider the Sections block as important for spacing, becuase it doesn't have a single inline value.

151

I would change "when it is less or equal to" to "to be less than or equal to".

Also, this test case only shows one of "equal to" or "less than". It should probably test both cases.

180

Perhaps worth also showing that ShOffset is ignored for these calculations (or not as the case may be)?

BTW, the title of the patch seems to have gotten messed up.

grimar updated this revision to Diff 258988.Apr 21 2020, 7:01 AM
grimar marked 12 inline comments as done.
grimar retitled this revision from [yaml2obj] - Program headers: and add an additional check for `Offset` to [yaml2obj] - Program headers: add an additional check for `Offset`.
  • Addressed review comments.
llvm/lib/ObjectYAML/ELFEmitter.cpp
760

I am not sure why it should..
Anyways this patch should not change the logic of computations. It's intended to add an Offset check only.

(aside: this piece was changed after rebasing, now when we know that frarments are sorted, the code is simpler).

llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml
113

Ok, thanks.

180

Done.

grimar updated this revision to Diff 258991.Apr 21 2020, 7:03 AM
  • Fix comment.
jhenderson accepted this revision.Apr 22 2020, 12:32 AM

LGTM, with nit.

llvm/lib/ObjectYAML/ELFEmitter.cpp
760

I am not sure why it should..
Anyways this patch should not change the logic of computations. It's intended to add an Offset check only.

(aside: this piece was changed after rebasing, now when we know that frarments are sorted, the code is simpler).

Not sure where I was coming form with that comment before, but it's worth noting that SHT_NOBITS offsets in theory should be meaningless. I don't see a strong reason to ignore them though.

llvm/test/tools/yaml2obj/ELF/program-header-size-offset.yaml
186

Nit: please add an extra space before FileCheck.

This revision is now accepted and ready to land.Apr 22 2020, 12:32 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2020, 3:12 AM