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@xxxxxxxxxx>
>> ---
>> 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);
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list