1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemuBuildHostNetStr: Don't leak @addr

Michal Privoznik posted 1 patch 6 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/74662c78ad210335a1fd6e90e5f7cece3c75962c.1505998512.git.mprivozn@redhat.com
src/qemu/qemu_command.c | 1 +
1 file changed, 1 insertion(+)
[libvirt] [PATCH] qemuBuildHostNetStr: Don't leak @addr
Posted by Michal Privoznik 6 years, 7 months ago 
The virSocketAddrFormat() allocates the string and it's caller
responsibility to free it afterwards.

==28857== 11 bytes in 1 blocks are definitely lost in loss record 37 of 168
==28857==    at 0x4C2BEDF: malloc (vg_replace_malloc.c:299)
==28857==    by 0x9A81D79: strdup (in /lib64/libc-2.23.so)
==28857==    by 0x5DA3BF0: virStrdup (virstring.c:902)
==28857==    by 0x5D96182: virSocketAddrFormatFull (virsocketaddr.c:427)
==28857==    by 0x5D95E13: virSocketAddrFormat (virsocketaddr.c:352)
==28857==    by 0x5706890: qemuBuildHostNetStr (qemu_command.c:3891)
==28857==    by 0x57138D3: qemuBuildInterfaceCommandLine (qemu_command.c:8597)
==28857==    by 0x5713D6A: qemuBuildNetCommandLine (qemu_command.c:8699)
==28857==    by 0x57176F6: qemuBuildCommandLine (qemu_command.c:10027)
==28857==    by 0x5769D61: qemuProcessCreatePretendCmd (qemu_process.c:6004)
==28857==    by 0x4056EC: testCompareXMLToArgv (qemuxml2argvtest.c:502)
==28857==    by 0x41DF40: virTestRun (testutils.c:180)

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_command.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9b3e3fc04..abeb24846 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3900,6 +3900,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
             if (ip->prefix)
                 virBufferAsprintf(&buf, "/%u", ip->prefix);
             virBufferAddChar(&buf, ',');
+            VIR_FREE(addr);
         }
         break;
 
-- 
2.13.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildHostNetStr: Don't leak @addr
Posted by John Ferlan 6 years, 7 months ago 

On 09/21/2017 08:55 AM, Michal Privoznik wrote:
> The virSocketAddrFormat() allocates the string and it's caller
> responsibility to free it afterwards.
> 
> ==28857== 11 bytes in 1 blocks are definitely lost in loss record 37 of 168
> ==28857==    at 0x4C2BEDF: malloc (vg_replace_malloc.c:299)
> ==28857==    by 0x9A81D79: strdup (in /lib64/libc-2.23.so)
> ==28857==    by 0x5DA3BF0: virStrdup (virstring.c:902)
> ==28857==    by 0x5D96182: virSocketAddrFormatFull (virsocketaddr.c:427)
> ==28857==    by 0x5D95E13: virSocketAddrFormat (virsocketaddr.c:352)
> ==28857==    by 0x5706890: qemuBuildHostNetStr (qemu_command.c:3891)
> ==28857==    by 0x57138D3: qemuBuildInterfaceCommandLine (qemu_command.c:8597)
> ==28857==    by 0x5713D6A: qemuBuildNetCommandLine (qemu_command.c:8699)
> ==28857==    by 0x57176F6: qemuBuildCommandLine (qemu_command.c:10027)
> ==28857==    by 0x5769D61: qemuProcessCreatePretendCmd (qemu_process.c:6004)
> ==28857==    by 0x4056EC: testCompareXMLToArgv (qemuxml2argvtest.c:502)
> ==28857==    by 0x41DF40: virTestRun (testutils.c:180)
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_command.c | 1 +
>  1 file changed, 1 insertion(+)
> 

Coverity saw this too - I just hadn't got around to posting something...

FWIW: The VIR_FREE() under cleanup is not necessary

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildHostNetStr: Don't leak @addr
Posted by Ján Tomko 6 years, 7 months ago 
On Thu, Sep 21, 2017 at 02:55:12PM +0200, Michal Privoznik wrote:
>The virSocketAddrFormat() allocates the string and it's caller
>responsibility to free it afterwards.
>

Introduced by not-yet-released commit 8703813

>==28857== 11 bytes in 1 blocks are definitely lost in loss record 37 of 168
>==28857==    at 0x4C2BEDF: malloc (vg_replace_malloc.c:299)
>==28857==    by 0x9A81D79: strdup (in /lib64/libc-2.23.so)
>==28857==    by 0x5DA3BF0: virStrdup (virstring.c:902)
>==28857==    by 0x5D96182: virSocketAddrFormatFull (virsocketaddr.c:427)
>==28857==    by 0x5D95E13: virSocketAddrFormat (virsocketaddr.c:352)
>==28857==    by 0x5706890: qemuBuildHostNetStr (qemu_command.c:3891)
>==28857==    by 0x57138D3: qemuBuildInterfaceCommandLine (qemu_command.c:8597)
>==28857==    by 0x5713D6A: qemuBuildNetCommandLine (qemu_command.c:8699)
>==28857==    by 0x57176F6: qemuBuildCommandLine (qemu_command.c:10027)
>==28857==    by 0x5769D61: qemuProcessCreatePretendCmd (qemu_process.c:6004)
>==28857==    by 0x4056EC: testCompareXMLToArgv (qemuxml2argvtest.c:502)
>==28857==    by 0x41DF40: virTestRun (testutils.c:180)
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_command.c | 1 +
> 1 file changed, 1 insertion(+)
>

ACK

Looks like aiforaf in libvirt_nss.c could use the same treatment.

Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildHostNetStr: Don't leak @addr
Posted by Michal Privoznik 6 years, 7 months ago 
On 09/21/2017 04:01 PM, Ján Tomko wrote:
> On Thu, Sep 21, 2017 at 02:55:12PM +0200, Michal Privoznik wrote:
>> The virSocketAddrFormat() allocates the string and it's caller
>> responsibility to free it afterwards.
>>
> 
> Introduced by not-yet-released commit 8703813
> 
>> ==28857== 11 bytes in 1 blocks are definitely lost in loss record 37
>> of 168
>> ==28857==    at 0x4C2BEDF: malloc (vg_replace_malloc.c:299)
>> ==28857==    by 0x9A81D79: strdup (in /lib64/libc-2.23.so)
>> ==28857==    by 0x5DA3BF0: virStrdup (virstring.c:902)
>> ==28857==    by 0x5D96182: virSocketAddrFormatFull (virsocketaddr.c:427)
>> ==28857==    by 0x5D95E13: virSocketAddrFormat (virsocketaddr.c:352)
>> ==28857==    by 0x5706890: qemuBuildHostNetStr (qemu_command.c:3891)
>> ==28857==    by 0x57138D3: qemuBuildInterfaceCommandLine
>> (qemu_command.c:8597)
>> ==28857==    by 0x5713D6A: qemuBuildNetCommandLine (qemu_command.c:8699)
>> ==28857==    by 0x57176F6: qemuBuildCommandLine (qemu_command.c:10027)
>> ==28857==    by 0x5769D61: qemuProcessCreatePretendCmd
>> (qemu_process.c:6004)
>> ==28857==    by 0x4056EC: testCompareXMLToArgv (qemuxml2argvtest.c:502)
>> ==28857==    by 0x41DF40: virTestRun (testutils.c:180)
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/qemu/qemu_command.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
> 
> ACK
> 
> Looks like aiforaf in libvirt_nss.c could use the same treatment.

I've no idea what aiforaf mean.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildHostNetStr: Don't leak @addr
Posted by Peter Krempa 6 years, 7 months ago 
On Thu, Sep 21, 2017 at 16:08:05 +0200, Michal Privoznik wrote:
> On 09/21/2017 04:01 PM, Ján Tomko wrote:
> > On Thu, Sep 21, 2017 at 02:55:12PM +0200, Michal Privoznik wrote:

[...]

> >>
> > 
> > ACK
> > 
> > Looks like aiforaf in libvirt_nss.c could use the same treatment.
> 
> I've no idea what aiforaf mean.

$ git grep aiforaf

tools/nss/libvirt_nss.c:aiforaf(const char *name, int af, struct addrinfo *pai, struct addrinfo **aip)
tools/nss/libvirt_nss.c:        aiforaf(name, AF_INET6, ai, &cur);
tools/nss/libvirt_nss.c:        aiforaf(name, AF_INET, ai, &cur);

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildHostNetStr: Don't leak @addr
Posted by Michal Privoznik 6 years, 7 months ago 
On 09/21/2017 04:58 PM, Peter Krempa wrote:
> On Thu, Sep 21, 2017 at 16:08:05 +0200, Michal Privoznik wrote:
>> On 09/21/2017 04:01 PM, Ján Tomko wrote:
>>> On Thu, Sep 21, 2017 at 02:55:12PM +0200, Michal Privoznik wrote:
> 
> [...]
> 
>>>>
>>>
>>> ACK
>>>
>>> Looks like aiforaf in libvirt_nss.c could use the same treatment.
>>
>> I've no idea what aiforaf mean.
> 
> $ git grep aiforaf
> 
> tools/nss/libvirt_nss.c:aiforaf(const char *name, int af, struct addrinfo *pai, struct addrinfo **aip)
> tools/nss/libvirt_nss.c:        aiforaf(name, AF_INET6, ai, &cur);
> tools/nss/libvirt_nss.c:        aiforaf(name, AF_INET, ai, &cur);
> 

Ah, aiforaf(). This is why I put brackets after function names so that
it's visible what I mean. Anyway, I was wondering why I haven't hit that
memleak and it's happening only on BSDs. I'll post a patch shortly.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildHostNetStr: Don't leak @addr
Posted by Laine Stump 6 years, 7 months ago 
On 09/21/2017 10:58 AM, Peter Krempa wrote:
> On Thu, Sep 21, 2017 at 16:08:05 +0200, Michal Privoznik wrote:
>> On 09/21/2017 04:01 PM, Ján Tomko wrote:
>>> On Thu, Sep 21, 2017 at 02:55:12PM +0200, Michal Privoznik wrote:
> [...]
>
>>> ACK
>>>
>>> Looks like aiforaf in libvirt_nss.c could use the same treatment.
>> I've no idea what aiforaf mean.
> $ git grep aiforaf
>
> tools/nss/libvirt_nss.c:aiforaf(const char *name, int af, struct addrinfo *pai, struct addrinfo **aip)
> tools/nss/libvirt_nss.c:        aiforaf(name, AF_INET6, ai, &cur);
> tools/nss/libvirt_nss.c:        aiforaf(name, AF_INET, ai, &cur);

fwiw, ianal but that string of letters is such gibberish, I initially
thought it was an acronym :-)

(okay, I'll stfu now)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuBuildHostNetStr: Don't leak @addr
Posted by Laine Stump 6 years, 7 months ago 
On 09/21/2017 08:55 AM, Michal Privoznik wrote:
> The virSocketAddrFormat() allocates the string and it's caller
> responsibility to free it afterwards.
>
> ==28857== 11 bytes in 1 blocks are definitely lost in loss record 37 of 168
> ==28857==    at 0x4C2BEDF: malloc (vg_replace_malloc.c:299)
> ==28857==    by 0x9A81D79: strdup (in /lib64/libc-2.23.so)
> ==28857==    by 0x5DA3BF0: virStrdup (virstring.c:902)
> ==28857==    by 0x5D96182: virSocketAddrFormatFull (virsocketaddr.c:427)
> ==28857==    by 0x5D95E13: virSocketAddrFormat (virsocketaddr.c:352)
> ==28857==    by 0x5706890: qemuBuildHostNetStr (qemu_command.c:3891)
> ==28857==    by 0x57138D3: qemuBuildInterfaceCommandLine (qemu_command.c:8597)
> ==28857==    by 0x5713D6A: qemuBuildNetCommandLine (qemu_command.c:8699)
> ==28857==    by 0x57176F6: qemuBuildCommandLine (qemu_command.c:10027)
> ==28857==    by 0x5769D61: qemuProcessCreatePretendCmd (qemu_process.c:6004)
> ==28857==    by 0x4056EC: testCompareXMLToArgv (qemuxml2argvtest.c:502)
> ==28857==    by 0x41DF40: virTestRun (testutils.c:180)
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_command.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 9b3e3fc04..abeb24846 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3900,6 +3900,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
>              if (ip->prefix)
>                  virBufferAsprintf(&buf, "/%u", ip->prefix);
>              virBufferAddChar(&buf, ',');
> +            VIR_FREE(addr);
>          }
>          break;
>  


ACK.


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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.