libelf: Fix extended numbering detection

[cem]

Summary

Extended numbering is used for any of these fields overflowing.

[emaste]

contrib/elftoolchain/libelf/libelf_ehdr.c

196 ↗

(On Diff #22688)

What's the story with the or in the previous case?

[cem]

contrib/elftoolchain/libelf/libelf_ehdr.c

190 ↗

(On Diff #22688)

This check should be shnum == 0, not !=.

196 ↗

(On Diff #22688)

My change to it is slightly wrong. I think the shoff check should just be removed.

The directly preceding if statement verifies that extended ELF objects have a section header table (shoff != 0). (Modulo a small bug.)

So in this condition, we can just check for an non-extended ELF object directly (shnum != 0 && phnum != PN_XNUM && strndx != SHN_XINDEX) and ignore shoff.

[cem]

Fix extended ELF => section header invariant; remove useless shoff check.

[emaste]

contrib/elftoolchain/libelf/libelf_ehdr.c

173–176 ↗

(On Diff #22710)

I suspect this comment belongs below instead.

190 ↗

(On Diff #22710)

I think shnum != 0 is correct. We're looking for invalid values or combinations of values.

• shnum >= SHN_LORESERVE is invalid
• if shoff == 0LL we have no section headers, so the file is invalid if:
  • we have a section header count (shnum != 0)
  • phnum == PN_XNUM indicating extended numbering for the phdr count
  • strndx == SHN_XINDEX indicating extended numbering for the string section

Or maybe the test should be

(shoff == 0 || shnum == 0) && (phnum == PN_XNUM || strndx == SHN_XINDEX)

196 ↗

(On Diff #22688)

I think if we change to the test suggested above this can be just phnum != PN_XNUM && strndx != SHN_XINDEX

[cem]

contrib/elftoolchain/libelf/libelf_ehdr.c

173–176 ↗

(On Diff #22710)

Yeah, agreed.

190 ↗

(On Diff #22710)

I think you're right re: shnum != 0.

http://docs.oracle.com/cd/E19253-01/817-1984/chapter6-46512/index.html:

If a file has no section header table, e_shnum holds the value zero.

If the number of sections is greater than or equal to SHN_LORESERVE (0xff00), e_shnum has the value zero.

So zero is overloaded and we need shoff to determine whether this is an extended shnum or empty section header table.

I don't think the proposed test is correct, since shoff != 0 && shnum == 0 && phnum == PN_XNUM is valid (both shnum and phnum are extended).

196 ↗

(On Diff #22688)

Both are wrong, I think.

We need phnum != PN_XNUM && strndx != SHN_XINDEX and also need to exclude objects with only extended shnum (shnum == 0 && shoff != 0).

[cem]

• Move old comment down to a more correct location.
• Revert change in shnum sense. It was right before.
• Fix check for extended values and simplify boolean expression by inverting the sense.

[emaste]

contrib/elftoolchain/libelf/libelf_ehdr.c

196 ↗

(On Diff #22727)

Should this be the opposite of the test above, i.e. shoff != 0 && (shnum == 0 || phnum == PN_XNUM || strndx == SHN_XINDEX)?

[cem]

contrib/elftoolchain/libelf/libelf_ehdr.c

196 ↗

(On Diff #22727)

Those should be functionally identical after the previous if statement/return.

[emaste]

I believe this is correct. @kaiw if you're going to review as well I'll hold off on committing it to the upstream elftoolchain repo until then.

contrib/elftoolchain/libelf/libelf_ehdr.c

196 ↗

(On Diff #22727)

I was thinking the intent would be more clear with the other formulation, but I suppose shnum == 0 && shoff != 0 is a complete condition for one case of extended numbering. OK.

[cem]

@kaiw, ping?