[libvirt] [PATCH 2/4] virNetSocket: Be more safe with fork() around virNetSocketDupFD()

[John Ferlan]

On 9/21/18 5:29 AM, Michal Privoznik wrote:
> If there was a caller which would dup the client FD without
> CLOEXEC flag and later decided to change the flag it wouldn't be
> safe to do because fork() might have had occurred meantime.

blank line

> Switch to the other pattern - always dup FD with the flag set and
> let callers clear the flag if they need to do so.

I don't see this last sentence as happening in this patch...

> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/libxl/libxl_migration.c |  4 ++--
>  src/qemu/qemu_migration.c   |  2 +-
>  src/rpc/virnetclient.c      | 10 +++++++++-
>  src/rpc/virnetsocket.c      |  7 ++-----
>  src/rpc/virnetsocket.h      |  2 +-
>  5 files changed, 15 insertions(+), 10 deletions(-)
> 

I'm with Marc on this - documenting virNetSocketDupFD is a good thing.
Although it could be considered out of the scope of what you're doing,
since you are changing the semantics describing the nuance that fork
brings about w/r/t fd duplication in the code rather than the commit
message doesn't make someone have to go back and look at the commit
message to gain more insight.  It also should provide the reader with
the knowledge of what virSetInherit would provide to remove if so desired.

Of course while you're at it, documenting virNetClientDupFD similarly
would be quite beneficial.

> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index fc7ccb53d0..5eb8eb1203 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -310,7 +310,7 @@ libxlMigrateDstReceive(virNetSocketPtr sock,
>      }
>      VIR_DEBUG("Accepted migration connection."
>                "  Spawning thread to process migration data");
> -    recvfd = virNetSocketDupFD(client_sock, true);
> +    recvfd = virNetSocketDupFD(client_sock);
>      virObjectUnref(client_sock);
>  
>      /*
> @@ -1254,7 +1254,7 @@ libxlDomainMigrationSrcPerform(libxlDriverPrivatePtr driver,
>          goto cleanup;
>      }
>  
> -    sockfd = virNetSocketDupFD(sock, true);
> +    sockfd = virNetSocketDupFD(sock);
>      virObjectUnref(sock);
>  
>      if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 0)
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 825a9d399b..129be0a11a 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3303,7 +3303,7 @@ qemuMigrationSrcConnect(virQEMUDriverPtr driver,
>      if (virNetSocketNewConnectTCP(host, port,
>                                    AF_UNSPEC,
>                                    &sock) == 0) {
> -        spec->dest.fd.qemu = virNetSocketDupFD(sock, true);
> +        spec->dest.fd.qemu = virNetSocketDupFD(sock);
>          virObjectUnref(sock);
>      }
>      if (qemuSecurityClearSocketLabel(driver->securityManager, vm->def) < 0 ||
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index b4d8fb2187..40ed3fde6d 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -715,8 +715,16 @@ int virNetClientGetFD(virNetClientPtr client)
>  int virNetClientDupFD(virNetClientPtr client, bool cloexec)
>  {
>      int fd;
> +
>      virObjectLock(client);
> -    fd = virNetSocketDupFD(client->sock, cloexec);
> +
> +    fd = virNetSocketDupFD(client->sock);
> +    if (!cloexec && fd >= 0 && virSetInherit(fd, true)) {
> +        virReportSystemError(errno, "%s",
> +                             _("Cannot disable close-on-exec flag"));
> +        VIR_FORCE_CLOSE(fd);
> +    }
> +

Caveat: my sock knowledge of setting or duplication of the socket fd
isn't that deep, especially as it relates to fork "mechanics".

So prior to this change, the call to virNetSocketDupFD(fd, false) would:

        fd = dup(sock->fd);

Or essentially create a new socket fd from the existing one copying
everything it already has...

With this change you have (discounting some new error paths)

        fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, 0);
        fcntl(fd, F_SETFD, ~FD_CLOEXEC)

which seems to be a nop and/or unnecessary to me.

Beyond that if the sock->fd didn't have CLOEXEC already, then what
impact does briefly changing CLOEXEC have on something that arrives
while set?

And of course if sock->fd had CLOEXEC and then you clear it on the
'shared' socket, that doesn't seem right either.

Seems to me the dup() does the trick and then lets the caller decide
what CLOEXEC semantics they want with the duplicated fd without perhaps
changing the sock->fd semantics.

>      virObjectUnlock(client);
>      return fd;
>  }
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index 55de3b2aad..27ffa23bcd 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -1372,14 +1372,11 @@ int virNetSocketGetFD(virNetSocketPtr sock)
>  }
>  
>  
> -int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec)
> +int virNetSocketDupFD(virNetSocketPtr sock)
>  {
>      int fd;
>  
> -    if (cloexec)
> -        fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, 0);
> -    else
> -        fd = dup(sock->fd);
> +    fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, 0);

So removing this "instance" of fork and client usage - is there no
belief that some day it may be a desirable option.

I think the end result you have in patch3 do the SetInherit call is
probably still proper since at that point you know by calling w/ false
all you'd be getting is a straight duplicate of what exists and the
decision at that point is to "alter" the view of the socket you have
received to change the CLOEXEC value

John

hopefully I'm not too far off into the weeds on this, but I'm not
convinced this patch is right even though I do think documenting the
semantics would be a really good thing to happen because I cannot
imagine the nuance(s) is(are) all that well understood.

>      if (fd < 0) {
>          virReportSystemError(errno, "%s",
>                               _("Unable to copy socket file handle"));
> diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h
> index de795af917..e6bac27566 100644
> --- a/src/rpc/virnetsocket.h
> +++ b/src/rpc/virnetsocket.h
> @@ -124,7 +124,7 @@ virNetSocketPtr virNetSocketNewPostExecRestart(virJSONValuePtr object);
>  virJSONValuePtr virNetSocketPreExecRestart(virNetSocketPtr sock);
>  
>  int virNetSocketGetFD(virNetSocketPtr sock);
> -int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec);
> +int virNetSocketDupFD(virNetSocketPtr sock);
>  
>  bool virNetSocketIsLocal(virNetSocketPtr sock);
>  
>