[PATCH] virsocketaddr: Zero @netmask in virSocketAddrPrefixToNetmask()

[Michal Privoznik]

On 10/10/20 6:39 PM, Laine Stump wrote:
> On 10/9/20 10:43 AM, Michal Privoznik wrote:
>> The aim of virSocketAddrPrefixToNetmask() is to initialize passed
>> virSocketAddr structure based on prefix length and family.
>> However, it doesn't set all members in the struct which may lead
>> to reads of uninitialized values:
>>
>> ==15421== Use of uninitialised value of size 8
>> ==15421==    at 0x50F297A: _itoa_word (in /lib64/libc-2.31.so)
>> ==15421==    by 0x510C8FE: __vfprintf_internal (in /lib64/libc-2.31.so)
>> ==15421==    by 0x5120295: __vsnprintf_internal (in /lib64/libc-2.31.so)
>> ==15421==    by 0x50F8969: snprintf (in /lib64/libc-2.31.so)
>> ==15421==    by 0x51BB602: getnameinfo (in /lib64/libc-2.31.so)
>> ==15421==    by 0x496DEE0: virSocketAddrFormatFull (virsocketaddr.c:486)
>> ==15421==    by 0x496DD9F: virSocketAddrFormat (virsocketaddr.c:444)
>> ==15421==    by 0x11871F: networkDnsmasqConfContents 
>> (bridge_driver.c:1404)
>> ==15421==    by 0x1118F5: testCompareXMLToConfFiles 
>> (networkxml2conftest.c:48)
>> ==15421==    by 0x111BAF: testCompareXMLToConfHelper 
>> (networkxml2conftest.c:112)
>> ==15421==    by 0x112679: virTestRun (testutils.c:142)
>> ==15421==    by 0x111D09: mymain (networkxml2conftest.c:144)
>> ==15421==  Uninitialised value was created by a stack allocation
>> ==15421==    at 0x1175D2: networkDnsmasqConfContents 
>> (bridge_driver.c:1056)
>>
>> All callers expect the function to initialize the structure
>> fully.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> 
> 
> Reviewed-by: Laine Stump <laine at redhat.com>
> 
> 
> Did you see actual errors caused by this, or just the valgrind 
> complaints? It's been like this since the function was first implemented 
> in 2010 (commit 1ab80f32. Yes, it was me. Sigh.) I wonder why nobody 
> ever saw this valgrind complaint before. Some check that was newly added?
> 
> 
> (unless you've seen a real-world error, my guess would be that the 
> fields that aren't being initialized are actually ignored when the 
> family is set to INET or INET6, but even then it's still safer to clear 
> everything out.)

Yeah, that's probably what is happening. I have not seen any real world 
error, but I'm writing some patches that touch network and ran the test 
under valgrind only to find the "bug".

Pushed, thanks.

Michal