The libelf file in base fails to perform elf_update when growing the file by adding new sections, because elf_getdata erroneously checks section boundaries against the old file size.
This can be fixed by omitting this check when doing elf_update.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Do you have a straightforward testcase that can be used to reproduce the issue (and the one in D10486)?
| contrib/elftoolchain/libelf/elf_data.c | ||
|---|---|---|
| 33 | alpha order | |
Comment Actions
Awaiting review; I pointed out a couple of minor nits to address on commit
| contrib/elftoolchain/libelf/_libelf.h | ||
|---|---|---|
| 235–236 | alpha order, no space between * and func | |
Comment Actions
I'm not sure why whatever updates the s_shdr doesn't also update e_rawsize to maintain the invariant.
The rawdata change seems spurious. It isn't used, as far as I can tell.
I don't see any other issues with the implementation here, other than that we're removing an invariant in the one case.
| contrib/elftoolchain/libelf/_libelf.h | ||
|---|---|---|
| 32–34 | Usually sys headers come before standard C ones | |
| 235–236 | Space after * (return type) does not match existing file style | |
Comment Actions
The changes from the former revision look good, modulo one nitpick/question below. My top-level questions still stand, though:
- Why not have updates to s_shdr update e_rawsize to maintain the invariant, instead of this?
- The change to elf_rawdata() seems spurious and unused. Is it? If you have plans to use it in a later patch, just let me know. But if there aren't plans, I don't see changing it as useful.
| contrib/elftoolchain/libelf/_libelf.h | ||
|---|---|---|
| 228 | Hard to tell from Phab, but it looks like other declarations have a tab between the underlying type and the * or function name, and the new ones may not? If they do, ignore this. | |
Comment Actions
OK, I reverted the rawdata change.
I'm not sure what you mean by the invariant. All this patch does is detect a violation and report an error in that case.
Comment Actions
From @jkoshy_users.sourceforge.net mail,
Per my understanding, the ELF(3) API requires users to run elf_update(..., ELF_C_NULL) after making structural changes to the ELF object's contents (e.g., after adding a new section, moving sections around, etc).
It isn't clear if the behavior in the bug was caused by incorrect use of the API -- in which case we would need to improve our documentation -- or whether this was a genuine bug in libelf. A small test case would help.
Comment Actions
This change relaxes the error condition sh_offset > e->e_rawsize || sh_size > e->e_rawsize - sh_offset to only apply if !updating. Could we instead correct e_rawsize when we are updating?
Comment Actions
I still don't understand what this change is attempting to do.
The proposed changes seem to handle the case where:
- the ELF object had been opened in modes ELF_C_READ or ELF_C_RDWR,
- a new section was added, with extents set to fall outside the ELF object,
- but no Elf_Data descriptors were associated with this new section (i.e., elf_newdata() was not called).
However, this would be incorrect usage of the ELF(3) API: if a new section is added to an Elf descriptor and if the section falls outside the current object, then elf_newdata() would need to be called on the section to actually associate data with the new byte range.
It could be that I have misunderstood this patch. A short, correct, compilable example (sscce.org) would help me understand the issue better.