Compute program header flags in rewriteSectionsLibrary #467
Conversation
Use reference instead of a copy of the struct as lambda argument. Use for loop to limit the scope of the iterator varible i.
Instead of using fixed flags of RW for the new program header, holding the replaced sections, check if the write or execute flags is required by any accompanying section. If many sections are going to be replaced a more strict method would be to split them up and add up-to 4 new program headers if necessary: read-only, read-write, read-execute, read-write-execute.
|
cc @brenoguim |
There was a problem hiding this comment.
I'm glad that this is being done, because it's a bit concerning that some sections are exposed with different access rights. Even though it doesn't fix everything, it's already a good improvement.
This is the kind of bug that would fit in perfectly with #468 because it would be lovely have a test for it. And in the current state of things, it would be cumbersome to create a reliable one.
| for (const auto & rs : replacedSections) { | ||
| const auto flags = findSectionHeader(rs.first).sh_flags; | ||
| if (flags & SHF_WRITE) | ||
| need_write = true; | ||
| if (flags & SHF_EXECINSTR) | ||
| need_exec = true; | ||
| } | ||
| wri(phdr.p_flags, PF_R | (need_write ? PF_W : 0) | (need_exec ? PF_X : 0)); |
There was a problem hiding this comment.
Another way to write this would be:
auto flagUnion = PF_R;
for (const auto& rs : replacedSections) {
const auto flags = findSectionHeader(rs.first).sh_flags;
flagUnion |= (flags & SHF_WRITE) ? PF_W : 0;
flagUnion |= (flags & SHF_EXECINSTR) ? PF_X : 0;
}
wri(phdr.p_flags, flagUnion);
But that's style, feel free to drop.
Since these flags are not being tracked, RW is the "works for all". I don't think we ever move executable sessions (because they are usually progbits) so we never needed.
I think so. I would expect things to break drastically if endianess is not respected. Do you know if we have tests for that? |
Instead of using fixed flags of RW for the new program header, holding
the replaced sections, check if the write or execute flags is required
by any accompanying section.
If many sections are going to be replaced a more strict method would be
to split them up and add up-to 4 new program headers if necessary:
read-only, read-write, read-execute, read-write-execute.
p.s.:
also the harded flags for executables seems suspicious:
patchelf/src/patchelf.cc
Line 562 in e37f892
Maybe that should be
wri(phdr.p_flags, rdi(phdrs.at(splitIndex).p_flags));?p.p.s:
Should these lines convert the read data
patchelf/src/patchelf.cc
Lines 558 to 560 in e37f892
e.g.
?