* [PATCH v3] linux-user: implement execveat
@ 2022-11-03 17:32 Drew DeVault
2022-11-03 19:54 ` Helge Deller
2022-11-04 9:53 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 7+ messages in thread
From: Drew DeVault @ 2022-11-03 17:32 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Drew DeVault, qemu-devel
References: https://gitlab.com/qemu-project/qemu/-/issues/1007
Signed-off-by: Drew DeVault <sir@cmpwn.com>
---
v2 => v3:
- Rebase to address the is_proc_myself fix
- Drop the ifdefs
linux-user/syscall.c | 203 ++++++++++++++++++++++---------------------
1 file changed, 105 insertions(+), 98 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 8402c1399d..38fbbbad6a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -689,7 +689,8 @@ safe_syscall4(pid_t, wait4, pid_t, pid, int *, status, int, options, \
#endif
safe_syscall5(int, waitid, idtype_t, idtype, id_t, id, siginfo_t *, infop, \
int, options, struct rusage *, rusage)
-safe_syscall3(int, execve, const char *, filename, char **, argv, char **, envp)
+safe_syscall5(int, execveat, int, dirfd, const char *, filename,
+ char **, argv, char **, envp, int, flags)
#if defined(TARGET_NR_select) || defined(TARGET_NR__newselect) || \
defined(TARGET_NR_pselect6) || defined(TARGET_NR_pselect6_time64)
safe_syscall6(int, pselect6, int, nfds, fd_set *, readfds, fd_set *, writefds, \
@@ -8349,6 +8350,106 @@ static int do_openat(CPUArchState *cpu_env, int dirfd, const char *pathname, int
return safe_openat(dirfd, path(pathname), flags, mode);
}
+static int do_execveat(CPUArchState *cpu_env, int dirfd, abi_long pathname, abi_long guest_argp, abi_long guest_envp, int flags)
+{
+ int ret;
+ char **argp, **envp;
+ int argc, envc;
+ abi_ulong gp;
+ abi_ulong addr;
+ char **q;
+ void *p;
+
+ argc = 0;
+
+ for (gp = guest_argp; gp; gp += sizeof(abi_ulong)) {
+ if (get_user_ual(addr, gp))
+ return -TARGET_EFAULT;
+ if (!addr)
+ break;
+ argc++;
+ }
+ envc = 0;
+ for (gp = guest_envp; gp; gp += sizeof(abi_ulong)) {
+ if (get_user_ual(addr, gp))
+ return -TARGET_EFAULT;
+ if (!addr)
+ break;
+ envc++;
+ }
+
+ argp = g_new0(char *, argc + 1);
+ envp = g_new0(char *, envc + 1);
+
+ for (gp = guest_argp, q = argp; gp;
+ gp += sizeof(abi_ulong), q++) {
+ if (get_user_ual(addr, gp))
+ goto execve_efault;
+ if (!addr)
+ break;
+ if (!(*q = lock_user_string(addr)))
+ goto execve_efault;
+ }
+ *q = NULL;
+
+ for (gp = guest_envp, q = envp; gp;
+ gp += sizeof(abi_ulong), q++) {
+ if (get_user_ual(addr, gp))
+ goto execve_efault;
+ if (!addr)
+ break;
+ if (!(*q = lock_user_string(addr)))
+ goto execve_efault;
+ }
+ *q = NULL;
+
+ /* Although execve() is not an interruptible syscall it is
+ * a special case where we must use the safe_syscall wrapper:
+ * if we allow a signal to happen before we make the host
+ * syscall then we will 'lose' it, because at the point of
+ * execve the process leaves QEMU's control. So we use the
+ * safe syscall wrapper to ensure that we either take the
+ * signal as a guest signal, or else it does not happen
+ * before the execve completes and makes it the other
+ * program's problem.
+ */
+ if (!(p = lock_user_string(pathname)))
+ goto execve_efault;
+
+ if (is_proc_myself(p, "exe")) {
+ ret = get_errno(safe_execveat(dirfd, exec_path, argp, envp, flags));
+ } else {
+ ret = get_errno(safe_execveat(dirfd, p, argp, envp, flags));
+ }
+
+ unlock_user(p, pathname, 0);
+
+ goto execve_end;
+
+execve_efault:
+ ret = -TARGET_EFAULT;
+
+execve_end:
+ for (gp = guest_argp, q = argp; *q;
+ gp += sizeof(abi_ulong), q++) {
+ if (get_user_ual(addr, gp)
+ || !addr)
+ break;
+ unlock_user(*q, addr, 0);
+ }
+ for (gp = guest_envp, q = envp; *q;
+ gp += sizeof(abi_ulong), q++) {
+ if (get_user_ual(addr, gp)
+ || !addr)
+ break;
+ unlock_user(*q, addr, 0);
+ }
+
+ g_free(argp);
+ g_free(envp);
+ return ret;
+}
+
#define TIMER_MAGIC 0x0caf0000
#define TIMER_MAGIC_MASK 0xffff0000
@@ -8846,104 +8947,10 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
unlock_user(p, arg2, 0);
return ret;
#endif
+ case TARGET_NR_execveat:
+ return do_execveat(cpu_env, arg1, arg2, arg3, arg4, arg5);
case TARGET_NR_execve:
- {
- char **argp, **envp;
- int argc, envc;
- abi_ulong gp;
- abi_ulong guest_argp;
- abi_ulong guest_envp;
- abi_ulong addr;
- char **q;
-
- argc = 0;
- guest_argp = arg2;
- for (gp = guest_argp; gp; gp += sizeof(abi_ulong)) {
- if (get_user_ual(addr, gp))
- return -TARGET_EFAULT;
- if (!addr)
- break;
- argc++;
- }
- envc = 0;
- guest_envp = arg3;
- for (gp = guest_envp; gp; gp += sizeof(abi_ulong)) {
- if (get_user_ual(addr, gp))
- return -TARGET_EFAULT;
- if (!addr)
- break;
- envc++;
- }
-
- argp = g_new0(char *, argc + 1);
- envp = g_new0(char *, envc + 1);
-
- for (gp = guest_argp, q = argp; gp;
- gp += sizeof(abi_ulong), q++) {
- if (get_user_ual(addr, gp))
- goto execve_efault;
- if (!addr)
- break;
- if (!(*q = lock_user_string(addr)))
- goto execve_efault;
- }
- *q = NULL;
-
- for (gp = guest_envp, q = envp; gp;
- gp += sizeof(abi_ulong), q++) {
- if (get_user_ual(addr, gp))
- goto execve_efault;
- if (!addr)
- break;
- if (!(*q = lock_user_string(addr)))
- goto execve_efault;
- }
- *q = NULL;
-
- if (!(p = lock_user_string(arg1)))
- goto execve_efault;
- /* Although execve() is not an interruptible syscall it is
- * a special case where we must use the safe_syscall wrapper:
- * if we allow a signal to happen before we make the host
- * syscall then we will 'lose' it, because at the point of
- * execve the process leaves QEMU's control. So we use the
- * safe syscall wrapper to ensure that we either take the
- * signal as a guest signal, or else it does not happen
- * before the execve completes and makes it the other
- * program's problem.
- */
- if (is_proc_myself(p, "exe")) {
- ret = get_errno(safe_execve(exec_path, argp, envp));
- } else {
- ret = get_errno(safe_execve(p, argp, envp));
- }
- unlock_user(p, arg1, 0);
-
- goto execve_end;
-
- execve_efault:
- ret = -TARGET_EFAULT;
-
- execve_end:
- for (gp = guest_argp, q = argp; *q;
- gp += sizeof(abi_ulong), q++) {
- if (get_user_ual(addr, gp)
- || !addr)
- break;
- unlock_user(*q, addr, 0);
- }
- for (gp = guest_envp, q = envp; *q;
- gp += sizeof(abi_ulong), q++) {
- if (get_user_ual(addr, gp)
- || !addr)
- break;
- unlock_user(*q, addr, 0);
- }
-
- g_free(argp);
- g_free(envp);
- }
- return ret;
+ return do_execveat(cpu_env, AT_FDCWD, arg1, arg2, arg3, 0);
case TARGET_NR_chdir:
if (!(p = lock_user_string(arg1)))
return -TARGET_EFAULT;
--
2.38.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] linux-user: implement execveat
2022-11-03 17:32 [PATCH v3] linux-user: implement execveat Drew DeVault
@ 2022-11-03 19:54 ` Helge Deller
2022-11-04 9:53 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 7+ messages in thread
From: Helge Deller @ 2022-11-03 19:54 UTC (permalink / raw)
To: Drew DeVault, Laurent Vivier; +Cc: qemu-devel
On 11/3/22 18:32, Drew DeVault wrote:
> References: https://gitlab.com/qemu-project/qemu/-/issues/1007
> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
> v2 => v3:
> - Rebase to address the is_proc_myself fix
> - Drop the ifdefs
>
> linux-user/syscall.c | 203 ++++++++++++++++++++++---------------------
> 1 file changed, 105 insertions(+), 98 deletions(-)
I'd suggest you add the missing strace part as well.
See in strace.list and compare what's done for execve.
Helge
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 8402c1399d..38fbbbad6a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -689,7 +689,8 @@ safe_syscall4(pid_t, wait4, pid_t, pid, int *, status, int, options, \
> #endif
> safe_syscall5(int, waitid, idtype_t, idtype, id_t, id, siginfo_t *, infop, \
> int, options, struct rusage *, rusage)
> -safe_syscall3(int, execve, const char *, filename, char **, argv, char **, envp)
> +safe_syscall5(int, execveat, int, dirfd, const char *, filename,
> + char **, argv, char **, envp, int, flags)
> #if defined(TARGET_NR_select) || defined(TARGET_NR__newselect) || \
> defined(TARGET_NR_pselect6) || defined(TARGET_NR_pselect6_time64)
> safe_syscall6(int, pselect6, int, nfds, fd_set *, readfds, fd_set *, writefds, \
> @@ -8349,6 +8350,106 @@ static int do_openat(CPUArchState *cpu_env, int dirfd, const char *pathname, int
> return safe_openat(dirfd, path(pathname), flags, mode);
> }
>
> +static int do_execveat(CPUArchState *cpu_env, int dirfd, abi_long pathname, abi_long guest_argp, abi_long guest_envp, int flags)
> +{
> + int ret;
> + char **argp, **envp;
> + int argc, envc;
> + abi_ulong gp;
> + abi_ulong addr;
> + char **q;
> + void *p;
> +
> + argc = 0;
> +
> + for (gp = guest_argp; gp; gp += sizeof(abi_ulong)) {
> + if (get_user_ual(addr, gp))
> + return -TARGET_EFAULT;
> + if (!addr)
> + break;
> + argc++;
> + }
> + envc = 0;
> + for (gp = guest_envp; gp; gp += sizeof(abi_ulong)) {
> + if (get_user_ual(addr, gp))
> + return -TARGET_EFAULT;
> + if (!addr)
> + break;
> + envc++;
> + }
> +
> + argp = g_new0(char *, argc + 1);
> + envp = g_new0(char *, envc + 1);
> +
> + for (gp = guest_argp, q = argp; gp;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp))
> + goto execve_efault;
> + if (!addr)
> + break;
> + if (!(*q = lock_user_string(addr)))
> + goto execve_efault;
> + }
> + *q = NULL;
> +
> + for (gp = guest_envp, q = envp; gp;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp))
> + goto execve_efault;
> + if (!addr)
> + break;
> + if (!(*q = lock_user_string(addr)))
> + goto execve_efault;
> + }
> + *q = NULL;
> +
> + /* Although execve() is not an interruptible syscall it is
> + * a special case where we must use the safe_syscall wrapper:
> + * if we allow a signal to happen before we make the host
> + * syscall then we will 'lose' it, because at the point of
> + * execve the process leaves QEMU's control. So we use the
> + * safe syscall wrapper to ensure that we either take the
> + * signal as a guest signal, or else it does not happen
> + * before the execve completes and makes it the other
> + * program's problem.
> + */
> + if (!(p = lock_user_string(pathname)))
> + goto execve_efault;
> +
> + if (is_proc_myself(p, "exe")) {
> + ret = get_errno(safe_execveat(dirfd, exec_path, argp, envp, flags));
> + } else {
> + ret = get_errno(safe_execveat(dirfd, p, argp, envp, flags));
> + }
> +
> + unlock_user(p, pathname, 0);
> +
> + goto execve_end;
> +
> +execve_efault:
> + ret = -TARGET_EFAULT;
> +
> +execve_end:
> + for (gp = guest_argp, q = argp; *q;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp)
> + || !addr)
> + break;
> + unlock_user(*q, addr, 0);
> + }
> + for (gp = guest_envp, q = envp; *q;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp)
> + || !addr)
> + break;
> + unlock_user(*q, addr, 0);
> + }
> +
> + g_free(argp);
> + g_free(envp);
> + return ret;
> +}
> +
> #define TIMER_MAGIC 0x0caf0000
> #define TIMER_MAGIC_MASK 0xffff0000
>
> @@ -8846,104 +8947,10 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
> unlock_user(p, arg2, 0);
> return ret;
> #endif
> + case TARGET_NR_execveat:
> + return do_execveat(cpu_env, arg1, arg2, arg3, arg4, arg5);
> case TARGET_NR_execve:
> - {
> - char **argp, **envp;
> - int argc, envc;
> - abi_ulong gp;
> - abi_ulong guest_argp;
> - abi_ulong guest_envp;
> - abi_ulong addr;
> - char **q;
> -
> - argc = 0;
> - guest_argp = arg2;
> - for (gp = guest_argp; gp; gp += sizeof(abi_ulong)) {
> - if (get_user_ual(addr, gp))
> - return -TARGET_EFAULT;
> - if (!addr)
> - break;
> - argc++;
> - }
> - envc = 0;
> - guest_envp = arg3;
> - for (gp = guest_envp; gp; gp += sizeof(abi_ulong)) {
> - if (get_user_ual(addr, gp))
> - return -TARGET_EFAULT;
> - if (!addr)
> - break;
> - envc++;
> - }
> -
> - argp = g_new0(char *, argc + 1);
> - envp = g_new0(char *, envc + 1);
> -
> - for (gp = guest_argp, q = argp; gp;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp))
> - goto execve_efault;
> - if (!addr)
> - break;
> - if (!(*q = lock_user_string(addr)))
> - goto execve_efault;
> - }
> - *q = NULL;
> -
> - for (gp = guest_envp, q = envp; gp;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp))
> - goto execve_efault;
> - if (!addr)
> - break;
> - if (!(*q = lock_user_string(addr)))
> - goto execve_efault;
> - }
> - *q = NULL;
> -
> - if (!(p = lock_user_string(arg1)))
> - goto execve_efault;
> - /* Although execve() is not an interruptible syscall it is
> - * a special case where we must use the safe_syscall wrapper:
> - * if we allow a signal to happen before we make the host
> - * syscall then we will 'lose' it, because at the point of
> - * execve the process leaves QEMU's control. So we use the
> - * safe syscall wrapper to ensure that we either take the
> - * signal as a guest signal, or else it does not happen
> - * before the execve completes and makes it the other
> - * program's problem.
> - */
> - if (is_proc_myself(p, "exe")) {
> - ret = get_errno(safe_execve(exec_path, argp, envp));
> - } else {
> - ret = get_errno(safe_execve(p, argp, envp));
> - }
> - unlock_user(p, arg1, 0);
> -
> - goto execve_end;
> -
> - execve_efault:
> - ret = -TARGET_EFAULT;
> -
> - execve_end:
> - for (gp = guest_argp, q = argp; *q;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp)
> - || !addr)
> - break;
> - unlock_user(*q, addr, 0);
> - }
> - for (gp = guest_envp, q = envp; *q;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp)
> - || !addr)
> - break;
> - unlock_user(*q, addr, 0);
> - }
> -
> - g_free(argp);
> - g_free(envp);
> - }
> - return ret;
> + return do_execveat(cpu_env, AT_FDCWD, arg1, arg2, arg3, 0);
> case TARGET_NR_chdir:
> if (!(p = lock_user_string(arg1)))
> return -TARGET_EFAULT;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] linux-user: implement execveat
2022-11-03 17:32 [PATCH v3] linux-user: implement execveat Drew DeVault
2022-11-03 19:54 ` Helge Deller
@ 2022-11-04 9:53 ` Philippe Mathieu-Daudé
2022-11-04 9:55 ` Drew DeVault
1 sibling, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-04 9:53 UTC (permalink / raw)
To: Drew DeVault, Laurent Vivier; +Cc: qemu-devel, Helge Deller
Hi Drew,
On 3/11/22 18:32, Drew DeVault wrote:
> References: https://gitlab.com/qemu-project/qemu/-/issues/1007
> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
> v2 => v3:
> - Rebase to address the is_proc_myself fix
> - Drop the ifdefs
>
> linux-user/syscall.c | 203 ++++++++++++++++++++++---------------------
> 1 file changed, 105 insertions(+), 98 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 8402c1399d..38fbbbad6a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -689,7 +689,8 @@ safe_syscall4(pid_t, wait4, pid_t, pid, int *, status, int, options, \
> #endif
> safe_syscall5(int, waitid, idtype_t, idtype, id_t, id, siginfo_t *, infop, \
> int, options, struct rusage *, rusage)
> -safe_syscall3(int, execve, const char *, filename, char **, argv, char **, envp)
> +safe_syscall5(int, execveat, int, dirfd, const char *, filename,
> + char **, argv, char **, envp, int, flags)
> #if defined(TARGET_NR_select) || defined(TARGET_NR__newselect) || \
> defined(TARGET_NR_pselect6) || defined(TARGET_NR_pselect6_time64)
> safe_syscall6(int, pselect6, int, nfds, fd_set *, readfds, fd_set *, writefds, \
> @@ -8349,6 +8350,106 @@ static int do_openat(CPUArchState *cpu_env, int dirfd, const char *pathname, int
> return safe_openat(dirfd, path(pathname), flags, mode);
> }
>
> +static int do_execveat(CPUArchState *cpu_env, int dirfd, abi_long pathname, abi_long guest_argp, abi_long guest_envp, int flags)
> +{
> + int ret;
> + char **argp, **envp;
> + int argc, envc;
> + abi_ulong gp;
> + abi_ulong addr;
> + char **q;
> + void *p;
> +
> + argc = 0;
> +
> + for (gp = guest_argp; gp; gp += sizeof(abi_ulong)) {
> + if (get_user_ual(addr, gp))
> + return -TARGET_EFAULT;
> + if (!addr)
> + break;
> + argc++;
> + }
> + envc = 0;
> + for (gp = guest_envp; gp; gp += sizeof(abi_ulong)) {
> + if (get_user_ual(addr, gp))
> + return -TARGET_EFAULT;
> + if (!addr)
> + break;
> + envc++;
> + }
> +
> + argp = g_new0(char *, argc + 1);
> + envp = g_new0(char *, envc + 1);
> +
> + for (gp = guest_argp, q = argp; gp;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp))
> + goto execve_efault;
> + if (!addr)
> + break;
> + if (!(*q = lock_user_string(addr)))
> + goto execve_efault;
> + }
> + *q = NULL;
> +
> + for (gp = guest_envp, q = envp; gp;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp))
> + goto execve_efault;
> + if (!addr)
> + break;
> + if (!(*q = lock_user_string(addr)))
> + goto execve_efault;
> + }
> + *q = NULL;
> +
> + /* Although execve() is not an interruptible syscall it is
> + * a special case where we must use the safe_syscall wrapper:
> + * if we allow a signal to happen before we make the host
> + * syscall then we will 'lose' it, because at the point of
> + * execve the process leaves QEMU's control. So we use the
> + * safe syscall wrapper to ensure that we either take the
> + * signal as a guest signal, or else it does not happen
> + * before the execve completes and makes it the other
> + * program's problem.
> + */
> + if (!(p = lock_user_string(pathname)))
> + goto execve_efault;
> +
> + if (is_proc_myself(p, "exe")) {
> + ret = get_errno(safe_execveat(dirfd, exec_path, argp, envp, flags));
> + } else {
> + ret = get_errno(safe_execveat(dirfd, p, argp, envp, flags));
> + }
> +
> + unlock_user(p, pathname, 0);
> +
> + goto execve_end;
> +
> +execve_efault:
> + ret = -TARGET_EFAULT;
> +
> +execve_end:
> + for (gp = guest_argp, q = argp; *q;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp)
> + || !addr)
> + break;
> + unlock_user(*q, addr, 0);
> + }
> + for (gp = guest_envp, q = envp; *q;
> + gp += sizeof(abi_ulong), q++) {
> + if (get_user_ual(addr, gp)
> + || !addr)
> + break;
> + unlock_user(*q, addr, 0);
> + }
> +
> + g_free(argp);
> + g_free(envp);
> + return ret;
> +}
> +
> #define TIMER_MAGIC 0x0caf0000
> #define TIMER_MAGIC_MASK 0xffff0000
>
> @@ -8846,104 +8947,10 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
> unlock_user(p, arg2, 0);
> return ret;
> #endif
> + case TARGET_NR_execveat:
> + return do_execveat(cpu_env, arg1, arg2, arg3, arg4, arg5);
> case TARGET_NR_execve:
> - {
> - char **argp, **envp;
> - int argc, envc;
> - abi_ulong gp;
> - abi_ulong guest_argp;
> - abi_ulong guest_envp;
> - abi_ulong addr;
> - char **q;
> -
> - argc = 0;
> - guest_argp = arg2;
> - for (gp = guest_argp; gp; gp += sizeof(abi_ulong)) {
> - if (get_user_ual(addr, gp))
> - return -TARGET_EFAULT;
> - if (!addr)
> - break;
> - argc++;
> - }
> - envc = 0;
> - guest_envp = arg3;
> - for (gp = guest_envp; gp; gp += sizeof(abi_ulong)) {
> - if (get_user_ual(addr, gp))
> - return -TARGET_EFAULT;
> - if (!addr)
> - break;
> - envc++;
> - }
> -
> - argp = g_new0(char *, argc + 1);
> - envp = g_new0(char *, envc + 1);
> -
> - for (gp = guest_argp, q = argp; gp;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp))
> - goto execve_efault;
> - if (!addr)
> - break;
> - if (!(*q = lock_user_string(addr)))
> - goto execve_efault;
> - }
> - *q = NULL;
> -
> - for (gp = guest_envp, q = envp; gp;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp))
> - goto execve_efault;
> - if (!addr)
> - break;
> - if (!(*q = lock_user_string(addr)))
> - goto execve_efault;
> - }
> - *q = NULL;
> -
> - if (!(p = lock_user_string(arg1)))
> - goto execve_efault;
> - /* Although execve() is not an interruptible syscall it is
> - * a special case where we must use the safe_syscall wrapper:
> - * if we allow a signal to happen before we make the host
> - * syscall then we will 'lose' it, because at the point of
> - * execve the process leaves QEMU's control. So we use the
> - * safe syscall wrapper to ensure that we either take the
> - * signal as a guest signal, or else it does not happen
> - * before the execve completes and makes it the other
> - * program's problem.
> - */
> - if (is_proc_myself(p, "exe")) {
> - ret = get_errno(safe_execve(exec_path, argp, envp));
> - } else {
> - ret = get_errno(safe_execve(p, argp, envp));
> - }
> - unlock_user(p, arg1, 0);
> -
> - goto execve_end;
> -
> - execve_efault:
> - ret = -TARGET_EFAULT;
> -
> - execve_end:
> - for (gp = guest_argp, q = argp; *q;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp)
> - || !addr)
> - break;
> - unlock_user(*q, addr, 0);
> - }
> - for (gp = guest_envp, q = envp; *q;
> - gp += sizeof(abi_ulong), q++) {
> - if (get_user_ual(addr, gp)
> - || !addr)
> - break;
> - unlock_user(*q, addr, 0);
> - }
> -
> - g_free(argp);
> - g_free(envp);
> - }
> - return ret;
> + return do_execveat(cpu_env, AT_FDCWD, arg1, arg2, arg3, 0);
> case TARGET_NR_chdir:
> if (!(p = lock_user_string(arg1)))
> return -TARGET_EFAULT;
Splitting this big patch would ease review:
1/ Replace safe_execve() by safe_execveat()
-safe_execve(exec_path, argp, envp))
+safe_execveat(AT_FDCWD, exec_path, argp, envp, 0));
2/ Extract do_execve()
3/ Convert do_execve() to do_execveat() adding dirfd/flags args
4/ Add TARGET_NR_execveat case
Thanks,
Phil.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] linux-user: implement execveat
2022-11-04 9:53 ` Philippe Mathieu-Daudé
@ 2022-11-04 9:55 ` Drew DeVault
2022-11-04 10:01 ` Philippe Mathieu-Daudé
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Drew DeVault @ 2022-11-04 9:55 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Laurent Vivier; +Cc: qemu-devel, Helge Deller
On Fri Nov 4, 2022 at 10:53 AM CET, Philippe Mathieu-Daudé wrote:
> Splitting this big patch would ease review:
It's only +165/-131, are you sure it really needs to be split?
> 1/ Replace safe_execve() by safe_execveat()
>
> -safe_execve(exec_path, argp, envp))
> +safe_execveat(AT_FDCWD, exec_path, argp, envp, 0));
>
> 2/ Extract do_execve()
>
> 3/ Convert do_execve() to do_execveat() adding dirfd/flags args
>
> 4/ Add TARGET_NR_execveat case
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] linux-user: implement execveat
2022-11-04 9:55 ` Drew DeVault
@ 2022-11-04 10:01 ` Philippe Mathieu-Daudé
2022-11-04 10:03 ` Laurent Vivier
2022-11-04 10:16 ` Daniel P. Berrangé
2 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-11-04 10:01 UTC (permalink / raw)
To: Drew DeVault, Laurent Vivier; +Cc: qemu-devel, Helge Deller
On 4/11/22 10:55, Drew DeVault wrote:
> On Fri Nov 4, 2022 at 10:53 AM CET, Philippe Mathieu-Daudé wrote:
>> Splitting this big patch would ease review:
>
> It's only +165/-131, are you sure it really needs to be split?
I tend to ignore patches over 120 lines of change :)
In this case it is about execve(), so critical enough to have me
spend the time to figure out your changes and decompose them.
Think about someone looking at your patch in months and not having
to do that decomposition again.
Anyway, it is up to the maintainer who will merge your patch ;)
>> 1/ Replace safe_execve() by safe_execveat()
>>
>> -safe_execve(exec_path, argp, envp))
>> +safe_execveat(AT_FDCWD, exec_path, argp, envp, 0));
>>
>> 2/ Extract do_execve()
>>
>> 3/ Convert do_execve() to do_execveat() adding dirfd/flags args
>>
>> 4/ Add TARGET_NR_execveat case
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] linux-user: implement execveat
2022-11-04 9:55 ` Drew DeVault
2022-11-04 10:01 ` Philippe Mathieu-Daudé
@ 2022-11-04 10:03 ` Laurent Vivier
2022-11-04 10:16 ` Daniel P. Berrangé
2 siblings, 0 replies; 7+ messages in thread
From: Laurent Vivier @ 2022-11-04 10:03 UTC (permalink / raw)
To: Drew DeVault, Philippe Mathieu-Daudé; +Cc: qemu-devel, Helge Deller
Le 04/11/2022 à 10:55, Drew DeVault a écrit :
> On Fri Nov 4, 2022 at 10:53 AM CET, Philippe Mathieu-Daudé wrote:
>> Splitting this big patch would ease review:
>
> It's only +165/-131, are you sure it really needs to be split?
>
>> 1/ Replace safe_execve() by safe_execveat()
>>
>> -safe_execve(exec_path, argp, envp))
>> +safe_execveat(AT_FDCWD, exec_path, argp, envp, 0));
>>
>> 2/ Extract do_execve()
>>
>> 3/ Convert do_execve() to do_execveat() adding dirfd/flags args
>>
>> 4/ Add TARGET_NR_execveat case
>
I think it's not needed, it's mainly a code move and it's easy to compare.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] linux-user: implement execveat
2022-11-04 9:55 ` Drew DeVault
2022-11-04 10:01 ` Philippe Mathieu-Daudé
2022-11-04 10:03 ` Laurent Vivier
@ 2022-11-04 10:16 ` Daniel P. Berrangé
2 siblings, 0 replies; 7+ messages in thread
From: Daniel P. Berrangé @ 2022-11-04 10:16 UTC (permalink / raw)
To: Drew DeVault
Cc: Philippe Mathieu-Daudé, Laurent Vivier, qemu-devel,
Helge Deller
On Fri, Nov 04, 2022 at 10:55:39AM +0100, Drew DeVault wrote:
> On Fri Nov 4, 2022 at 10:53 AM CET, Philippe Mathieu-Daudé wrote:
> > Splitting this big patch would ease review:
>
> It's only +165/-131, are you sure it really needs to be split?
IMHO it is a standard best practice that code movement always be
done in a separate commit from bug fixes / new features, regardless
of # lines of code affected. I've seen way too many patches where
bugs have been hidden due code movement / whitespace changes mixed
in, even when the patch was small.
> > 1/ Replace safe_execve() by safe_execveat()
> >
> > -safe_execve(exec_path, argp, envp))
> > +safe_execveat(AT_FDCWD, exec_path, argp, envp, 0));
> >
> > 2/ Extract do_execve()
> >
> > 3/ Convert do_execve() to do_execveat() adding dirfd/flags args
> >
> > 4/ Add TARGET_NR_execveat case
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-04 10:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-03 17:32 [PATCH v3] linux-user: implement execveat Drew DeVault
2022-11-03 19:54 ` Helge Deller
2022-11-04 9:53 ` Philippe Mathieu-Daudé
2022-11-04 9:55 ` Drew DeVault
2022-11-04 10:01 ` Philippe Mathieu-Daudé
2022-11-04 10:03 ` Laurent Vivier
2022-11-04 10:16 ` Daniel P. Berrangé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).