Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework file shifting to avoid sections crossing multiple segments #415

Merged
merged 1 commit into from
Oct 26, 2022

Conversation

Bo98
Copy link
Contributor

@Bo98 Bo98 commented Oct 10, 2022

Fixes #403.

The previous logic was incorrect in some cases.

The way it worked before is we chose a point where to create a gap (startOffset), just after a .dynstr or the last non-interp PROGBITS. We then calculated how much space to add, move everything and create a PT_LOAD with that size.

The size of that PT_LOAD was incorrect however, as it only took into account the additional space we were adding - it did not take into account the space before startOffset, so it could be too small. The offsets of subsequent segments had similar logic. This created the potential for a section to overlap between segments.

To fix this, I've changed it so we don't add a new segment at all. Instead, we find the existing PT_LOAD segment that contains startOffset and increase the size of that by the calculated amount. Everything after startOffset is moved and has offsets adjusted as before, while everything before is now kept as is with only virtual memory offsets adjusted.

@Bo98 Bo98 force-pushed the load-overlap-fix branch 5 times, most recently from 2b8f1b9 to a86f590 Compare October 12, 2022 17:29
@Mic92
Copy link
Member

Mic92 commented Oct 13, 2022

This seems to cause some crashes now. You should be able to reproduce by running make check.

@Bo98
Copy link
Contributor Author

Bo98 commented Oct 13, 2022

I'm not actually:

============================================================================
Testsuite summary for patchelf 0.15.0
============================================================================
# TOTAL: 48
# PASS:  48
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

Is there anything in particular I about the CI environment that I should be aware of in order to reproduce?

@Bo98
Copy link
Contributor Author

Bo98 commented Oct 13, 2022

Ah figured it out - it was Nix specific for some reason.

The original code made the new segment have RW flags. The new code didn't touch the flag so the original segment could have had just R.

So what I've done is instead produce two split segments like before rather than one large one, but still largely keeping the newer fixed logic that calculates the point to split at properly. This will reduce the diff a little and make it easier to compare.

@Bo98 Bo98 force-pushed the load-overlap-fix branch 4 times, most recently from 176d10b to 1eb209d Compare October 13, 2022 17:18
@Mic92 Mic92 merged commit af77f12 into NixOS:master Oct 26, 2022
@Mic92
Copy link
Member

Mic92 commented Oct 26, 2022

Thanks!

@haampie
Copy link
Contributor

haampie commented Oct 26, 2022

Thanks! Is it worth backporting this? Or cutting a new release?

@Mic92
Copy link
Member

Mic92 commented Oct 26, 2022

Yes, I think a new release is due.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PT_NOTE logic in normalizeNoteSegments causes .dynstr not to be contained fully in a segment
3 participants