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

off-by-one error leads to space at the end of every path #9

Closed
ryandesign opened this issue May 31, 2015 · 5 comments
Closed

off-by-one error leads to space at the end of every path #9

ryandesign opened this issue May 31, 2015 · 5 comments

Comments

@ryandesign
Copy link
Contributor

There is an off-by-one error that causes every path processed to have a space at the end of it (mentioned in a comment in #2).

The problem is that you find the position of the string " (" in the output of otool, and then feed that to the second argument of substr, but the second argument of substr expects a length, not a position. If the start position were 0 that wouldn't be a problem, but the start position is 1 (to remove the leading tab character), so you need to substract that 1 from the found position to get the length.

Also, find finds the first occurrence of the string. What if a path for some reason contained that string " ("? Better to use rfind which finds the last occurrence of the string.

This patch fixes these problems:

https://trac.macports.org/browser/trunk/dports/devel/dylibbundler/files/patch-src-DylibBundler.cpp.diff?rev=136949

In addition, you should revert fef319a which is not correct. fef319a will cause problems with files whose names actually do end with a space, though that's of course very unlikely to ever occur in practice.

@auriamg
Copy link
Owner

auriamg commented May 31, 2015

Thanks for the patch, I've applied it

@ryandesign
Copy link
Contributor Author

Thanks. Will you still revert fef319a?

@auriamg
Copy link
Owner

auriamg commented Jun 7, 2015

Regarding fef319a I am unsure; it seems like it was originally committed with a specific purpose in mind, so reverting it may break that (even though I'm not sure why). Given that filenames ending with a space should be excessively rare, I would favor the status quo - after all, much of dylibbundler is slightly hackish anyway and will fall apart if you try hard enough

@ryandesign
Copy link
Contributor Author

So let's fix the hacks!

fef319a was an incorrect attempt to fix the problem now correctly fixed in this ticket in 8eb915a / 0d8c739.

None of dylibbundler will currently work with files whose names begin or end with or even contain spaces, because it doesn't properly escape file arguments anytime it runs a system command. But if all those were fixed, which I'd like to work on next, then you'd see that fef319a will be the reason why things don't work for those hypothetical files whose names end with spaces.

Implementing a test suite would be a great way to verify the program works correctly for various inputs...

@auriamg
Copy link
Owner

auriamg commented Jun 8, 2015

If you feel like improving the hacks, you are more than welcome! As for myself dylibbundler fills my current needs so I have no plans to do any more significant work on it. But if someone else would like to keep working on it that would be great

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

No branches or pull requests

2 participants