From dd12f892f1b857e5161abd33a2967125b265d550 Mon Sep 17 00:00:00 2001
From: johnl-oai <johnl@openai.com>
Date: Wed, 7 Oct 2026 20:34:56 +0000
Subject: [PATCH] Avoid sharing violations when repairing Windows runtime ACLs
 (#51822)

## Why

Opening active EXE/DLL files with `MAXIMUM_ALLOWED` can request access that causes sharing violations during sandbox runtime ACL repair.

## What changed

Restrict `MAXIMUM_ALLOWED` opens to directories that need to suppress child ACL propagation. For files, request `READ_CONTROL | WRITE_DAC` only when an ACL update is needed.

Preserve the non-inheriting grant behavior from https://github.com/openai/codex/pull/49690, including existing permissions, deny ACEs, and null DACLs.

## Testing

Extend the Windows runtime repair regression test to hold `node.exe` open with `FILE_SHARE_READ` throughout repair and revalidation, while checking read/execute access and preserved denials.

GitOrigin-RevId: 2e087b51adc0221d0eda061fa0a34d2e98409662
---
 codex-rs/windows-sandbox-rs/src/acl.rs          | 17 ++++++++++++-----
 .../setup_runtime_bin_tests.rs                  |  9 +++++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs
index 0ef5e44ffc..0b4bba3149 100644
--- a/codex-rs/windows-sandbox-rs/src/acl.rs
+++ b/codex-rs/windows-sandbox-rs/src/acl.rs
@@ -546,7 +546,14 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl(
     access_mode: ACCESS_MODE,
     inheritance: u32,
 ) -> Result<bool> {
-    let directory = if inheritance == 0 {
+    // Only directories need MAXIMUM_ALLOWED to suppress child ACL propagation.
+    // On active EXE/DLL files, it can request access that causes sharing violations.
+    let directory = if inheritance == 0
+        && path
+            .metadata()
+            .with_context(|| format!("inspect ACL target {}", path.display()))?
+            .is_dir()
+    {
         Some(
             OpenOptions::new()
                 .access_mode(MAXIMUM_ALLOWED)
@@ -559,7 +566,7 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl(
         None
     };
     // A root-only grant must preserve existing permissions and deny ACEs.
-    let access_mode = if directory.is_some() {
+    let access_mode = if inheritance == 0 {
         GRANT_ACCESS
     } else {
         access_mode
@@ -610,7 +617,7 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl(
             continue;
         }
         if !dacl_allow_mask_needs_refresh(p_dacl, *sid, allow_mask, disallow_mask)
-            || directory.is_some() && p_dacl.is_null()
+            || inheritance == 0 && p_dacl.is_null()
         {
             continue;
         }
@@ -643,8 +650,8 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl(
         if code2 != ERROR_SUCCESS {
             return Err(anyhow!("SetEntriesInAclW failed: {code2}"));
         }
-        // For inheriting grants, request ACL-write access only when an update
-        // is needed. Use the same long-path support as the read handle.
+        // For files and inheriting directory grants, request ACL-write access only
+        // when an update is needed. Use the same long-path support as the read handle.
         let handle = match directory {
             // MAXIMUM_ALLOWED suppresses propagation of existing inheritable ACEs too.
             Some(directory) => directory,
diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin_tests.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin_tests.rs
index 010d75abc9..ac5a4edefe 100644
--- a/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin_tests.rs
+++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning/setup_runtime_bin_tests.rs
@@ -7,6 +7,7 @@ use crate::ensure_allow_mask_aces_with_inheritance;
 use crate::path_mask_allows;
 use pretty_assertions::assert_eq;
 use std::fs;
+use std::os::windows::fs::OpenOptionsExt;
 use std::path::PathBuf;
 use windows_sys::Win32::Foundation::HLOCAL;
 use windows_sys::Win32::Foundation::LocalFree;
@@ -20,6 +21,7 @@ use windows_sys::Win32::Storage::FileSystem::DELETE;
 use windows_sys::Win32::Storage::FileSystem::FILE_APPEND_DATA;
 use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_EXECUTE;
 use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ;
+use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ;
 use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA;
 use windows_sys::Win32::Storage::FileSystem::WRITE_DAC;
 
@@ -166,6 +168,13 @@ fn repairs_children_when_runtime_root_already_has_read_execute_access() {
             .expect("deny runtime writes")
     );
 
+    // Keep data-write and delete opens blocked throughout repair and revalidation.
+    let _reader = fs::OpenOptions::new()
+        .read(true)
+        .share_mode(FILE_SHARE_READ)
+        .open(&node)
+        .expect("hold runtime file open for reading");
+
     for _ in 0..2 {
         ensure_runtime_tree_readable(runtime.path(), sandbox_sid.as_ptr())
             .expect("repair runtimes");