Closed Bug 674470 Opened 10 years ago Closed 10 years ago

PR_MemMap documentation mentions incorrect restriction


(NSPR :: NSPR, defect, P2)


(Not tracked)



(Reporter: gcp, Assigned: wtc)



(1 file, 1 obsolete file)

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

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:

"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:

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:,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:

Could you write a patch to fix the comment and httpget.c?  Thanks.
Assignee: wtc → gpascutto
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:
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
Attachment #549005 - Attachment is obsolete: true
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.