Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,11 @@ env:
CARGO_ARGS_NO_SSL: --no-default-features --features stdlib,importlib,stdio,encodings,sqlite
# Skip additional tests on Windows. They are checked on Linux and MacOS.
# test_glob: many failing tests
# test_io: many failing tests
# test_os: many failing tests
# test_pathlib: panic by surrogate chars
# test_posixpath: OSError: (22, 'The filename, directory name, or volume label syntax is incorrect. (os error 123)')
# test_venv: couple of failing tests
WINDOWS_SKIPS: >-
test_glob
test_io
test_os
test_rlcompleter
test_pathlib
test_posixpath
Expand Down
16 changes: 0 additions & 16 deletions Lib/test/test_ntpath.py
Original file line number Diff line number Diff line change
Expand Up @@ -990,8 +990,6 @@ def test_realpath_permission(self):

self.assertPathEqual(test_file, ntpath.realpath(test_file_short))

# TODO: RUSTPYTHON
@unittest.expectedFailureIfWindows("TODO: RUSTPYTHON; ValueError: illegal environment variable name")
def test_expandvars(self):
with os_helper.EnvironmentVarGuard() as env:
env.clear()
Expand All @@ -1018,8 +1016,6 @@ def test_expandvars(self):
tester('ntpath.expandvars("\'%foo%\'%bar")', "\'%foo%\'%bar")
tester('ntpath.expandvars("bar\'%foo%")', "bar\'%foo%")

# TODO: RUSTPYTHON
@unittest.expectedFailureIfWindows("TODO: RUSTPYTHON; ValueError: illegal environment variable name")
@unittest.skipUnless(os_helper.FS_NONASCII, 'need os_helper.FS_NONASCII')
def test_expandvars_nonascii(self):
def check(value, expected):
Expand All @@ -1040,8 +1036,6 @@ def check(value, expected):
check('%spam%bar', '%sbar' % nonascii)
check('%{}%bar'.format(nonascii), 'ham%sbar' % nonascii)

# TODO: RUSTPYTHON
@unittest.expectedFailureIfWindows("TODO: RUSTPYTHON")
def test_expanduser(self):
tester('ntpath.expanduser("test")', 'test')

Expand Down Expand Up @@ -1515,16 +1509,6 @@ class NtCommonTest(test_genericpath.CommonTest, unittest.TestCase):
pathmodule = ntpath
attributes = ['relpath']

# TODO: RUSTPYTHON
@unittest.expectedFailureIfWindows("TODO: RUSTPYTHON; ValueError: illegal environment variable name")
def test_expandvars(self):
return super().test_expandvars()

# TODO: RUSTPYTHON
@unittest.expectedFailureIfWindows("TODO: RUSTPYTHON; ValueError: illegal environment variable name")
def test_expandvars_nonascii(self):
return super().test_expandvars_nonascii()


class PathLikeTests(NtpathTestCase):

Expand Down
2 changes: 0 additions & 2 deletions Lib/test/test_unittest/testmock/testpatch.py
Original file line number Diff line number Diff line change
Expand Up @@ -658,8 +658,6 @@ def test():
self.assertEqual(foo, {})


# TODO: RUSTPYTHON
@unittest.expectedFailureIfWindows("TODO: RUSTPYTHON")
def test_patch_dict_with_string(self):
@patch.dict('os.environ', {'konrad_delong': 'some value'})
def test():
Expand Down
17 changes: 13 additions & 4 deletions crates/vm/src/stdlib/nt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,13 @@ pub(crate) mod module {
let environ = vm.ctx.new_dict();

for (key, value) in env::vars() {
// Skip hidden Windows environment variables (e.g., =C:, =D:, =ExitCode)
// These are internal cmd.exe bookkeeping variables that store per-drive
// current directories. They cannot be modified via _wputenv() and should
// not be exposed to Python code.
if key.starts_with('=') {
continue;
}
environ.set_item(&key, vm.new_pyobj(value), vm).unwrap();
}
environ
Expand Down Expand Up @@ -364,8 +371,9 @@ pub(crate) mod module {
if key_str.contains('\0') || value_str.contains('\0') {
return Err(vm.new_value_error("embedded null character"));
}
// Validate: no '=' in key
if key_str.contains('=') {
// Validate: no '=' in key (search from index 1 because on Windows
// starting '=' is allowed for defining hidden environment variables)
if key_str.get(1..).is_some_and(|s| s.contains('=')) {
return Err(vm.new_value_error("illegal environment variable name"));
Comment on lines +374 to 377
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical issue: key consisting of only "=" is incorrectly accepted.

The validation at line 369 has the same flaw as the corresponding code in os.rs. When key_str = "=", the check key_str.get(1..).is_some_and(|s| s.contains('=')) returns false because the substring after index 1 is empty and does not contain '='.

A key consisting solely of "=" is not a valid environment variable name and should be explicitly rejected.

Apply this diff to fix the validation:

             // Validate: no '=' in key (search from index 1 because on Windows
             // starting '=' is allowed for defining hidden environment variables)
-            if key_str.get(1..).is_some_and(|s| s.contains('=')) {
+            if key_str == "=" || key_str.get(1..).is_some_and(|s| s.contains('=')) {
                 return Err(vm.new_value_error("illegal environment variable name"));
             }
🤖 Prompt for AI Agents
In crates/vm/src/stdlib/nt.rs around lines 367 to 370, the current validation
misses the case where key_str == "=" so a single "=" is incorrectly accepted;
update the validation to explicitly reject a key that is exactly "=" (return the
same vm.new_value_error) before the existing substring check (or alter the
condition to treat an empty substring after index 1 as invalid), ensuring any
key consisting solely of "=" is treated as illegal.

}

Expand Down Expand Up @@ -480,8 +488,9 @@ pub(crate) mod module {
if key_str.contains('\0') || value_str.contains('\0') {
return Err(vm.new_value_error("embedded null character"));
}
// Validate: no '=' in key
if key_str.contains('=') {
// Validate: no '=' in key (search from index 1 because on Windows
// starting '=' is allowed for defining hidden environment variables)
if key_str.get(1..).is_some_and(|s| s.contains('=')) {
return Err(vm.new_value_error("illegal environment variable name"));
Comment on lines +491 to 494
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical issue: key consisting of only "=" is incorrectly accepted.

The validation at line 486 has the same flaw as spawnve above. A key of "=" will incorrectly pass validation because key_str.get(1..).is_some_and(|s| s.contains('=')) returns false when the substring after index 1 is empty.

Apply this diff to fix the validation:

             // Validate: no '=' in key (search from index 1 because on Windows
             // starting '=' is allowed for defining hidden environment variables)
-            if key_str.get(1..).is_some_and(|s| s.contains('=')) {
+            if key_str == "=" || key_str.get(1..).is_some_and(|s| s.contains('=')) {
                 return Err(vm.new_value_error("illegal environment variable name"));
             }
🤖 Prompt for AI Agents
crates/vm/src/stdlib/nt.rs around lines 484 to 487: the current check allows a
key of "=" because get(1..) is None and the condition is false; change the
validation to also reject a lone "=" by updating the conditional to fail when
key_str == "=" or when key_str.get(1..).is_some_and(|s| s.contains('=')).
Replace the existing if condition with a compound check that returns Err for
either case.

}

Expand Down
106 changes: 81 additions & 25 deletions crates/vm/src/stdlib/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
convert::{IntoPyException, ToPyException, ToPyObject},
function::{ArgumentError, FromArgs, FuncArgs},
};
use std::{ffi, fs, io, path::Path};
use std::{fs, io, path::Path};

pub(crate) fn fs_metadata<P: AsRef<Path>>(
path: P,
Expand Down Expand Up @@ -112,7 +112,8 @@ pub(super) struct FollowSymlinks(
#[pyarg(named, name = "follow_symlinks", default = true)] pub bool,
);

fn bytes_as_os_str<'a>(b: &'a [u8], vm: &VirtualMachine) -> PyResult<&'a ffi::OsStr> {
#[cfg(not(windows))]
fn bytes_as_os_str<'a>(b: &'a [u8], vm: &VirtualMachine) -> PyResult<&'a std::ffi::OsStr> {
rustpython_common::os::bytes_as_os_str(b)
.map_err(|_| vm.new_unicode_decode_error("can't decode path for utf-8"))
}
Expand Down Expand Up @@ -160,7 +161,7 @@ pub(super) mod _os {
suppress_iph,
},
convert::{IntoPyException, ToPyObject},
function::{ArgBytesLike, Either, FsPath, FuncArgs, OptionalArg},
function::{ArgBytesLike, FsPath, FuncArgs, OptionalArg},
ospath::{IOErrorBuilder, OsPath, OsPathOrFd, OutputMode},
protocol::PyIterReturn,
recursion::ReprGuard,
Expand All @@ -171,7 +172,7 @@ pub(super) mod _os {
use crossbeam_utils::atomic::AtomicCell;
use itertools::Itertools;
use std::{
env, ffi, fs,
env, fs,
fs::OpenOptions,
io,
path::PathBuf,
Expand Down Expand Up @@ -403,7 +404,7 @@ pub(super) mod _os {
b"." | b".." => None,
_ => Some(
OutputMode::String
.process_path(ffi::OsStr::from_bytes(fname), vm),
.process_path(std::ffi::OsStr::from_bytes(fname), vm),
),
}
})
Expand All @@ -415,31 +416,62 @@ pub(super) mod _os {
Ok(list)
}

fn env_bytes_as_bytes(obj: &Either<PyStrRef, PyBytesRef>) -> &[u8] {
#[cfg(not(windows))]
fn env_bytes_as_bytes(obj: &crate::function::Either<PyStrRef, PyBytesRef>) -> &[u8] {
match obj {
Either::A(s) => s.as_bytes(),
Either::B(b) => b.as_bytes(),
crate::function::Either::A(s) => s.as_bytes(),
crate::function::Either::B(b) => b.as_bytes(),
}
}

/// Check if environment variable length exceeds Windows limit.
/// size should be key.len() + value.len() + 2 (for '=' and null terminator)
#[cfg(windows)]
fn check_env_var_len(size: usize, vm: &VirtualMachine) -> PyResult<()> {
unsafe extern "C" {
fn _wputenv(envstring: *const u16) -> libc::c_int;
}

/// Check if wide string length exceeds Windows environment variable limit.
#[cfg(windows)]
fn check_env_var_len(wide_len: usize, vm: &VirtualMachine) -> PyResult<()> {
use crate::common::windows::_MAX_ENV;
if size > _MAX_ENV {
if wide_len > _MAX_ENV + 1 {
return Err(vm.new_value_error(format!(
"the environment variable is longer than {} characters",
_MAX_ENV
"the environment variable is longer than {_MAX_ENV} characters",
)));
}
Ok(())
}

#[cfg(windows)]
#[pyfunction]
fn putenv(key: PyStrRef, value: PyStrRef, vm: &VirtualMachine) -> PyResult<()> {
let key_str = key.as_str();
let value_str = value.as_str();
// Search from index 1 because on Windows starting '=' is allowed for
// defining hidden environment variables.
if key_str.is_empty()
|| key_str.get(1..).is_some_and(|s| s.contains('='))
|| key_str.contains('\0')
|| value_str.contains('\0')
{
return Err(vm.new_value_error("illegal environment variable name"));
}
let env_str = format!("{}={}", key_str, value_str);
let wide = env_str.to_wide_with_nul();
check_env_var_len(wide.len(), vm)?;

// Use _wputenv like CPython (not SetEnvironmentVariableW) to update CRT environ
let result = unsafe { suppress_iph!(_wputenv(wide.as_ptr())) };
if result != 0 {
return Err(vm.new_last_errno_error());
}
Ok(())
}
Comment on lines +444 to +468
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Search for test cases related to putenv
rg -n "putenv|os.putenv" --type=py crates/

Repository: RustPython/RustPython

Length of output: 186


🏁 Script executed:

# Check if there are any test files for the os module
fd -e py -p test | rg -i "test.*os|os.*test" | head -20

Repository: RustPython/RustPython

Length of output: 186


🏁 Script executed:

# Look at the git context to see if there are comments or commits related to this
rg -n "hidden.*environment\|environment.*variable.*name" --type=rs

Repository: RustPython/RustPython

Length of output: 186


🌐 Web query:

CPython putenv Windows environment variable validation

💡 Result:

Short answer: CPython validates environment-variable names for os.putenv / os.environ on Windows — names may not contain '=' except possibly as the first character (Windows "hidden" variables like "=C:..."); CPython rejects names with '=' elsewhere and keeps behavior in sync with the C runtime/Windows APIs. See Python issue tracker discussions and the related CPython changes. [1][2][3]

Sources

  • Reject '=' in env var names (issue discussion & PRs). [1]
  • Windows parsing/leading '=' (hidden) behavior and tests. [2]
  • Changes to os.putenv/os.unsetenv implementation and Windows notes (CRT vs SetEnvironmentVariable). [3]

🌐 Web query:

Windows environment variable naming rules "=" character

💡 Result:

Short answer: Windows environment variable names cannot contain the equals sign ("="). Environment strings are stored as "Name=Value\0" so the '=' is the separator and is not allowed in the name. [1][2]

References:

  • Microsoft: Environment Variables (Win32) — "The name of an environment variable cannot include an equal sign (=)." [1]
  • POSIX / Open Group: environment strings have the form name=value and "names shall not contain the character '='." (useful for general background). [2]

🌐 Web query:

CPython source code putenv Windows implementation validation

💡 Result:

Short answer: CPython validates Windows putenv names and values in Modules/posixmodule.c (win32_putenv). On Windows it:

  • rejects empty names and any name containing '=' at or after index 1 (leading '=' is allowed for hidden vars).
  • builds a "NAME=VALUE" wide string and uses PyUnicode_AsWideCharString (which rejects embedded NULs).
  • checks the combined length against _MAX_ENV and raises ValueError if too long.
  • calls the CRT _wputenv (so CRT updates _wenviron/_environ).

Sources: CPython win32_putenv implementation in Modules/posixmodule.c and the related tracker/commit discussion. [1][2]

References

  • win32_putenv implementation (Modules/posixmodule.c). Shows the '=' check, NUL rejection via PyUnicode_AsWideCharString, length check against _MAX_ENV, and use of _wputenv. [1]
  • bpo-39406 discussion / commits about using CRT putenv on Windows (context why CPython prefers _wputenv). [2]

[1] Modules/posixmodule.c — win32_putenv implementation.
[2] bpo-39406 / CPython commit discussion (os.putenv() behavior on Windows).


Fix edge case: key consisting of only "=" should be rejected

The validation logic allows a key consisting of just "=", which is invalid. The check key_str.get(1..).is_some_and(|s| s.contains('=')) returns false for this case because the substring after index 1 is empty.

Per Windows environment variable rules and CPython's implementation, "=" alone is not a valid environment variable name (it is the separator character in "NAME=VALUE" format). While the code correctly allows leading "=" for Windows hidden variables (e.g., "=C:"), it should reject a key that is only "=".

Add an additional check: || key_str == "=" to explicitly reject this edge case.

🤖 Prompt for AI Agents
In crates/vm/src/stdlib/os.rs around lines 446 to 476, the current validation
for Windows environment variable keys mistakenly allows a key equal to "=";
update the existing conditional that rejects empty, null-containing, or
'='-containing keys to also explicitly reject the single-character key "="
(e.g., add a check like key_str == "=" to the OR chain) so that a lone "=" is
treated as an invalid environment variable name.


#[cfg(not(windows))]
#[pyfunction]
fn putenv(
key: Either<PyStrRef, PyBytesRef>,
value: Either<PyStrRef, PyBytesRef>,
key: crate::function::Either<PyStrRef, PyBytesRef>,
value: crate::function::Either<PyStrRef, PyBytesRef>,
vm: &VirtualMachine,
) -> PyResult<()> {
let key = env_bytes_as_bytes(&key);
Expand All @@ -450,17 +482,44 @@ pub(super) mod _os {
if key.is_empty() || key.contains(&b'=') {
return Err(vm.new_value_error("illegal environment variable name"));
}
#[cfg(windows)]
check_env_var_len(key.len() + value.len() + 2, vm)?;
let key = super::bytes_as_os_str(key, vm)?;
let value = super::bytes_as_os_str(value, vm)?;
// SAFETY: requirements forwarded from the caller
unsafe { env::set_var(key, value) };
Ok(())
}

#[cfg(windows)]
#[pyfunction]
fn unsetenv(key: PyStrRef, vm: &VirtualMachine) -> PyResult<()> {
let key_str = key.as_str();
// Search from index 1 because on Windows starting '=' is allowed for
// defining hidden environment variables.
if key_str.is_empty()
|| key_str.get(1..).is_some_and(|s| s.contains('='))
|| key_str.contains('\0')
{
return Err(vm.new_value_error("illegal environment variable name"));
}
// "key=" to unset (empty value removes the variable)
let env_str = format!("{}=", key_str);
let wide = env_str.to_wide_with_nul();
check_env_var_len(wide.len(), vm)?;

// Use _wputenv like CPython (not SetEnvironmentVariableW) to update CRT environ
let result = unsafe { suppress_iph!(_wputenv(wide.as_ptr())) };
if result != 0 {
return Err(vm.new_last_errno_error());
}
Ok(())
}
Comment on lines +492 to +515
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical issue: key consisting of only "=" is incorrectly accepted.

The validation at lines 498-502 has the same flaw as putenv above. When key_str = "=", the check key_str.get(1..).is_some_and(|s| s.contains('=')) returns false because the substring after index 1 is empty.

A key of just "=" is not a valid environment variable name and should be rejected.

Apply this diff to fix the validation:

         if key_str.is_empty()
             || key_str.get(1..).is_some_and(|s| s.contains('='))
+            || key_str == "="
             || key_str.contains('\0')
         {
🤖 Prompt for AI Agents
In crates/vm/src/stdlib/os.rs around lines 492 to 515, the validation allows a
key consisting only of "=" because checking key_str.get(1..) misses the case
where the first character is '='; update the guard to also reject keys that
start with '=' (e.g., key_str.starts_with('=') or key_str.chars().next() ==
Some('=')) so that a lone "=" or any name beginning with '=' is treated as
invalid, while keeping the existing empty and NUL checks and the subsequent
logic unchanged.


#[cfg(not(windows))]
#[pyfunction]
fn unsetenv(key: Either<PyStrRef, PyBytesRef>, vm: &VirtualMachine) -> PyResult<()> {
fn unsetenv(
key: crate::function::Either<PyStrRef, PyBytesRef>,
vm: &VirtualMachine,
) -> PyResult<()> {
let key = env_bytes_as_bytes(&key);
if key.contains(&b'\0') {
return Err(vm.new_value_error("embedded null byte"));
Expand All @@ -474,9 +533,6 @@ pub(super) mod _os {
),
));
}
// For unsetenv, size is key + '=' (no value, just clearing)
#[cfg(windows)]
check_env_var_len(key.len() + 1, vm)?;
let key = super::bytes_as_os_str(key, vm)?;
// SAFETY: requirements forwarded from the caller
unsafe { env::remove_var(key) };
Expand Down Expand Up @@ -984,7 +1040,7 @@ pub(super) mod _os {
OsPathOrFd::Path(path) => {
use rustpython_common::os::ffi::OsStrExt;
let path = path.as_ref().as_os_str().as_bytes();
let path = match ffi::CString::new(path) {
let path = match std::ffi::CString::new(path) {
Ok(x) => x,
Err(_) => return Ok(None),
};
Expand Down Expand Up @@ -1483,7 +1539,7 @@ pub(super) mod _os {

#[pyfunction]
fn strerror(e: i32) -> String {
unsafe { ffi::CStr::from_ptr(libc::strerror(e)) }
unsafe { std::ffi::CStr::from_ptr(libc::strerror(e)) }
.to_string_lossy()
.into_owned()
}
Expand Down Expand Up @@ -1587,7 +1643,7 @@ pub(super) mod _os {
if encoding.is_null() || encoding.read() == '\0' as libc::c_char {
"UTF-8".to_owned()
} else {
ffi::CStr::from_ptr(encoding).to_string_lossy().into_owned()
std::ffi::CStr::from_ptr(encoding).to_string_lossy().into_owned()
}
};

Expand Down
Loading