There is one caller (virSecurityManagerMetadataLock) which
duplicates the connection FD and wants to have the flag set.
However, trying to set the flag after dup() is not safe as
another thread might fork() meanwhile. Therefore, switch to
duplicating with the flag set and only let callers refine this
later.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/locking/domain_lock.c       | 18 ++++++++++++++++++
 src/locking/lock_driver_lockd.c |  2 +-
 src/rpc/virnetclient.c          |  9 +--------
 src/rpc/virnetclient.h          |  2 +-
 4 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
index 705b132457..db20fa86a3 100644
--- a/src/locking/domain_lock.c
+++ b/src/locking/domain_lock.c
@@ -188,6 +188,24 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin,
     ret = virLockManagerAcquire(lock, NULL, flags,
                                 dom->def->onLockFailure, fd);
 
+    if (ret >= 0 && fd && *fd >= 0 && virSetInherit(*fd, true) < 0) {
+        int saved_errno = errno;
+        virErrorPtr origerr;
+
+        virErrorPreserveLast(&origerr);
+
+        if (virLockManagerRelease(lock, NULL, 0) < 0)
+            VIR_WARN("Unable to release acquired resourced in cleanup path");
+
+        virErrorRestore(&origerr);
+        errno = saved_errno;
+
+        virReportSystemError(errno, "%s",
+                             _("Cannot disable close-on-exec flag"));
+
+        ret = -1;
+    }
+
     virLockManagerFree(lock);
 
     return ret;
diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
index 0c672b05b1..9b1943daa6 100644
--- a/src/locking/lock_driver_lockd.c
+++ b/src/locking/lock_driver_lockd.c
@@ -796,7 +796,7 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
         goto cleanup;
 
     if (fd &&
-        (*fd = virNetClientDupFD(client, false)) < 0)
+        (*fd = virNetClientDupFD(client)) < 0)
         goto cleanup;
 
     if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) {
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 40ed3fde6d..6b0ddbeaad 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -712,19 +712,12 @@ int virNetClientGetFD(virNetClientPtr client)
 }
 
 
-int virNetClientDupFD(virNetClientPtr client, bool cloexec)
+int virNetClientDupFD(virNetClientPtr client)
 {
     int fd;
 
     virObjectLock(client);
-
     fd = virNetSocketDupFD(client->sock);
-    if (!cloexec && fd >= 0 && virSetInherit(fd, true)) {
-        virReportSystemError(errno, "%s",
-                             _("Cannot disable close-on-exec flag"));
-        VIR_FORCE_CLOSE(fd);
-    }
-
     virObjectUnlock(client);
     return fd;
 }
diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h
index 9cf32091f5..3702f7fe5a 100644
--- a/src/rpc/virnetclient.h
+++ b/src/rpc/virnetclient.h
@@ -95,7 +95,7 @@ void virNetClientSetCloseCallback(virNetClientPtr client,
                                   virFreeCallback ff);
 
 int virNetClientGetFD(virNetClientPtr client);
-int virNetClientDupFD(virNetClientPtr client, bool cloexec);
+int virNetClientDupFD(virNetClientPtr client);
 
 bool virNetClientHasPassFD(virNetClientPtr client);
 
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 9/21/18 5:29 AM, Michal Privoznik wrote:
> There is one caller (virSecurityManagerMetadataLock) which
> duplicates the connection FD and wants to have the flag set.
> However, trying to set the flag after dup() is not safe as
> another thread might fork() meanwhile. Therefore, switch to
> duplicating with the flag set and only let callers refine this
> later.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/locking/domain_lock.c       | 18 ++++++++++++++++++
>  src/locking/lock_driver_lockd.c |  2 +-
>  src/rpc/virnetclient.c          |  9 +--------
>  src/rpc/virnetclient.h          |  2 +-
>  4 files changed, 21 insertions(+), 10 deletions(-)
> 

> diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
> index 705b132457..db20fa86a3 100644
> --- a/src/locking/domain_lock.c
> +++ b/src/locking/domain_lock.c
> @@ -188,6 +188,24 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin,
>      ret = virLockManagerAcquire(lock, NULL, flags,
>                                  dom->def->onLockFailure, fd);
>  
> +    if (ret >= 0 && fd && *fd >= 0 && virSetInherit(*fd, true) < 0) {

Not quite sure 'how' ret > 0, but I'll go with it considering other
consumers perform the same check.

> +        int saved_errno = errno;
> +        virErrorPtr origerr;
> +
> +        virErrorPreserveLast(&origerr);
> +
> +        if (virLockManagerRelease(lock, NULL, 0) < 0)
> +            VIR_WARN("Unable to release acquired resourced in cleanup path");

*resource(s)

> +
> +        virErrorRestore(&origerr);
> +        errno = saved_errno;
> +
> +        virReportSystemError(errno, "%s",
> +                             _("Cannot disable close-on-exec flag"));
> +
> +        ret = -1;
> +    }
> +

OK so this perhaps *is* the only thing you're after. Discounting patch2,
you get a dup()'d socket and you then remove the CLOEXEC from it.

The rest of the patch doesn't seem to matter.  Perhaps some day there is
a virNetClientDupFD consumer that wants to pass @true, then they have to
add back all that you removed.

John

>      virLockManagerFree(lock);
>  
>      return ret;
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 0c672b05b1..9b1943daa6 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -796,7 +796,7 @@ static int virLockManagerLockDaemonAcquire(virLockManagerPtr lock,
>          goto cleanup;
>  
>      if (fd &&
> -        (*fd = virNetClientDupFD(client, false)) < 0)
> +        (*fd = virNetClientDupFD(client)) < 0)
>          goto cleanup;
>  
>      if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) {
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index 40ed3fde6d..6b0ddbeaad 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -712,19 +712,12 @@ int virNetClientGetFD(virNetClientPtr client)
>  }
>  
>  
> -int virNetClientDupFD(virNetClientPtr client, bool cloexec)
> +int virNetClientDupFD(virNetClientPtr client)
>  {
>      int fd;
>  
>      virObjectLock(client);
> -
>      fd = virNetSocketDupFD(client->sock);
> -    if (!cloexec && fd >= 0 && virSetInherit(fd, true)) {
> -        virReportSystemError(errno, "%s",
> -                             _("Cannot disable close-on-exec flag"));
> -        VIR_FORCE_CLOSE(fd);
> -    }
> -
>      virObjectUnlock(client);
>      return fd;
>  }
> diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h
> index 9cf32091f5..3702f7fe5a 100644
> --- a/src/rpc/virnetclient.h
> +++ b/src/rpc/virnetclient.h
> @@ -95,7 +95,7 @@ void virNetClientSetCloseCallback(virNetClientPtr client,
>                                    virFreeCallback ff);
>  
>  int virNetClientGetFD(virNetClientPtr client);
> -int virNetClientDupFD(virNetClientPtr client, bool cloexec);
> +int virNetClientDupFD(virNetClientPtr client);
>  
>  bool virNetClientHasPassFD(virNetClientPtr client);
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 09/25/2018 06:29 PM, John Ferlan wrote:
> 
> 
> On 9/21/18 5:29 AM, Michal Privoznik wrote:
>> There is one caller (virSecurityManagerMetadataLock) which
>> duplicates the connection FD and wants to have the flag set.
>> However, trying to set the flag after dup() is not safe as
>> another thread might fork() meanwhile. Therefore, switch to
>> duplicating with the flag set and only let callers refine this
>> later.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/locking/domain_lock.c       | 18 ++++++++++++++++++
>>  src/locking/lock_driver_lockd.c |  2 +-
>>  src/rpc/virnetclient.c          |  9 +--------
>>  src/rpc/virnetclient.h          |  2 +-
>>  4 files changed, 21 insertions(+), 10 deletions(-)
>>
> 
>> diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
>> index 705b132457..db20fa86a3 100644
>> --- a/src/locking/domain_lock.c
>> +++ b/src/locking/domain_lock.c
>> @@ -188,6 +188,24 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin,
>>      ret = virLockManagerAcquire(lock, NULL, flags,
>>                                  dom->def->onLockFailure, fd);
>>  
>> +    if (ret >= 0 && fd && *fd >= 0 && virSetInherit(*fd, true) < 0) {
> 
> Not quite sure 'how' ret > 0, but I'll go with it considering other
> consumers perform the same check.

Yeah, maybe there's no way for ret > 0 right now. I am just trying to be
more future proof and follow our (perhaps unwritten) rule on return
values: a negative value means error, zero or positive value means success.

> 
>> +        int saved_errno = errno;
>> +        virErrorPtr origerr;
>> +
>> +        virErrorPreserveLast(&origerr);
>> +
>> +        if (virLockManagerRelease(lock, NULL, 0) < 0)
>> +            VIR_WARN("Unable to release acquired resourced in cleanup path");
> 
> *resource(s)
> 
>> +
>> +        virErrorRestore(&origerr);
>> +        errno = saved_errno;
>> +
>> +        virReportSystemError(errno, "%s",
>> +                             _("Cannot disable close-on-exec flag"));
>> +
>> +        ret = -1;
>> +    }
>> +
> 
> OK so this perhaps *is* the only thing you're after. Discounting patch2,
> you get a dup()'d socket and you then remove the CLOEXEC from it.

Actually, I am trying to fix leaking FD when doing metadata locking.

> 
> The rest of the patch doesn't seem to matter.  Perhaps some day there is
> a virNetClientDupFD consumer that wants to pass @true, then they have to
> add back all that you removed.

That should never happen (TM). If somebody will try to revert these two
patches they will have to deal with FD leaks in some way. I'd be
interested to see how they do that.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list