update execve #113

robinyuan1002 · 2025-02-21T00:46:39Z

Some modify with Wasmtime and Rawposix to support execve. So for right now we don't need to change cage anymore, we do exactly the same behavior with linux.

Yaxuan-w

Great! Minor suggestions and we can merge dup3 pr after this one

Yaxuan-w · 2025-02-21T00:47:47Z

src/RawPOSIX/src/safeposix/syscalls/sys_calls.rs

@@ -133,17 +133,8 @@ impl Cage {
    /*
    *   exec() will only return if error happens 


Add comments about what we're doing here and man page link

also add how's our exec in rawposix is different from linux and why are we doing in this way

Yaxuan-w · 2025-02-21T00:48:08Z

src/RawPOSIX/src/safeposix/syscalls/sys_calls.rs

+            let mainsigsetatomic = self
+                .sigset
+                .get(
+                    &self
+                        .main_threadid
+                        .load(interface::RustAtomicOrdering::Relaxed),
+                )
+                .unwrap();
+            let mainsigset = interface::RustAtomicU64::new(
+                mainsigsetatomic.load(interface::RustAtomicOrdering::Relaxed),
+            );
+            newsigset.insert(0, mainsigset);


should comment out currently

Yaxuan-w · 2025-02-21T00:50:25Z

src/wasmtime/crates/lind-multi-process/src/lib.rs

@@ -785,8 +785,20 @@ impl<T: Clone + Send + 'static + std::marker::Sync, U: Clone + Send + 'static +
            // to-do: exec should not change the process id/cage id, however, the exec call from rustposix takes an
            // argument to change the process id. If we pass the same cageid, it would cause some error
            // lind_exec(cloned_pid as u64, cloned_pid as u64);


should delete this comment

qianxichen233 · 2025-02-21T01:24:50Z

src/RawPOSIX/src/safeposix/syscalls/sys_calls.rs

+        interface::cagetable_remove(self.cageid);

-        interface::cagetable_insert(child_cageid, newcage);
+        interface::cagetable_insert(self.cageid, newcage);


why choose delete the old cage and create a new one instead of modifying on the existing one

@Yaxuan-w and I discussed this previously. I believe she mentioned some data types were difficult to modify. But I think those issues may not exist anymore? I agree it would be better to modify the existing struct if we can.

she mentioned some data types were difficult to modify.

These're not issues for this version rawposix because all things are in same library.

The issue would raise when splitting type conversion and constants as external libs, since this would cause circular imports

Quote reply
Reference in new issue

I believe both way works. Must we modify on the existing one?

My preference would be to modify the existing struct rather than create a new one. Makes more sense logically as well as in terms of reducing overhead and complexity.

qianxichen233 · 2025-02-21T01:26:36Z

src/RawPOSIX/src/safeposix/syscalls/sys_calls.rs

            zombies: interface::RustLock::new(cloned_zombies), // when a process exec-ed, its child relationship should be perserved
-            child_num: interface::RustAtomicU64::new(child_num),
+            child_num: interface::RustAtomicU64::new(0),


child relationship should be perserved, so child_num should be kept

qianxichen233 · 2025-02-21T01:28:34Z

src/RawPOSIX/src/safeposix/syscalls/sys_calls.rs

@@ -133,17 +133,8 @@ impl Cage {
    /*
    *   exec() will only return if error happens 


also add how's our exec in rawposix is different from linux and why are we doing in this way

qianxichen233 · 2025-02-21T01:30:03Z

src/RawPOSIX/src/safeposix/syscalls/sys_calls.rs

            zombies: interface::RustLock::new(cloned_zombies), // when a process exec-ed, its child relationship should be perserved
-            child_num: interface::RustAtomicU64::new(child_num),
+            child_num: interface::RustAtomicU64::new(0),
+            vmmap: interface::RustLock::new(Vmmap::new()), // memory is cleared after exec


the real memory address should also be cleaned up not just vmmap. Probably need to add an interface in mem.rs to do memory cleanup

rennergade · 2025-02-21T01:36:15Z

Some modify with Wasmtime and Rawposix to support execve. So for right now we don't need to change cage anymore, we do exactly the same behavior with linux.

This isn't really a great description for the PR, ideally you want to list the changes you made and why

JustinCappos · 2025-02-21T01:43:28Z

At a minimum, the rationale for this needs to be thoroughly documented. So, anyone who reads the code should know exactly why we did it this way from reading the comments, when we are done.

…

On Thu, Feb 20, 2025 at 8:41 PM Nicholas Renner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/RawPOSIX/src/safeposix/syscalls/sys_calls.rs <#113 (comment)> : > + interface::cagetable_remove(self.cageid); - interface::cagetable_insert(child_cageid, newcage); + interface::cagetable_insert(self.cageid, newcage); @Yaxuan-w <https://github.com/Yaxuan-w> and I discussed this previously. I believe she mentioned some data types were difficult to modify. But I think those issues may not exist anymore? I agree it would be better to modify the existing struct if we can. — Reply to this email directly, view it on GitHub <#113 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRODZYJPCG7TEV7WMIWLT2QZ73FAVCNFSM6AAAAABXSB7ABKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMZRGYZDGOJYGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

qianxichen233 · 2025-02-21T20:38:28Z

src/glibc/sysdeps/unix/sysv/linux/i386/execve.c

@@ -5,3 +5,4 @@ int __execve (const char *__path, char *const __argv[], char *const __envp[])
 {
  return MAKE_SYSCALL(69, "syscall|execve", __path, __argv, __envp, NOTUSED, NOTUSED, NOTUSED);
 }
+weak_alias (__execve, execve)


let's make this strong alias rather than weak alias

JustinCappos · 2025-02-21T23:21:54Z

Is there ever a reason we need to use weak_alias? my understanding is that this isn't very portable.

…

On Fri, Feb 21, 2025 at 3:38 PM Qianxi Chen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/glibc/sysdeps/unix/sysv/linux/i386/execve.c <#113 (comment)> : > @@ -5,3 +5,4 @@ int __execve (const char *__path, char *const __argv[], char *const __envp[]) { return MAKE_SYSCALL(69, "syscall|execve", __path, __argv, __envp, NOTUSED, NOTUSED, NOTUSED); } +weak_alias (__execve, execve) let's make this strong alias rather than weak alias — Reply to this email directly, view it on GitHub <#113 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRODY2FU747C3AABJ3TCT2Q6FFTAVCNFSM6AAAAABXSB7ABKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMZUGE3TCNBZHA> . You are receiving this because you commented.Message ID: ***@***.***>

robinyuan1002 · 2025-02-22T00:14:47Z

Is there ever a reason we need to use weak_alias? my understanding is that this isn't very portable.
…
On Fri, Feb 21, 2025 at 3:38 PM Qianxi Chen @.> wrote: @.* commented on this pull request. ------------------------------ In src/glibc/sysdeps/unix/sysv/linux/i386/execve.c <#113 (comment)> : > @@ -5,3 +5,4 @@ int __execve (const char __path, char const __argv[], char const __envp[]) { return MAKE_SYSCALL(69, "syscall|execve", __path, __argv, __envp, NOTUSED, NOTUSED, NOTUSED); } +weak_alias (__execve, execve) let's make this strong alias rather than weak alias — Reply to this email directly, view it on GitHub <#113 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGRODY2FU747C3AABJ3TCT2Q6FFTAVCNFSM6AAAAABXSB7ABKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMZUGE3TCNBZHA . You are receiving this because you commented.Message ID: @.>

I believe in this case both work, but strong alias is the best way

update execve

1fb4677

robinyuan1002 requested review from rennergade, yzhang71, Yaxuan-w and qianxichen233 February 21, 2025 00:46

Yaxuan-w requested changes Feb 21, 2025

View reviewed changes

qianxichen233 requested changes Feb 21, 2025

View reviewed changes

qianxichen233 reviewed Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update execve #113

update execve #113

robinyuan1002 commented Feb 21, 2025

Yaxuan-w left a comment

Yaxuan-w Feb 21, 2025

qianxichen233 Feb 21, 2025

Yaxuan-w Feb 21, 2025

Yaxuan-w Feb 21, 2025

qianxichen233 Feb 21, 2025

rennergade Feb 21, 2025

Yaxuan-w Feb 21, 2025

robinyuan1002 Feb 22, 2025

rennergade Feb 22, 2025

qianxichen233 Feb 21, 2025

qianxichen233 Feb 21, 2025

qianxichen233 Feb 21, 2025

rennergade commented Feb 21, 2025

JustinCappos commented Feb 21, 2025 via email

qianxichen233 Feb 21, 2025

JustinCappos commented Feb 21, 2025 via email

robinyuan1002 commented Feb 22, 2025

		@@ -133,17 +133,8 @@ impl Cage {
		/*
		* exec() will only return if error happens

update execve #113

Are you sure you want to change the base?

update execve #113

Conversation

robinyuan1002 commented Feb 21, 2025

Yaxuan-w left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rennergade commented Feb 21, 2025

JustinCappos commented Feb 21, 2025 via email

Choose a reason for hiding this comment

JustinCappos commented Feb 21, 2025 via email

robinyuan1002 commented Feb 22, 2025