gh-105436： the environment block should end with two zero-valued wchar_t values

[sku2000]

In the CreateProcessW document, the environment block should end with two zero-valued wchar_t values（i.e. four zero bytes）.

• Issue: subprocess.run(..., env={}) broken on Windows #105436

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[ghost]

All commit authors signed the Contributor License Agreement.

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[eryksun]

Using goto error is misleading. I suggest changing the function's exit label name to "cleanup".

Using PyMem_Calloc() avoids the need to manually zero the buffer for the null characters. Also, make sure to raise a MemoryError exception if allocation fails.

Suggested change

	/* A Unicode environment block is terminated by four zero bytes:
	two for the last string, two more to terminate the block. */
	if (envsize == 0) {
	buffer = PyMem_NEW(wchar_t, 2);
	p = buffer;
	*p++ = L'\0';
	*p++ = L'\0';
	goto error;
	}
	if (envsize == 0) {
	// A environment block must be terminated by two null characters --
	// one for the last string and one for the block.
	buffer = PyMem_Calloc(2, sizeof(wchar_t));
	if (!buffer) {
	PyErr_NoMemory();
	}
	goto cleanup;
	}

[sku2000]

thanks

[sku2000]

I can't reproduce the problem in the test.
This question may be related to this [issue].(#104791)

[zooba]

I guess the new test has exposed a leak on Ubuntu that no other tests trigger.

Maybe make the new test Windows only and tag it with this issue? No reason to block your change here - it's definitely not the cause of that leak.

[miss-islington]

Thanks @sku2000 for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

[bedevere-bot]

GH-105700 is a backport of this pull request to the 3.12 branch.

[bedevere-bot]

GH-105701 is a backport of this pull request to the 3.11 branch.

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM64 Windows 3.x has failed when building commit 4f7d3b6.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/729/builds/4732) and take a look at the build logs.
4. Check if the failure is related to this commit (4f7d3b6) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/729/builds/4732

Failed tests:

• test_subprocess

Failed subtests:

• test_run_with_an_empty_env - test.test_subprocess.RunFuncTestCase.test_run_with_an_empty_env

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

413 tests OK.

10 slowest tests:

• test_math: 6 min 59 sec
• test_multiprocessing_spawn: 4 min 32 sec
• test_peg_generator: 4 min 10 sec
• test_unparse: 2 min 25 sec
• test_tokenize: 2 min 23 sec
• test_tarfile: 2 min 6 sec
• test_capi: 2 min 2 sec
• test_unicodedata: 1 min 59 sec
• test_concurrent_futures: 1 min 47 sec
• test_fstring: 1 min 39 sec

1 test failed:
test_subprocess

33 tests skipped:
test.test_asyncio.test_unix_events test_curses test_dbm_gnu
test_dbm_ndbm test_devpoll test_epoll test_fcntl test_fork1
test_gdb test_grp test_ioctl test_kqueue test_multiprocessing_fork
test_multiprocessing_forkserver test_openpty test_perf_profiler
test_perfmaps test_poll test_posix test_pty test_pwd test_readline
test_resource test_syslog test_threadsignals test_tkinter test_ttk
test_wait3 test_wait4 test_xxlimited test_xxtestfuzz
test_zipfile64 test_zoneinfo

1 re-run test:
test_subprocess

Total duration: 19 min 51 sec

Click to see traceback logs

Traceback (most recent call last):
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\test_subprocess.py", line 1700, in test_run_with_an_empty_env
    self.assertEqual(res.returncode, 57)
AssertionError: 3221225781 != 57

[eryksun]

The child process failing at startup with STATUS_DLL_NOT_FOUND (0xC0000135, 3221225781) is probably due to a DLL init routine trying to load another DLL from a path that's based on a standard environment variable, such as SystemRoot. There's really no need to test the exit status. Just assert that OSError isn't raised.

[zooba]

Are we copying python.exe for this test but not moving python312.dll? That would be causing everything to fail if so.

[eryksun]

There's already a test_empty_env() test, which is skipped on Windows because the SystemRoot environment variable is required to start Python prior to 3.11. For example, here's 3.10 failing to start:

>>> sys.version_info[:2]
(3, 10)
>>> subprocess.call(sys.executable, env={'a':'b'})
Fatal Python error: _Py_HashRandomization_Init: failed to get random numbers to initialize Python
Python runtime state: preinitialized

The SystemRoot environment variable is no longer required to start Python 3.11+, at least not on x86/x64, because the internal win32_urandom() function was updated to call BCryptGenRandom() instead of CryptAcquireContextW() / CryptGenRandom().

The bot that's failing is an ARM64 system. I don't know whether process initialization under ARM64 depends on SystemRoot or some other environment variable in order to load a library that's particular to the API implementation on ARM64. What I do know is that for this Windows-only test, it doesn't matter whether the child process can start and exit with some know exit status. All we need to check is that the CreateProcessW() call doesn't fail, meaning that in principle it's possible to spawn a child process with an empty environment.

[zooba]

The ARM64 bot has a slightly different setup compared to the rest of our machines, because it's doing a cross-compile and then running elsewhere. So there might be something critical in PATH. It's unfortunate the Windows logging for DLL errors is so terrible, otherwise we might have a hint.

But you're right, the only important part is the OSError. I've reopened the bug, let's move discussion there.