39011 - Problems with ssl_EmulateSendFile

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P3)

Description

[Wan-Teh Chang]

mozilla/security/nss/lib/ssl/emulate.c, revision 1.1
Function ssl_EmulateSendFile

1. The alignment of the offset argument to PR_MemMap
   is not the page size on Windows.  (See bug #38896.)
   On Windows you need to call GetSystemInfo() and
   align the offset to the "allocation granularity".
2. The size argument to PR_CreateFileMap should be
   0 or file size (info.size).
3. Strictly speaking, you need to pass 64-bit integers
   to PR_CreateFileMap and PR_MemMap as the second
   argument.

ssl_EmulateTransmitFile doesn't have problems 1 and 2
(assuming TRANSMITFILE_MMAP_CHUNK, 256K, meets the
alignment requirement on all platforms).

Comment 1

[Wan-Teh Chang]

Attachment: A patch for problems 1 and 2.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh,

1. Bug 38896 doesn't seem to have anything to do with this.
I suspect you meant to cite a different bug, but don't know
which one.  Which one was it?

2. I object to having to make an OS-dependent call in order
to use NSPR's memory mapping.  It seems to defeat the whole
purpose of having a "portable" runtime API.  I guess I'm 
saying I think there needs to be an NSPR function that returns
the mem mapping alignment boundary.

3. The code in ssl_EmulateSendFile, which was copied more-or-less
out of NSPR, depends on pagesize being a power of 2, and generates
incorrect results if it is not.  Are we guaranteed that it is 
always a power of two on Windows?

4. What does a zero size mean, exactly, for PR_CreateFileMap?

5. If the only valid values for size are 0 (which I gather 
implicitly means the size of the file), or the size of the
file, then what purpose does this argument serve? 
Isn't the size argument unnecessary? 

5. Does passing 0 for the size argument in PR_CreateFileMap work
on all platforms?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

emulate.c rev 1.2 contains more-or-less the changes you suggested.
I passed info.size, rather than zero, for the file size argument
to PR_CreateFileMap.

Comment 4

[Wan-Teh Chang]

1. The related bug that I meant to cite should be
bug #38996, "New function PR_GetMemMapAlignment".

2. The reason my patch (id=8574) has to make an
OS-dependent call is that NSPR 4.0 and earlier
incorrectly assume that the mem mapping
alignment boundary is the page size.  To work
around this NSPR bug, you have to make an
OS-dependent call.  When you upgrade to NSPR
4.1 you will be able to call the new function
PR_GetMemMapAlignment() instead. (But you
might as well use the new PR_EmulateSendFile
function in NSPR 4.1 directly.)

3. I'm not sure if any OS guarantees that the
page size or rather mem mapping alignment
boundary is a power of 2.  I think any sane
OS designer will use a power of 2 to make
certain operations faster.  We can certainly
replace the & operations (which rely on the
mem mapping alignment boundary being a power
of 2) with / and % operations.

4,5,6. The size argument to PR_CreateFileMap
causes the underlying file to be extended if
it is larger than the current size. In the
current implementation, a size argument of 0
means the current file size, however this
behavior is not documented so it's a good
idea to just pass the current file size to
PR_CreateFileMap.

Comment 5

[Wan-Teh Chang]

You might want to convert info.size to a PRInt64
and pass the PRInt64 as the second argument to
PR_CreateFileMap.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Why?  Given that the function is prototyped, and the argument
is typed in the prototype, ANSI c guarantees that the correct
conversion will be implicitly supplied.  What value is there
in explicitly supplying it?

Comment 7

[Wan-Teh Chang]

PRInt64 is defined as a struct on platforms
without a 64-bit integer type.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Do we actually support any such platforms any more?
Wasn't Win16 the last one?

Comment 9

[Wan-Teh Chang]

Since that code is ifdef's with WIN32 and
XP_UNIX, this could only break under older
Unix compilers.  The recent Unix compilers
and gcc all support a 64-bit integer type.
I just tend to be more conservative.

Comment 10

[John Unruh]

Verified per wtc's comments.