[sys/linux] Fix fork and execve syscalls on arm64 #3476
Conversation
core/sys/linux/sys.odin
Outdated
| @@ -2803,8 +2816,8 @@ getrandom :: proc "contextless" (buf: []u8, flags: Get_Random_Flags) -> (int, Er | |||
| Execute program relative to a directory file descriptor. | |||
| Available since Linux 3.19. | |||
| */ | |||
| execveat :: proc "contextless" (dirfd: Fd, name: cstring, argv: [^]cstring, envp: [^]cstring) -> (Errno) { | |||
| ret := syscall(SYS_execveat, dirfd, cast(rawptr) name, cast(rawptr) argv, cast(rawptr) envp) | |||
| execveat :: proc "contextless" (dirfd: Fd, name: cstring, argv: [^]cstring, envp: [^]cstring, flags: i32) -> (Errno) { | |||
There was a problem hiding this comment.
Since you're adding flags could you make this value a bitset instead of an i32? It would help people interfacing with this syscall.
core/sys/linux/sys.odin
Outdated
| clone :: proc "contextless" (flags: u64, stack: rawptr, parent_tid, child_tid: ^i32, tls: u64) -> (i64, Errno) { | ||
| when ODIN_ARCH == .amd64 { | ||
| ret := syscall(SYS_clone, flags, stack, parent_tid, child_tid, tls) | ||
| return errno_unwrap(ret, i64) |
There was a problem hiding this comment.
I'll explain why I didn't want to make clone syscall part of the API. If one calls this function, providing a stack parameter that is not NULL, then this line will crash the child process, because child's stack doesn't have the value for RIP on the stack.
Usually this syscall is used for creating threads, and for threads the ideal API is a callback. The way one would set this up is by calling the clone syscall directly, and if we found ourselves as the child process, we jump to (not call) an assembly routine that sets up the stack, copies the TLS image and does a few things, before calling the thread callback with needed parameters.
But calling a callback, let alone having a separate assembly routine is too much of an API change, which goes against the way this package is supposed to work. That's why I didn't want to provide clone as part of this sys/linux API.
Even if you manage to create a thread you need to know the exact TLS layout for the given libc the program is compiled with. And the TLS is defined by the dynamic linker, which makes things so much harder because there's no API's that let you get the TLS image / struct from a dynamic linker and call it done. Having a different TLS layout can lead to bugs, from what I've heard nvidia drivers on linux expect glibc's TLS layout and don't work with e.g. musl.
So I think this procedure needs to be removed and syscall called directly in the fork() procedure.
core/sys/linux/sys.odin
Outdated
| return errno_unwrap(ret, Pid) | ||
| ret, err := clone(u64(Signal.SIGCHLD), nil, nil, nil, 0) | ||
| if err != .NONE do return Pid(ret), err | ||
| return Pid(ret), err |
There was a problem hiding this comment.
This could be simplified...
There was a problem hiding this comment.
I suppose this is not relevant anymore?
core/sys/linux/sys.odin
Outdated
| @@ -769,8 +783,7 @@ execve :: proc "contextless" (name: cstring, argv: [^]cstring, envp: [^]cstring) | |||
| ret := syscall(SYS_execve, cast(rawptr) name, cast(rawptr) argv, cast(rawptr) envp) | |||
| return Errno(-ret) | |||
| } else { | |||
| ret := syscall(SYS_execveat, AT_FDCWD, cast(rawptr) name, cast(rawptr) argv, cast(rawptr) envp) | |||
| return Errno(-ret) | |||
| return execveat(AT_FDCWD, name, argv, envp, 0) | |||
There was a problem hiding this comment.
It's a small thing so I was wondering as to whether I want to make this remark, but I don't want to call procedures that call syscalls as opposed to calling syscalls directly, because they might have additional logic that maybe you don't want to duplicate. Let's keep this package as plain as possible
|
Is there a particular reason as to why the |
I believe this one may have been assumed =] SYSCALL_DEFINE3(open, const char __user *, filename, int, flags, umode_t, mode)
{
if (force_o_largefile())
flags |= O_LARGEFILE;
return do_sys_open(AT_FDCWD, filename, flags, mode);
}
SYSCALL_DEFINE4(openat, int, dfd, const char __user *, filename, int, flags,
umode_t, mode)
{
if (force_o_largefile())
flags |= O_LARGEFILE;
return do_sys_open(dfd, filename, flags, mode);
} |
|
@flysand7 I made the requested changes. Is there still something that needs to be changed? |
|
@gingerBill looks good to merge |
This PR does the following things:
Fixes #3472
Fixes #3487
The syscalls have been implemented based on the ChromiumOS docs