Skip to content
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.

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

feat: re-enable std in uv-trampoline #4722

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

samypr100
Copy link
Contributor

@samypr100 samypr100 commented Jul 2, 2024

Summary

Partially closes #1917

This PR picks up on some of the great work from #1864 and opted to keep panic_immediate_abort (for size reasons). I split the PR in different isolated commits in case we want to separate/cherry-pick them out.

  1. The first commit ports mostly all std changes from that PR into this PR. Binary sizes stayed the same ~16kb.
  2. The second commit migrates our existing usage of windows-sys to windows for a safer ffi calls with Results!. It also changes all large unsafe blocks to be isolated to the actual unsafe calls, and switches some areas to use std such as getenv port ( which seemed buggy! ) from launcher.c. In addition, this also adds more error checking in order to match some missing assertions from distlib's launcher.c. Note, due to the additional .text data, the binary sizes increased to ~20.5kb, but we can cut back on some of the added error msgs as needed.
  3. The third commit switches to using xwin for building on all 3 supported trampoline targets for sanity, and adds a CI bloat check for core::fmt and panic as a precaution. Sadly, this will invalidate the xwin cache on the first run.

Test Plan

Most changes were tested on a couple of local GUI apps and console apps, also tested some of the error states manually by using SetLastError at different points in the code and/or passing in invalid handles.

I'm not sure how far we can get with migrating some of the other calls without increasing binary size substantially. An initial attempt at using std::path didn't seem so bad size wise when I tried it (~1k). On other cases, such as std::process::exit added ~10k to the total binary size.

@samypr100
Copy link
Contributor Author

samypr100 commented Jul 3, 2024

Below are some extra logging I added that is not part of distlib's launcher.c which we could arguably remove, although all these errors (if they occur) can diminish the stability of the console/gui script.

Patch File Diff (in case we want to remove the added logging)

@@ -400,9 +400,7 @@
         print_last_error_and_exit("Failed to spawn the python child process.");
     });
     let child_process_info = unsafe { child_process_info.assume_init() };
-    unsafe { CloseHandle(child_process_info.hThread) }.unwrap_or_else(|_| {
-        print_last_error_and_exit("Failed to close handle to python child process main thread.");
-    });
+    let _ = unsafe { CloseHandle(child_process_info.hThread) };
     // Return handle to child process.
     child_process_info.hProcess
 }
@@ -415,12 +413,8 @@
     // See distlib/PC/launcher.c::cleanup_standard_io()
     for std_handle in [STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, STD_ERROR_HANDLE] {
         if let Ok(handle) = unsafe { GetStdHandle(std_handle) } {
-            unsafe { CloseHandle(handle) }.unwrap_or_else(|_| {
-                eprintln!("Failed to close standard device handle {}.", handle.0);
-            });
-            unsafe { SetStdHandle(std_handle, INVALID_HANDLE_VALUE) }.unwrap_or_else(|_| {
-                eprintln!("Failed to modify standard device handle {}.", std_handle.0);
-            });
+            let _ = unsafe { CloseHandle(handle) };
+            let _ = unsafe { SetStdHandle(std_handle, INVALID_HANDLE_VALUE) };
         }
     }
 
@@ -434,9 +428,7 @@
     for i in 0..handle_count {
         let handle_ptr = unsafe { handle_start.offset(i).read_unaligned() } as *const HANDLE;
         // Close all fds inherited from the parent, except for the standard I/O fds.
-        unsafe { CloseHandle(*handle_ptr) }.unwrap_or_else(|_| {
-            eprintln!("Failed to close child file descriptors at {}.", i);
-        });
+        let _ = unsafe { CloseHandle(*handle_ptr) };
     }
 }
 
@@ -463,16 +455,10 @@
     let mut msg = MaybeUninit::<MSG>::uninit();
     unsafe {
         // End the launcher's "app starting" cursor state.
-        PostMessageA(None, 0, None, None).unwrap_or_else(|_| {
-            eprintln!("Failed to post a message to specified window.");
-        });
-        if GetMessageA(msg.as_mut_ptr(), None, 0, 0) != TRUE {
-            eprintln!("Failed to retrieve posted window message.");
-        }
+        let _ = PostMessageA(None, 0, None, None);
+        let _ = GetMessageA(msg.as_mut_ptr(), None, 0, 0);
         // Proxy the child's input idle event.
-        if WaitForInputIdle(child_handle, INFINITE) != 0 {
-            eprintln!("Failed to wait for input from window.");
-        }
+        let _ = WaitForInputIdle(child_handle, INFINITE);
         // Signal the process input idle event by creating a window and pumping
         // sent messages. The window class isn't important, so just use the
         // system "STATIC" class.
@@ -491,10 +477,8 @@
             None,
         );
         // Process all sent messages and signal input idle.
-        _ = PeekMessageA(msg.as_mut_ptr(), hwnd, 0, 0, PEEK_MESSAGE_REMOVE_TYPE(0));
-        DestroyWindow(hwnd).unwrap_or_else(|_| {
-            print_last_error_and_exit("Failed to destroy temporary window.");
-        });
+        let _ = PeekMessageA(msg.as_mut_ptr(), hwnd, 0, 0, PEEK_MESSAGE_REMOVE_TYPE(0));
+        let _ = DestroyWindow(hwnd);
     }
 }
 
@@ -517,9 +501,7 @@
 
     // (best effort) Switch to some innocuous directory, so we don't hold the original cwd open.
     // See distlib/PC/launcher.c::switch_working_directory
-    if std::env::set_current_dir(std::env::temp_dir()).is_err() {
-        eprintln!("Failed to set cwd to temp dir.");
-    }
+    let _ = std::env::set_current_dir(std::env::temp_dir());
 
     // We want to ignore control-C/control-Break/logout/etc.; the same event will
     // be delivered to the child, so we let them decide whether to exit or not.
@@ -535,7 +517,7 @@
         clear_app_starting_state(child_handle);
     }
 
-    _ = unsafe { WaitForSingleObject(child_handle, INFINITE) };
+    let _ = unsafe { WaitForSingleObject(child_handle, INFINITE) };
     let mut exit_code = 0u32;
     if unsafe { GetExitCodeProcess(child_handle, &mut exit_code) }.is_err() {
         print_last_error_and_exit("Failed to get exit code of child process.");

if let Some(tmp) = getenv(c"TEMP") {
SetCurrentDirectoryA(tmp.as_ptr() as *const _);
} else {
SetCurrentDirectoryA(c"c:\\".as_ptr() as *const _);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, the c:\\ approach was removed as it's not part of launcher.c, plus the sysroot drive could vary on a system.

getenv(c"TEMP") was changed to use std instead as it had bugs when parsing the value of TEMP on my system probably due to W vs A calls but didn't dig too much, as a result getenv was deleted.

crates/uv-trampoline/Cargo.toml Outdated Show resolved Hide resolved
@konstin
Copy link
Member

konstin commented Jul 3, 2024

Leaving in draft for now to see if we want to invest more time on this.

This looks good to merge to me

@konstin
Copy link
Member

konstin commented Jul 3, 2024

I didn't commit the binaries, do we manually commit those on the PR?

A member of the team will make a follow-up where the build the binaries and commit them

@samypr100
Copy link
Contributor Author

samypr100 commented Jul 3, 2024

A member of the team will make a follow-up where the build the binaries and commit them

Sounds good, I ended up commiting them to ensure tests pass, but from a security angle it makes sense someone from Astral builds them.

@samypr100 samypr100 marked this pull request as ready for review July 3, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use std for uv-trampoline
2 participants