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

Overwrite soname in place if there is room #159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reidpr
Copy link

@reidpr reidpr commented Sep 27, 2018

This addresses issue #44 for the case where the new soname is the same or shorter than the old.

It's not great code. I'm not a C++ programmer, and I couldn't figure out how to make the overwrite idiomatic. So, it uses C string functions.

But, it does work for us.

Copy link

@cliffwoolley cliffwoolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This worked for me, thanks.

Cliff Woolley
NVIDIA Corp

@haampie
Copy link
Contributor

haampie commented Mar 31, 2020

Any reason this was not merged?

@domenkozar
Copy link
Member

It would be great if you can create a regression test case in tests directory.

@reidpr
Copy link
Author

reidpr commented Jun 3, 2020

@domenkozar I agree 100%. Is there documentation on how to run the tests and how to write a new one? I didn't see anything at first glance. I don't have much time for this, and I apologize for that, but I don't see me adding a test unless it's very clear and straightforward.

Thanks for your hard work on patchelf! It's extremely useful for us.

@domenkozar
Copy link
Member

@reidpr see 81c0ea3

You can see a bunch of examples there, you'll also need to provide a sample binary to test.

@reidpr
Copy link
Author

reidpr commented Jun 4, 2020

OK, thanks for your help. make check I can handle but to write a test I need clear step-by-step instructions. I see there is a bunch of stuff in the tests directory, but I don't have time to reverse engineer the procedure to write a new test for an unfamiliar code base.

@domenkozar
Copy link
Member

@reidpr if you provide a binary and a small bash script to call patchelf and assert it works (and confirm it doesn't with the old patchelf) that should be sufficient.

issue #44. */
if (newSoname.size() <= sonameSize) {
debug("overwriting old SONAME with new...\n");
strcpy(soname, newSoname.c_str());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already know the size of newSoname, memcpy is the better choice:

Suggested change
strcpy(soname, newSoname.c_str());
memcpy(soname, newSoname.c_str(), newSoname.size() + 1);

debug("overwriting old SONAME with new...\n");
strcpy(soname, newSoname.c_str());
changed = true;
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functionality will have to change now @reidpr
I introduced new changes that always do this->rewriteSections(); after each changed = true This is to support multiple commands on a single invocation.

I agree that this is not an ideal tracking each return.
I would like to figure out how to do a destructor for each method and then apply this->rewriteSections(); only if changed == true.

Either way this code will produce errors for now if done with other commands.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened https://github.com/NixOS/patchelf/pull/363/files# to help make this simpler.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, thanks for the heads up. I still hope (after 3 years) this PR will get merged, but I don't have time to maintain it.

Copy link
Contributor

@fzakaria fzakaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my latest message.

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.

6 participants