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

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Mike Hommey [:glandium]]

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 1

[Mike Hommey [:glandium]]

Attachment: Fix possible off-by-one-page in custom linker

Comment 2

[Nathan Froyd [:froydnj]]

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)|.

Comment 3

[Mike Hommey [:glandium]]

(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 4

[Nathan Froyd [:froydnj]]

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.

Comment 5

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b88aa014ffa6

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b88aa014ffa6