Closed Bug 828894 Opened 9 years ago Closed 9 years ago

Possible off-by-one-page mapping of anonymous memory in custom linker

Categories

(Core :: mozglue, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: glandium, Assigned: glandium)

Details

Attachments

(1 file)

Similarly to bug 824715, _MappableBuffer miscomputes where to mmap an anonymous page to create a gap. "Fortunately", this only happens when the length of the buffer is exactly a multiple of the page size, which is pretty rare (the buffer size currently being the size of the files being uncompressed, library files rarely have such sizes).
Comment on attachment 700274 [details] [diff] [review]
Fix possible off-by-one-page in custom linker

Review of attachment 700274 [details] [diff] [review]:
-----------------------------------------------------------------

If |length| is page-aligned and occupies N pages, then we're going to mmap N+1 pages.  We're then going to make an anonymous mapping that covers the last page of that space--the "gap".  That doesn't sound right.  For non-page-aligned values, the overlap is less than a full page, which is fine.  But here, the overlap is a full page.  Does a zero-byte sized "gap" DTRT?

ISTM that the simpler solution is to just mmap |length| bytes initially, then mmap an anonymous page at |buf + ((length + PAGE_SIZE - 1) & PAGE_MASK)|.
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 700274 [details] [diff] [review]
> Fix possible off-by-one-page in custom linker
> 
> Review of attachment 700274 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If |length| is page-aligned and occupies N pages, then we're going to mmap
> N+1 pages.  We're then going to make an anonymous mapping that covers the
> last page of that space--the "gap".  That doesn't sound right.

That's exactly what' wanted.

> For non-page-aligned values, the overlap is less than a full page, which is
> fine.  But here, the overlap is a full page.

For non-page-aligned values, we still map N+1 pages.

> Does a zero-byte sized "gap" DTRT?
>
> ISTM that the simpler solution is to just mmap |length| bytes initially,
> then mmap an anonymous page at |buf + ((length + PAGE_SIZE - 1) &
> PAGE_MASK)|.

That doesn't guarantee that the page at |buf + ((length + PAGE_SIZE - 1) & PAGE_MASK)| is free, which is why i'm mapping length + PAGE_SIZE.
Comment on attachment 700274 [details] [diff] [review]
Fix possible off-by-one-page in custom linker

Review of attachment 700274 [details] [diff] [review]:
-----------------------------------------------------------------

OK, if a zero-byte gap is fine, then r=me.

::: mozglue/linker/Mappable.cpp
@@ +200,5 @@
>       * descriptor right away. Allocate one page more than requested so that
>       * there is a gap between this mapping and the subsequent one. */
>      void *buf = ::mmap(NULL, length + PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>      if (buf != MAP_FAILED) {
>        /* Actually create the gap with anonymous memory */

Modifying this comment would not be a bad thing.  Maybe something like:

/* Cover the gap with anonymous memory.  In the case of |length| being page-aligned,
   the anonymous memory will fully cover the additional page we allocated above.  */

I'm not sure how to explain it succinctly.
Attachment #700274 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/b88aa014ffa6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.