| Message ID | 20220426092715.3931705-13-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc cleanups | expand |
On 4/26/22 02:27, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > qga/commands-posix.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 77f4672ca2c9..094487c2c395 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -2529,7 +2529,7 @@ void qmp_guest_set_user_password(const char *username, > goto out; > } > > - if (pipe(datafd) < 0) { > + if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) { > error_setg(errp, "cannot create pipe FDs"); > goto out; > } This looks wrong, since the next thing that happens is fork+execl. r~
Hi On Wed, Apr 27, 2022 at 5:08 AM Richard Henderson < richard.henderson@linaro.org> wrote: > On 4/26/22 02:27, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > qga/commands-posix.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > > index 77f4672ca2c9..094487c2c395 100644 > > --- a/qga/commands-posix.c > > +++ b/qga/commands-posix.c > > @@ -2529,7 +2529,7 @@ void qmp_guest_set_user_password(const char > *username, > > goto out; > > } > > > > - if (pipe(datafd) < 0) { > > + if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) { > > error_setg(errp, "cannot create pipe FDs"); > > goto out; > > } > > This looks wrong, since the next thing that happens is fork+execl. > > Before exec(), it does close(datafd[1]); dup2(datafd[0], 0); 0, the newfd, does not share file descriptor flags (the close-on-exec flag). I did a quick test, and it seems to be fine.
On Wed, Apr 27, 2022 at 12:24:44PM +0400, Marc-André Lureau wrote: > Hi > > On Wed, Apr 27, 2022 at 5:08 AM Richard Henderson < > richard.henderson@linaro.org> wrote: > > > On 4/26/22 02:27, marcandre.lureau@redhat.com wrote: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > --- > > > qga/commands-posix.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > > > index 77f4672ca2c9..094487c2c395 100644 > > > --- a/qga/commands-posix.c > > > +++ b/qga/commands-posix.c > > > @@ -2529,7 +2529,7 @@ void qmp_guest_set_user_password(const char > > *username, > > > goto out; > > > } > > > > > > - if (pipe(datafd) < 0) { > > > + if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) { > > > error_setg(errp, "cannot create pipe FDs"); > > > goto out; > > > } > > > > This looks wrong, since the next thing that happens is fork+execl. > > > > > Before exec(), it does > close(datafd[1]); > dup2(datafd[0], 0); > > 0, the newfd, does not share file descriptor flags (the close-on-exec flag). > > I did a quick test, and it seems to be fine. The 'dup' man page says The two file descriptors do not share file descriptor flags (the close-on-exec flag). The close-on-exec flag (FD_CLOEXEC; see fcntl(2)) for the duplicate descriptor is off. so we're fine in this respect. You could need to use dup3 to explicitly turn on FD_CLOEXEC on the duplicate, so Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 77f4672ca2c9..094487c2c395 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -2529,7 +2529,7 @@ void qmp_guest_set_user_password(const char *username, goto out; } - if (pipe(datafd) < 0) { + if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) { error_setg(errp, "cannot create pipe FDs"); goto out; }