[libvirt] [PATCH 2/3] virnetsocket: Resolve Coverity RESOURCE_LEAK

[John Ferlan]

On 08/22/2014 12:42 PM, John Ferlan wrote:
> 
> 
> On 08/22/2014 11:49 AM, Michal Privoznik wrote:
>> On 22.08.2014 17:28, John Ferlan wrote:
>>> Since '1b807f92d' - Coverity complains that in the error paths of
>>> both virFork() and virProcessWait() that the 'passfd' will not be closed
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>   src/rpc/virnetsocket.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
>>> index f913365..ce908fa 100644
>>> --- a/src/rpc/virnetsocket.c
>>> +++ b/src/rpc/virnetsocket.c
>>> @@ -593,8 +593,10 @@ int virNetSocketNewConnectUNIX(const char *path,
>>>            * behaviour on sockets according to POSIX, so it doesn't
>>>            * work outside Linux.
>>>            */
>>> -        if ((pid = virFork()) < 0)
>>> +        if ((pid = virFork()) < 0) {
>>> +            VIR_FORCE_CLOSE(passfd);
>>>               goto error;
>>> +        }
>>>   
>>>           if (pid == 0) {
>>>               umask(0077);
>>> @@ -604,8 +606,10 @@ int virNetSocketNewConnectUNIX(const char *path,
>>>               _exit(EXIT_SUCCESS);
>>>           }
>>>   
>>> -        if (virProcessWait(pid, &status, false) < 0)
>>> +        if (virProcessWait(pid, &status, false) < 0) {
>>> +            VIR_FORCE_CLOSE(passfd);
>>>               goto error;
>>> +        }
>>>   
>>>           if (status != EXIT_SUCCESS) {
>>>               /*
>>>
>>
>> Unfortunately not enough context is shown to express myself, so I'll paste interesting knobs from the function:
>>
>>         if (listen(passfd, 0) < 0) {
>>              virReportSystemError(errno, "%s",
>>                                   _("Failed to listen on socket that's about "
>>                                     "to be passed to the daemon"));
>>              goto error;
>>          }
>>  
>>          if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) {
>>              virReportSystemError(errno, _("Failed to connect socket to '%s'"),
>>                                   path);
>>              goto error;
>>          }
>>  
>>          if (virNetSocketForkDaemon(binary, passfd) < 0)
>>              goto error;
>>      }
>>  
>>      localAddr.len = sizeof(localAddr.data);
>>      if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) {
>>          virReportSystemError(errno, "%s", _("Unable to get local socket name"));
>>          goto error;
>>      }
>>  
>>      if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0)))
>>          goto error;
>>  
>>      return 0;
>>  
>>   error:
>>      VIR_FREE(buf);

While implementing the change to add passfd here - I noticed that buf is
never used in this context?  Strange.  There's an initialization to NULL
and a VIR_FREE(buf);, but nothing else uses it, so I removed it.

John

>>      VIR_FORCE_CLOSE(fd);
>>      if (spawnDaemon)
>>          unlink(path);
>>      return -1;
>>  }
>>
>>
>> Here, if any of listen() or connect() fail, the control continues on the error label and the @passfd is leaked again.
>> virNetSocketForkDaemon() is different - it closes passfd on failure. So I suggest moving VIR_FORCE_CLOSE() under the error label.
>>
> 
> Yeah - Coverity only complained about the two paths - so I was hesitant
> to put the VIR_FORCE_CLOSE(passfd) in the error: path...
> 
> In any case, putting it the error: path does resolve the issue as well
> as making sure to initialize passfd = -1 (initializing fd = -1 wasn't
> necessary since there's currently no way to error unless it's set)
> 
> John
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>