Possible uninitialized variable use in pt_RecvFrom()

RESOLVED FIXED in 4.8

Status

NSPR
NSPR
--
trivial
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Daniel Franke, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.41 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
(Reporter)

Comment 1

9 years ago
Created attachment 371764 [details] [diff] [review]
Fix
Attachment #371764 - Flags: review?(nelson)
Comment on attachment 371764 [details] [diff] [review]
Fix

asking self for review
(Assignee)

Comment 3

9 years ago
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);
Attachment #371764 - Flags: review?(nelson) → review+
(Assignee)

Comment 4

9 years ago
Created attachment 375281 [details] [diff] [review]
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.
Attachment #371764 - Attachment is obsolete: true
(Assignee)

Comment 5

9 years ago
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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 years ago
OS: Linux → All
Resolution: --- → FIXED
Target Milestone: --- → 4.8
You need to log in before you can comment on or make changes to this bug.