Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#299 closed defect (fixed)

python: Special BEGINLIBPATH processing is broken

Reported by: dmik Owned by:
Priority: major Milestone:
Component: python Version:
Severity: high Keywords:
Cc:

Description

At least with subprocess.Popen() setting BEGINLIBPATH with either os.putenv or passing an env dictionary with this variable set has no effect. It used to work and could be broken by the latest subprocess changes (like r1281).

Change History (5)

comment:1 Changed 6 years ago by dmik

Actually, it seems that it never worked right. As it's known, these vars are pseudo-vars and there is a broken mess of different approaches in attempts to sync these pseudo-vars with the contents of os.environ wrt to various calls where these vars are used. Will take some time to sort it out.

BTW, I discovered when trying to run mach cppunittest in Firefox where it is needed to locate XUL.DLL in tests that are statically linked to it.

comment:2 Changed 6 years ago by dmik

Sorted out a few things:

  1. Actual values of BEGINLIBPATH, ENDLIBPATH and LIBPATHSTRICT are read with DosQueryExtLIBPATH and put into os.environ at Python process startup.
  2. os.putenv understands BEGINLIBPATH, ENDLIBPATH and LIBPATHSTRICT and properly calls DosSetExtLIBPATH for them.
  3. os.putenv is also used whenever the os.environ dictionary is modified directly so doing so for those vars is also fine.
  4. os.putenv, however, does not update os.environ with the new values of BEGINLIBPATH, ENDLIBPATH and LIBPATHSTRICT, so both os.getenv and os.environ continue to return the old value (that was there at program startup). This is clearly wrong (inconsistent with the initialization behavior, at least) and needs to be fixed.
  5. os.execve filters out BEGINLIBPATH, ENDLIBPATH and LIBPATHSTRICT from the environment list passed to the underlying LIBC call (and down to DosExecPgm?) to avoid confusing started programs as these vars are not real environment variables. However, neither os.spawnve nor my new os.spawn2 do the same. They should do it too, so a fix is needed.
  6. After BEGINLIBPATH, ENDLIBPATH and LIBPATHSTRICT are filtered out by os.execve from the environment list, they are merely ignored, no calls to DosSetEXTLIBPATH are made. This means that passing these vars in the environment dict to os.execve (as opposed to e.g calling os.putenv and then os.execv) has no effect on the started process (and it will fail to locate the DLL if the calling code relies on BEGINLIBPATH for that). os.spawnve and os.spawn2 don't do filtering but they don't call DosSetEXTLIBPATH either. This is clearly wrong too and needs a fix. And this is the reason why mach cppunittest in Firefox fails — because it passes the path to XUL.DLL to subprocess.Popen (which now uses os.spawn2) via the environment dict rather than via os.putenv (which of course makes perfect sense).

comment:3 Changed 6 years ago by dmik

Re 4, unfortunately, this is the documented behavior, see https://docs.python.org/2/library/os.html#os.environ. So I will leave it as is. This also means that in Python os.putenv, os.unsetenv should not be used directly: the os.environ dict should be always modified instead. Note that os.getenv(NAME) is basically an alias to os.environ[NAME] but there is a difference: os.getenv will return None if the requested variable does not exist but accessing os.environ[NAME] will throw an exception. In order to avoid the exception the dict should be accessed as os.environ.get(NAME, None) rather than by index.

comment:4 Changed 6 years ago by dmik

Resolution: fixed
Status: newclosed

5 and 6 are done in r1294 but only for os.execve and os.spawnve as for os.spawn2 it's better to be done in LIBCx to provide thread-safety in P_2_THREADSAFE mode. And since subprocess.Popen uses os.spawn2, Firefox has to wait for a LIBCx fix.

I also disabled polluting os.environ with unset BEGINLIBPATH/etc. values (appeared as empty strings there which makes no sense — a standard environment variable is deleted from the environment when assigned an empty string).

comment:5 Changed 6 years ago by dmik

The LIBCx part is done in https://github.com/bitwiseworks/libcx/commit/5b911092f01b27aa326c129c6560e8f58f3aafa9. I had to do a bit more than just that; see the commit log.

Note: See TracTickets for help on using tickets.