Page MenuHomeFreeBSD

Bug 218861 - libelf elf_update fails when adding sections
Needs ReviewPublic

Authored by on Apr 24 2017, 11:21 PM.



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.

Diff Detail

rS FreeBSD src repository
Lint Skipped
Unit Tests Skipped

Event Timeline updated this revision to Diff 27706.
emaste added a subscriber: emaste.

Do you have a straightforward testcase that can be used to reproduce the issue (and the one in D10486)?

emaste added a subscriber: cem.Aug 1 2018, 4:41 PM
emaste added inline comments.

alpha order

emaste added a comment.Aug 1 2018, 6:17 PM

Awaiting review; I pointed out a couple of minor nits to address on commit


alpha order, no space between * and func

cem added a comment.Aug 1 2018, 7:31 PM

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.


Usually sys headers come before standard C ones


Space after * (return type) does not match existing file style marked 4 inline comments as done.
cem added a comment.Aug 23 2018, 3:44 PM

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.

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. updated this revision to Diff 47197.

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.

From 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.

cem added a comment.EditedSep 1 2018, 3:43 AM

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.

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?

I still don't understand what this change is attempting to do.

The proposed changes seem to handle the case where:

  1. the ELF object had been opened in modes ELF_C_READ or ELF_C_RDWR,
  2. a new section was added, with extents set to fall outside the ELF object,
  3. 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 ( would help me understand the issue better.