bootloader: do not overwrite/lose LD_LIBRARY_PATH #2148

ThomasWaldmann · 2016-08-17T15:04:39Z

rather:

prepend our path to the existing path
save the original path in the environment

also:
implement pyi_strjoin (oh the joys of string processing in C!)

for more info, see #625, #1759 and the comments in the changeset.

htgoebel · 2016-08-17T15:24:46Z

Thanks for this fix. Not being a C-programmer, this basically looks good for me. Would you please provide some doc-update, too describing the new env-var?

htgoebel · 2016-08-17T15:26:20Z

bootloader/src/pyi_utils.c

+     */
+    int first_len=0, sep_len=0, second_len=0;
+    char *result;
+    if(first)


Please add a space in front of the parent and put the body into {…;}. This is how it is done in the remaining code.

htgoebel · 2016-08-17T16:11:21Z

Looks good for me. @bjones1 What do you think?

ThomasWaldmann · 2016-08-17T16:22:06Z

Note: I am currently running tests in vagrant VM (first on debian wheezy, where we discovered the problem with fakeroot, afterwards on freebsd).

bjones1 · 2016-08-17T16:25:09Z

Looks fine. But @codewarrior0 is really the bootloader expert...

ThomasWaldmann · 2016-08-17T16:39:06Z

vagrant@vagrant:/vagrant/borg$ fakeroot ./borg.exe --version
ERROR: ld.so: object 'libfakeroot-sysv.so' from LD_PRELOAD cannot be preloaded: ignored.
ERROR: ld.so: object 'libfakeroot-sysv.so' from LD_PRELOAD cannot be preloaded: ignored.
ERROR: ld.so: object 'libfakeroot-sysv.so' from LD_PRELOAD cannot be preloaded: ignored.
ERROR: ld.so: object 'libfakeroot-sysv.so' from LD_PRELOAD cannot be preloaded: ignored.
borg.exe 1.1.dev339+ng0e945a1

vagrant@vagrant:/vagrant/borg$ fakeroot ./borg2.exe --version
borg2.exe 1.1.dev339+ng0e945a1

borg.exe was (accidentally) built with the bundled and still as-before 32bit linux bootloader (because it first failed building it due to a missing --no-lsb option in my Vagrantfile).

borg2.exe was built with the modified bootloader.

Update: Yay, 14 test failures less (our tests were a bit disturbed by the unexpected output on stderr).

dimaqq · 2016-08-17T17:04:59Z

bootloader/src/pyi_utils.c

+    VS("LOADER: %s=%s\n", env_var_orig, orig_path);
+
+    /* prepend our path to the original path */
+    new_path = pyi_strjoin(path, ":", orig_path);


new_path needs to be freed somewhere...

right, copies will be made, so we can free this one.

How about this?

new_path = alloc(strlen(path) + 1 + strlen(orig_path) + 1); sprintf(new_path, "%s:%s", path, orig_path);

I think this is the much simpler solution. (the double +1 could just be a +2, but I put it this way to signify why it's there)

+1 for PlasmaPower.
we could even use alloca() considering there's a practical limit on PATH length which is, now, half of env.

this does not consider the case when the current env var value is empty.

(or when the first param is an empty string, which can not be the case for this caller)

if (orig_path && *orig_path != '\0') { new_path = alloca(strlen(path) + 1 + strlen(orig_path) + 1); sprintf(new_path, "%s:%s", path, orig_path); } else { new_path = path; }

Then don't free it.

@PlasmaPower (github @ completion does not like you, btw): i would not advocate for having 6 lines that need to be copy&paste at every place where string join is needed, but rather have the simple 1-line function call.

With alloca it can't be in a function as that memory is only valid within that function.

Makes sense.

ThomasWaldmann · 2016-08-17T23:52:44Z

OK, I'm done. Decide yourself whether you rather like first or first+second changeset. First one is easier, first+second saves some cpu cycles, but has more LoC.

Can we have this in a 3.2.1 soon? Without this our unit tests are partially broken and it would be nice not having to patch it and build the bootloaders with LSB stuff via vagrant.

Please don't forget to rebuild all the bootloader binaries with this.

codewarrior0 · 2016-08-18T00:39:48Z

In the docs for pyi_strjoin, I would have put "This function returns a null-terminated string which the caller is responsible for freeing. Returns NULL if memory could not be allocated."

Other than that, looks good to me!

rather: - prepend our path to the exisiting path - save the original path in the environment also: - update docs - implement pyi_strjoin (oh the joys of string processing in C!) Likely, this fixes pyinstaller#625 and helps with pyinstaller#1759.

ThomasWaldmann · 2016-08-18T12:49:26Z

@codewarrior0 yup, added docs about return value.

ThomasWaldmann · 2016-08-18T13:09:42Z

found a more compact way to compute the length values.

ThomasWaldmann · 2016-08-18T13:19:45Z

ok, now really finished. before merge, tell me whether you want to have it micro-optimized (see commits) or not, then I'll do a final rebase -i and collapse all the wanted changesets.

codewarrior0 · 2016-08-19T07:12:48Z

whether you want to have it micro-optimized

Sure, I don't see why not. Let me know when it's ready.

ThomasWaldmann · 2016-08-19T12:08:50Z

@codewarrior0 I'm done.

see pyinstaller/pyinstaller#2148 also: use waf --no-lsb (the bootloader does not build without it on our wheezy32 vagrant VM)

codewarrior0 · 2016-08-20T00:32:06Z

Thanks!

see pyinstaller/pyinstaller#2148 also: use waf --no-lsb (the bootloader does not build without it on our wheezy32 vagrant VM)

see pyinstaller/pyinstaller#2148

see pyinstaller/pyinstaller#2148 also: use waf --no-lsb (the bootloader does not build without it on our wheezy32 vagrant VM)

see pyinstaller/pyinstaller#2148

ThomasWaldmann · 2016-08-21T21:06:39Z

@htgoebel could you rebuild the Linux bootloader binaries please?

Would make my Vagrantfile less complicated. :)

htgoebel · 2016-08-22T12:57:20Z

@ThomasWaldmann done. Thanks for this fix.

pyinstaller/pyinstaller#2148 This fixed a bug that was most apparent in the linux binary. It would manifest as being unable to launch openvpn as a subprocess shell commands because of the dirty environment.

ThomasWaldmann force-pushed the do-not-overwrite-LD_LP branch from 6724dc1 to 8707bd8 Compare August 17, 2016 15:07

This was referenced Aug 17, 2016

set_dynamic_library_path change breaks plugin library use #625

Closed

LD_LIBRARY_PATH mod causes trouble for forked system applications #1759

Closed

borg binary tests failing on wheezy32 borgbackup/borg#1416

Closed

htgoebel added the area:bootloader Caused be or effecting the bootloader label Aug 17, 2016

htgoebel reviewed Aug 17, 2016
View reviewed changes

ThomasWaldmann changed the title ~~bootloader: do not overwrite and lose LD_LIBRARY_PATH~~ WIP: bootloader: do not overwrite/lose LD_LIBRARY_PATH Aug 17, 2016

dimaqq reviewed Aug 17, 2016
View reviewed changes

ThomasWaldmann force-pushed the do-not-overwrite-LD_LP branch 2 times, most recently from d26cbc7 to 271db7b Compare August 17, 2016 22:26

ThomasWaldmann changed the title ~~WIP: bootloader: do not overwrite/lose LD_LIBRARY_PATH~~ bootloader: do not overwrite/lose LD_LIBRARY_PATH Aug 17, 2016

htgoebel assigned codewarrior0 Aug 18, 2016

ThomasWaldmann force-pushed the do-not-overwrite-LD_LP branch from bf0073d to 6589af3 Compare August 18, 2016 12:49

ThomasWaldmann force-pushed the do-not-overwrite-LD_LP branch from 6589af3 to 4b1d81e Compare August 18, 2016 13:08

ThomasWaldmann added 2 commits August 18, 2016 15:16

fixup: more compact length computation

7ae6e37

micro-optimize pyi_strjoin for no good reason

1fab36c

ThomasWaldmann force-pushed the do-not-overwrite-LD_LP branch from 4b1d81e to 1fab36c Compare August 18, 2016 13:17

ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this pull request Aug 19, 2016

use patched LDLP-preserving pyinstaller

ea80bec

see pyinstaller/pyinstaller#2148 also: use waf --no-lsb (the bootloader does not build without it on our wheezy32 vagrant VM)

ThomasWaldmann mentioned this pull request Aug 19, 2016

use patched LDLP-preserving pyinstaller borgbackup/borg#1507

Merged

codewarrior0 merged commit 639fcec into pyinstaller:develop Aug 20, 2016

ThomasWaldmann deleted the do-not-overwrite-LD_LP branch August 20, 2016 09:03

ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this pull request Aug 20, 2016

use patched LDLP-preserving pyinstaller

82c64d5

see pyinstaller/pyinstaller#2148 also: use waf --no-lsb (the bootloader does not build without it on our wheezy32 vagrant VM)

ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this pull request Aug 20, 2016

restore original LDLP, if possible

c31ebf5

see pyinstaller/pyinstaller#2148

ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this pull request Aug 20, 2016

restore original LDLP, if possible, fixes borgbackup#1498

e7fccac

see pyinstaller/pyinstaller#2148

vishnu2kmohan mentioned this pull request Aug 29, 2016

The binary versions of the DC/OS CLI are affected by PyInstaller LD_LIBRARY_PATH issues: #625, #1759 dcos/dcos-cli#746

Closed

dvj mentioned this pull request Dec 12, 2016

update 64-bit bootloaders for Linux, macOS, and Windows #2310

Closed

almarklein mentioned this pull request Feb 3, 2020

Library mismatch between pyzo and system pyzo/pyzo#665

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootloader: do not overwrite/lose LD_LIBRARY_PATH #2148

bootloader: do not overwrite/lose LD_LIBRARY_PATH #2148

ThomasWaldmann commented Aug 17, 2016 •

edited

htgoebel commented Aug 17, 2016

htgoebel Aug 17, 2016

htgoebel commented Aug 17, 2016

ThomasWaldmann commented Aug 17, 2016 •

edited

bjones1 commented Aug 17, 2016

ThomasWaldmann commented Aug 17, 2016 •

edited

dimaqq Aug 17, 2016

ThomasWaldmann Aug 17, 2016

PlasmaPower Aug 17, 2016 •

edited

dimaqq Aug 17, 2016

ThomasWaldmann Aug 17, 2016 •

edited

PlasmaPower Aug 17, 2016 •

edited

ThomasWaldmann Aug 17, 2016

PlasmaPower Aug 18, 2016

ThomasWaldmann commented Aug 17, 2016 •

edited

codewarrior0 commented Aug 18, 2016

ThomasWaldmann commented Aug 18, 2016

ThomasWaldmann commented Aug 18, 2016

ThomasWaldmann commented Aug 18, 2016

codewarrior0 commented Aug 19, 2016

ThomasWaldmann commented Aug 19, 2016

codewarrior0 commented Aug 20, 2016

ThomasWaldmann commented Aug 21, 2016

htgoebel commented Aug 22, 2016

bootloader: do not overwrite/lose LD_LIBRARY_PATH #2148

bootloader: do not overwrite/lose LD_LIBRARY_PATH #2148

Conversation

ThomasWaldmann commented Aug 17, 2016 • edited

htgoebel commented Aug 17, 2016

htgoebel Aug 17, 2016

Choose a reason for hiding this comment

htgoebel commented Aug 17, 2016

ThomasWaldmann commented Aug 17, 2016 • edited

bjones1 commented Aug 17, 2016

ThomasWaldmann commented Aug 17, 2016 • edited

dimaqq Aug 17, 2016

Choose a reason for hiding this comment

ThomasWaldmann Aug 17, 2016

Choose a reason for hiding this comment

PlasmaPower Aug 17, 2016 • edited

Choose a reason for hiding this comment

dimaqq Aug 17, 2016

Choose a reason for hiding this comment

ThomasWaldmann Aug 17, 2016 • edited

Choose a reason for hiding this comment

PlasmaPower Aug 17, 2016 • edited

Choose a reason for hiding this comment

ThomasWaldmann Aug 17, 2016

Choose a reason for hiding this comment

PlasmaPower Aug 18, 2016

Choose a reason for hiding this comment

ThomasWaldmann commented Aug 17, 2016 • edited

codewarrior0 commented Aug 18, 2016

ThomasWaldmann commented Aug 18, 2016

ThomasWaldmann commented Aug 18, 2016

ThomasWaldmann commented Aug 18, 2016

codewarrior0 commented Aug 19, 2016

ThomasWaldmann commented Aug 19, 2016

codewarrior0 commented Aug 20, 2016

ThomasWaldmann commented Aug 21, 2016

htgoebel commented Aug 22, 2016

ThomasWaldmann commented Aug 17, 2016 •

edited

ThomasWaldmann commented Aug 17, 2016 •

edited

ThomasWaldmann commented Aug 17, 2016 •

edited

PlasmaPower Aug 17, 2016 •

edited

ThomasWaldmann Aug 17, 2016 •

edited

PlasmaPower Aug 17, 2016 •

edited

ThomasWaldmann commented Aug 17, 2016 •

edited