Closed Bug 674470 Opened 10 years ago Closed 10 years ago

PR_MemMap documentation mentions incorrect restriction

Categories

(NSPR :: NSPR, defect, P2)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gcp, Assigned: wtc)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
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.
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.
Assignee: wtc → gpascutto
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 4.8.9
Attached patch Patch for httpget.c (obsolete) — Splinter Review
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
Assignee: gpascutto → wtc
Attachment #549005 - Flags: review?(gpascutto)
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.
Attachment #549005 - Flags: review?(gpascutto) → review+
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
Attachment #549005 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.