339994 - Coverity 498, PR_EmulateSendFile may pass NULL to func that dereferences it

Status: NEW

(NSPR :: NSPR, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

PR_EmulateSendFile may pass a NULL mapHandle to PR_MemMap, which dereferences it.

To do so, this sequence of statements:

376  	    if (sfd->file_nbytes)
377  	        file_bytes = sfd->file_nbytes;
378  	    else
379  	        file_bytes = info.size - sfd->file_offset;
380  	
381  	    alignment = PR_GetMemMapAlignment();
382  	
383  	    /* number of initial bytes to skip in mmap'd segment */
384  	    addr_offset = sfd->file_offset % alignment;
385  	
386  	    /* find previous mmap alignment boundary */
387  	    file_mmap_offset = sfd->file_offset - addr_offset;
388  	
389  	    /*
390  	     * If the file is large, mmap and send the file in chunks so as
391  	     * to not consume too much virtual address space
392  	     */
393  	    mmap_len = PR_MIN(file_bytes + addr_offset, SENDFILE_MMAP_CHUNK);
394  	    len = mmap_len - addr_offset;
395  	
396  	    /*
397  	     * Map in (part of) file. Take care of zero-length files.
398  	     */
399  	    if (len) {

must result in len being zero, so that the following lines are not executed:

400  	        LL_I2L(zero64, 0);
401  	        mapHandle = PR_CreateFileMap(sfd->fd, zero64, PR_PROT_READONLY);

There may be several ways this can occur. Here's one:
if "alignment" is greater than SENDFILE_MMAP_CHUNK (currently 256K)
and the caller sets these members of sfd as shown:

sfd->file_offset = SENDFILE_MMAP_CHUNK;
sfd->file_nbytes = any value greater than zero, and not greater than
                   the file size minus SENDFILE_MMAP_CHUNK
sfd->hlen        = any value greater than zero.

I do not know if PR_GetMemMapAlignment() ever returns a value > 256k.
IIRC, on some boxes, the page size in IRIX is 256k on some boxes, 
but that is not greater than 256K.

Comment 1

[Wan-Teh Chang]

If "alignment" is greater than SENDFILE_MMAP_CHUNK, "len"
can be not only 0 but also negative.

The code in that function requires not only SENDFILE_MMAP_CHUNK
be greater than "alignment" but also SENDFILE_MMAP_CHUNK
be a multiple of "alignment".  This requirement is enfored
by an assertion later in the function:

        /*
         * Map in (part of) file
         */
        file_mmap_offset = sfd->file_offset + count - sfd->hlen;
        PR_ASSERT((file_mmap_offset % alignment) == 0);

        LL_I2L(file_mmap_offset64, file_mmap_offset);
        addr = PR_MemMap(mapHandle, file_mmap_offset64, len);

To ensure that SENDFILE_MMAP_CHUNK is a multiple of
"alignment", we can define a variable "sendfile_mmap_chunk"
as n*alignment, where n = 256K/alignment,
and use sendfile_mmap_chunk instead of SENDFILE_MMAP_CHUNK
throughout the code.  I'm not sure if this change will
silence Coverity though.

At line 395 we should also assert that if files_bytes > 0,
then len > 0:
    PR_ASSERT((file_bytes == 0) || len > 0);

The reason is that if files_bytes > 0, then we must call
PR_CreateFileMap to create mapHandle.  Right now that is
done inside the if (len) block.

Sample values of "alignment":
HP-UX ia64 32-bit: 4K
HP-UX ia64 64-bit: 4K
Linux x86: 4K
Linux x86-64: 4K
Mac OS X PowerPC: 4K
Solaris 9 SPARC 32-bit: 8K
Solaris 9 SPARC 64-bit: 8K
Windows XP SP2: 64K

None of these is greater than SENDFILE_MMAP_CHUNK (256K).
So right now this bug is only a theoretical possibility.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.