674470 - PR_MemMap documentation mentions incorrect restriction

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Gian-Carlo Pascutto [:gcp]]

In the documentation for PR_MemMap, the following restrictions are listed:

http://www.mozilla.org/projects/nspr/reference/html/priofnc.html#19516

offset: The starting offset of the section of file to be mapped. The offset must be aligned to whole pages.
len: Length of the section of the file to be mapped. *The length must be a multiple of whole pages*. 

I believe the latter restriction is a mistake and not actually needed. For example, POSIX says the following of mmap: http://pubs.opengroup.org/onlinepubs/7908799/xsh/mmap.html

"Thus, while the argument len need not meet a size or alignment constraint,..."

In the use case where you mmap a physical file, it also makes little sense to have to align the size to pages, instead of being able to map the actual length of the file.

Looking through the Mozilla codebase, there are several users that are not aligning length: https://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsZipArchive.cpp

Either the documentation is wrong (most likely) or the users of the function are broken.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

The prio.h header supports this:
/*
 * return the alignment (in bytes) of the offset argument to PR_MemMap
 */
NSPR_API(PRInt32) PR_GetMemMapAlignment(void);

NSPR_API(void *) PR_MemMap(
    PRFileMap *fmap,
    PROffset64 offset,  /* must be aligned and sized according to the
                         * return value of PR_GetMemMapAlignment() */
    PRUint32 len);

This only talks about offset, not length.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

There is also no documentation about what value should be considered an error. I see at least 2 different usages in the Mozilla codebase. Most code assumes 0 is returned on error, but for example nsprpub/tools/httpget.c seems to assume -1 is returned on error.

Comment 3

[Wan-Teh Chang]

PR_MemMap returns NULL on failure.  nsprpub/tools/httpget.c is wrong.
The (void *) -1 return value is used by the Unix mmap() function, but
NSPR changes that to NULL:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/md/unix/unix.c&rev=3.59&mark=3603,3612-3613,3615#3603

I agree the comment for the 'len' argument is wrong.  I don't see any
such requirement for the dwNumberOfBytesToMap argument to the
Windows MapViewOfFile Function either:
http://msdn.microsoft.com/en-us/library/aa366761%28v=vs.85%29.aspx

Could you write a patch to fix the comment and httpget.c?  Thanks.

Comment 4

[Wan-Teh Chang]

Attachment: Patch for httpget.c

gcp: I just realized that you are reporting errors in NSPR's
online documentation.  I fixed both places:
http://www.mozilla.org/projects/nspr/reference/html/priofnc.html#19516
https://developer.mozilla.org/en/PR_MemMap

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 549005 [details] [diff] [review]
Patch for httpget.c

> 	PR_CloseFileMap(outfMap);
> 	return PR_FAILURE;
>     }
>     PR_ASSERT(addr != (void *) -1);

Maybe that PR_ASSERT can go too. It's probably unnecessary.

Comment 6

[Wan-Teh Chang]

Attachment: Patch for httpget.c, v2

I removed that PR_ASSERT.  Thanks for the suggestion.

Patch checked in on the NSPR trunk (NSPR 4.8.9).

Checking in httpget.c;
/cvsroot/mozilla/nsprpub/tools/httpget.c,v  <--  httpget.c
new revision: 3.8; previous revision: 3.7
done