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

[grimar]

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.

[grimar]

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]

• Allow any "Offset" for empty segments.

[jhenderson]

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?

[grimar]

In D78304#1991775, @jhenderson wrote:

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]

In D78304#1991909, @grimar wrote:

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.

[grimar]

There is some misunderstanding it seems:

In D78304#1992107, @jhenderson wrote:

In D78304#1991909, @grimar wrote:

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 ?

[jhenderson]

In D78304#1992157, @grimar wrote:

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]

In D78304#1992196, @jhenderson wrote:

In D78304#1992157, @grimar wrote:

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]

• Remove "POffset".

[jhenderson]

llvm/lib/ObjectYAML/ELFEmitter.cpp

763

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

"'Offset' for segment with index"

764

Should this ignore SHT_NOBITS?

765

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.

111

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)

141

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.

149

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.

178

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

[jhenderson]

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

[grimar]

• Addressed review comments.

llvm/lib/ObjectYAML/ELFEmitter.cpp

764

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

111

Ok, thanks.

178

Done.

[grimar]

• Fix comment.

[jhenderson]

LGTM, with nit.

llvm/lib/ObjectYAML/ELFEmitter.cpp

764

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

184

Nit: please add an extra space before FileCheck.

llvm/lib/ObjectYAML/ELFEmitter.cpp

for (auto &YamlPhdr : Doc.ProgramHeaders) {
     Elf_Phdr &PHeader = PHeaders[PhdrIdx++];
     std::vector<Fragment> Fragments = getPhdrFragments(YamlPhdr, SHeaders);
     if (!llvm::is_sorted(Fragments, [](const Fragment &A, const Fragment &B) {
           return A.Offset < B.Offset;
         }))
       reportError("sections in the program header with index " +
                   Twine(PhdrIdx) + " are not sorted by their file offset");
 
-    if (YamlPhdr.Offset)
-      PHeader.p_offset = *YamlPhdr.Offset;
-    else if (!Fragments.empty())
-      PHeader.p_offset = Fragments.front().Offset;
-    else
-      PHeader.p_offset = 0;
+    uint64_t PhdrFileOffset = Fragments.empty() ? 0 : Fragments.front().Offset;
+    if (YamlPhdr.Offset) {
+      if (!Fragments.empty() && *YamlPhdr.Offset > PhdrFileOffset)
+        reportError("'Offset' for segment with index " + Twine(PhdrIdx) +
+                    " must be less than or equal to the minimum file offset of "
+                    "all included sections (0x" +
+                    Twine::utohexstr(PhdrFileOffset) + ")");
+      PhdrFileOffset = *YamlPhdr.Offset;
+    }
+    PHeader.p_offset = PhdrFileOffset;
 
     // Find the maximum offset of the end of a section in order to set p_filesz
     // and p_memsz. When setting p_filesz, trailing SHT_NOBITS sections are not
     // counted.
     uint64_t FileOffset = PHeader.p_offset, MemOffset = PHeader.p_offset;
     for (const Fragment &F : Fragments) {
       uint64_t End = F.Offset + F.Size;
       MemOffset = std::max(MemOffset, End);

llvm/test/Object/invalid.test

 # DYN-TABLE-PHDR: warning: '[[FILE]]': PT_DYNAMIC segment offset + size exceeds the size of the file
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
-Sections:
-  - Name: .dynamic
-    Type: SHT_DYNAMIC
-    Entries:
-      - Tag:   DT_NULL
-        Value: 0
 ProgramHeaders:
   - Type:   PT_DYNAMIC
     Offset: 0xffff0000
-    Sections:
-      - Section: .dynamic
 
 ## PT_DYNAMIC's p_filesz field is so large that p_offset + p_filesz is larger
 ## than the object size. Check llvm-readobj reports it.
 
 # RUN: yaml2obj %s --docnum=23 -o %t23
 # RUN: llvm-readobj --dyn-relocations %t23 2>&1 \
 # RUN:  | FileCheck -DFILE=%t23 --check-prefix=DYN-TABLE-PHDR %s
 

llvm/test/tools/llvm-objcopy/ELF/invalid-p_filesz-p_offset.test

 # ERR2: error: program header with offset 0x100000 and file size 0x1 goes past the end of the file
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
-Sections:
-  - Name: .foo
-    Type: SHT_PROGBITS
-    Size: 1
 ProgramHeaders:
-  - Type:        PT_LOAD
-    Offset:      0x100000
-    FileSize:    1
-    Sections:
-      - Section: .foo
+  - Type:     PT_LOAD
+    Offset:   0x100000
+    FileSize: 1

llvm/test/tools/llvm-readobj/ELF/gnu-notes.test

 # ERR3: error: '[[FILE]]': PT_NOTE header has invalid offset (0xffff0000) or size (0x0)
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_CORE
   Machine: EM_X86_64
-Sections:
-  - Name:  .note
-    Type:  SHT_NOTE
-    Notes: []
 ProgramHeaders:
-  - Type: PT_NOTE
+  - Type:   PT_NOTE
     Offset: 0xffff0000
-    Sections:
-      - Section: .note
 
 ## Test tools report an error if a note program header has an invalid size that
 ## goes past the end of file.
 
 # RUN: yaml2obj --docnum=5 %s -o %t5.so
 # RUN: not llvm-readelf --notes %t5.so 2>&1 | FileCheck -DFILE=%t5.so  %s --check-prefix=ERR4
 # RUN: not llvm-readobj --notes %t5.so 2>&1 | FileCheck -DFILE=%t5.so %s --check-prefix=ERR4
 

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

 ## Show that yaml2obj properly emits program headers with explicit file size,
 ## memory size and offset parameters.
 
-# RUN: yaml2obj %s -o %t
-# RUN: llvm-readobj %t --program-headers | FileCheck %s
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readobj %t1 --program-headers | FileCheck %s
 
 # CHECK: ProgramHeaders [
 # CHECK:    Offset: 0x1234
 # CHECK:    FileSize: 1111
 # CHECK:    MemSize: 9999
 
 # CHECK:    Offset: 0x2000
 # CHECK:    FileSize: 6

 # CHECK:    FileSize: 3
 # CHECK:    MemSize: 2
 
 # CHECK:    Offset: 0x2004
 # CHECK:    FileSize: 4
 # CHECK:    MemSize: 6
 # CHECK: ]
 
-!ELF
+--- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_EXEC
   Machine:         EM_X86_64
 Sections:
   - Name: .text
     Type: SHT_PROGBITS
Sections:
       - Section: .rodata
   # Program header with sections, valid properties.
   - Type:     0x6abcdef0
     Offset:   0xffe
     FileSize: 7
     MemSize:  9
     Sections:
       - Section: .text
-  # Program header with sections, invalid properties.
+  # Program header with invalid properties.
   - Type:     0x6abcdef0
     Offset:   0x3000
     FileSize: 3
     MemSize:  2
-    Sections:
-      - Section: .data
   # Program header with 2 SHT_NOBITS sections.
   - Type:     0x6abcdef0
     Offset:   0x2004
     Sections:
       - Section: .data
       - Section: .nobits1
       - Section: .nobits2
+
+## Test the "Offset" property.
+
+## Check that by default the p_offset field of a segment is set to the
+## offset of the section with the minimum offset.
+# RUN: yaml2obj --docnum=2 %s -o %t2
+# RUN: llvm-readelf %t2 --sections --program-headers | \
+# RUN:   FileCheck %s --check-prefixes=DEFAULT-OFFSET
+
+# DEFAULT-OFFSET:      [Nr] Name Type     Address          Off
+# DEFAULT-OFFSET:      [ 1] .foo PROGBITS 0000000000001000 0000b0
+# DEFAULT-OFFSET-NEXT: [ 2] .bar PROGBITS 0000000000001001 0000b1
+
+# DEFAULT-OFFSET:       Type Offset
+# DEFAULT-OFFSET-NEXT:  LOAD 0x0000b0
+# DEFAULT-OFFSET-NEXT:  LOAD 0x0000b1
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    .foo
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC ]
+    Size:    0x1
+    Address: 0x1000
+  - Name:  .bar
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_ALLOC ]
+    Size:  0x1
+ProgramHeaders:
+  - Type: PT_LOAD
+    Sections:
+      - Section: .foo
+      - Section: .bar
+  - Type: PT_LOAD
+    Sections:
+      - Section: .bar
+
+## Check we can set the "Offset" value explicitly to be less than or equal to
+## the offset of a section in the segment.
+# RUN: yaml2obj --docnum=3 -DOFFSET=0x77 %s -o %t3
+# RUN: llvm-readelf %t3 --sections --program-headers | \
+# RUN:   FileCheck %s --check-prefixes=VALID-OFFSET,VALID-OFFSET-LESS
+# RUN: yaml2obj --docnum=3 -DOFFSET=0x78 %s -o %t4
+# RUN: llvm-readelf %t4 --sections --program-headers | \
+# RUN:   FileCheck %s --check-prefixes=VALID-OFFSET,VALID-OFFSET-EQ
+
+# VALID-OFFSET: [Nr] Name Type     Address          Off
+# VALID-OFFSET: [ 1] .foo PROGBITS 0000000000000000 000078
+
+# VALID-OFFSET:      Type Offset
+# VALID-OFFSET-EQ:   LOAD 0x000078
+# VALID-OFFSET-LESS: LOAD 0x000077
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:  .foo
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_ALLOC ]
+    Size:  0x1
+ProgramHeaders:
+  - Type: PT_LOAD
+    Offset: [[OFFSET]]
+    Sections:
+      - Section: .foo
+
+## Check we report an error when the "Offset" value is larger than the offset of a section in the segment.
+# RUN: not yaml2obj --docnum=3 -DOFFSET=0x79 %s -o /dev/null 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=INVALID-OFFSET
+
+# INVALID-OFFSET: yaml2obj: error: 'Offset' for segment with index 1 must be less than or equal to the minimum file offset of all included sections (0x78)
+
+## Document that the "Offset" value is checked after the section offset is overriden using "ShOffset".
+# RUN: yaml2obj --docnum=4 %s -o %t5
+# RUN: llvm-readelf %t5 --sections --program-headers | FileCheck %s --check-prefix=SHOFFSET
+
+# SHOFFSET: [Nr] Name Type     Address          Off
+# SHOFFSET: [ 1] .foo PROGBITS 0000000000000000 ffffffff
+
+# SHOFFSET:      Type Offset
+# SHOFFSET-NEXT: LOAD 0xffffff00
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:     .foo
+    Type:     SHT_PROGBITS
+    Flags:    [ SHF_ALLOC ]
+    Size:     0x1
+## Note: the real .foo offset is much less than 0xFFFFFFFF or
+##       0xFFFFFF00, but no error is reported.
+    ShOffset: 0xFFFFFFFF
+ProgramHeaders:
+  - Type:   PT_LOAD
+    Offset: 0xFFFFFF00
+    Sections:
+      - Section: .foo