Skip to content

test_os, test_io on windows #6379

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.

Sign up for GitHub

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

Merged
youknowone merged 3 commits into from
Dec 11, 2025
Merged

test_os, test_io on windows #6379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ca6053
Allow hidden env vars on nt
youknowone Dec 9, 2025
069d439
Enable test_os on windows
youknowone Dec 9, 2025
df899ea
enable test_io on windows
youknowone Dec 10, 2025
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Contributor

@coderabbitai coderabbitai bot Dec 10, 2025

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
Contributor

@coderabbitai coderabbitai bot Dec 10, 2025

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
View file
Open in desktop
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(())
}

#[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
Contributor

@coderabbitai coderabbitai bot Dec 10, 2025

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