487537 - Possible uninitialized variable use in pt_RecvFrom()

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Daniel Franke]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090408 Shiretoko/3.5b4pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090408 Shiretoko/3.5b4pre

In the following code from pt_RecvFrom() in nsprpub/pr/src/pthreads/ptio.c:

#ifdef _PR_HAVE_SOCKADDR_LEN
    if (bytes >= 0)
    {
        /* ignore the sa_len field of struct sockaddr */
        if (addr)
        {
            addr->raw.family = ((struct sockaddr*)addr)->sa_family;
        }
    }
#endif /* _PR_HAVE_SOCKADDR_LEN */
#ifdef _PR_INET6
        if (addr && (AF_INET6 == addr->raw.family))
        addr->raw.family = PR_AF_INET6;
#endif

addr->raw.family gets used uninitialized in the last 'if' statement when bytes < 0.  That line should instead read:

        if (bytes >= 0 && addr && (AF_INET6 == addr->raw.family))

Spotted by Valgrind.

Reproducible: Always

Steps to Reproduce:
Call pt_RecvFrom in any manner such that the underlying recvfrom() system call returns -1.
Actual Results:  
Valgrind complains.

Expected Results:  
Blissful silence.

Comment 1

[Daniel Franke]

Attachment: Fix

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 371764 [details] [diff] [review]
Fix

asking self for review

Comment 3

[Wan-Teh Chang]

Comment on attachment 371764 [details] [diff] [review]
Fix

Thanks for the patch.  This patch is correct.  I
suggest adding parentheses around bytes >= 0
to match the style used in that file.

While looking at the file I found that we have
related problems elsewhere in the file: In
pt_GetSockName and pt_GetPeerName, we have

#ifdef _PR_INET6
        if (AF_INET6 == addr->raw.family)
            addr->raw.family = PR_AF_INET6;
#endif

We need a null check for 'addr' in the "if".

We should create a function, Say AdjustAddrFamily,
for this piece of code, which appears in four
functions right now:

static void AdjustAddrFamily(PRNetAddr *addr)
{
#ifdef _PR_HAVE_SOCKADDR_LEN
    /* ignore the sa_len field of struct sockaddr */
    if (addr)
    {
        addr->raw.family = ((struct sockaddr*)addr)->sa_family;
    }
#endif /* _PR_HAVE_SOCKADDR_LEN */
#ifdef _PR_INET6
    if (addr && (AF_INET6 == addr->raw.family))
        addr->raw.family = PR_AF_INET6;
#endif
}

Then in pt_RecvFrom, we can just say:

    if (bytes >= 0)
        AdjustAddrFamily(addr);

Comment 4

[Wan-Teh Chang]

Attachment: Fix v2

This patch is the same as Daniel's fix, but I applied some
source code transformations so that the two blocks of ifdef
code remain similar to the blocks of code in pt_Accept,
pt_GetSockName and pt_GetPeerName.

I checked in this patch on the NSPR trunk (NSPR 4.8) in
mozilla/nsprpub/pr/src/pthreads/ptio.c, rev. 3.111.

Comment 5

[Wan-Teh Chang]

I also found that the missing null check for 'addr' in
pt_GetSockName and pt_GetPeerName that I pointed out in
comment 3 is not a bug because if 'addr' is NULL,
getsockname and getpeername will fail with the EFAULT
error:
http://opengroup.org/onlinepubs/007908799/xns/getsockname.html
http://opengroup.org/onlinepubs/007908799/xns/getpeername.html
So if getsockname or getpeername returns successfully,
we know 'addr' isn't NULL.