Clean up the memory mapped array buffer implementations

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ehoogeveen, Assigned: ehoogeveen)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Just as bug 988813 was landing I happened to look at the Linux implementation, and it struck me that the two implementations are actually pretty similar, but the differences in coding style obscure the similarity. There are also some behavioral differences that we overlooked.
(Assignee)

Comment 1

2 years ago
Created attachment 8787214 [details] [diff] [review]
Part 1: Fix and enable tests that were still disabled on Windows.

While working on this, I noticed that we aren't running the tests on Windows at all! Let's fix that.

The _get_osfhandle() stuff is a bit weird - apparently the file descriptor you get from open() or fileno() isn't actually a handle that Windows understands, so we have to convert it to the right kind of file descriptor (actually a handle) just for Windows. XHR actually *does* pass the right kind of file descriptor (and uses a fiddly NSPR function to get it), so it doesn't make sense to do this in AllocateMappedContent itself.
Attachment #8787214 - Flags: review?(sphink)
(Assignee)

Comment 2

2 years ago
Created attachment 8787218 [details] [diff] [review]
Part 2: Clean up AllocateMappedContent and make it consistent across all OSes.

There are a few things about this that are somewhat controversial since they represent changes in behavior:

1) I moved the file existence and size checks into an #ifdef DEBUG block. I'm pretty sure CreateFileMapping/MapViewOfFile and mmap will just fail if the file doesn't exist or the offset is out of bounds, but this also just seems like API abuse to me. Why are you using this function on a file that's too small or doesn't exist? It seems to me like the kind of thing assertions are meant to catch. I had to remove two tests that would now crash otherwise.

2) I removed the memsets from the Linux implementation that zero out everything before and after the requested mapping. This is technically part of the spec, but nothing relies on it outside of the tests, and doesn't it defeat the point of making this copy-on-write to begin with? Either way, this is something we should make consistent between Linux and Windows. Do you think we should consult with the people who made the spec about this?

Personally I don't think the memsets offer much additional safety; if you're reading bytes from before or after your requested mapping, something is going pretty wrong already. Anyway, let me know what you think.
Attachment #8787218 - Flags: review?(sphink)
(Assignee)

Comment 3

2 years ago
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)
> but nothing relies on it outside of the tests

Er, nothing relies on it *period*. I was thinking about the first change when I added this line.
Comment on attachment 8787214 [details] [diff] [review]
Part 1: Fix and enable tests that were still disabled on Windows.

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

I'm always expecting that we'll have to get more serious about how we pass around and manage file descriptors portably. But I'm fine with kicking the can down the road a little farther. :-)
Attachment #8787214 - Flags: review?(sphink) → review+
Comment on attachment 8787218 [details] [diff] [review]
Part 2: Clean up AllocateMappedContent and make it consistent across all OSes.

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

I'm going to disagree with the API abuse argument, because in practice it seems like it would be impossible to avoid in a race-free way. You can't know that someone didn't truncate the file in between checks.

But by the same argument, checking and returning nullptr is kind of wrong too. I think it'd probably be better to either (1) remove all the checking code and let the mapping call fail; or (2) go the other way: leave the checks in, but only warn if they fail. Continue on and do the (very probably) failing mapping call anyway.

Otherwise, it just feels like it's a recipe for intermittents.

::: js/src/gc/Memory.cpp
@@ +307,2 @@
>      if (!hMap)
>          return nullptr;

Do the callers do anything with GetLastError() if this returns nullptr? I guess we can't report anything in this code.

@@ -815,5 @@
> -    // Reset the data before target file, which we don't need to see.
> -    memset(buf, 0, offset - pa_start);
> -
> -    // Reset the data after target file, which we don't need to see.
> -    memset(buf + (offset - pa_start) + length, 0, pa_end - (offset + length));

Rereading the original bug where this stuff was added, I think it might be better to keep this stuff, but DEBUG only. The reason is that this is used to mmap chunks of a file, possibly compressed. If the caller were to accidentally pass in too short a length, and depend on stuff outside of its mapped window, then it won't show up right away.

Clearing the stuff before the mapping seems less useful.

I'm not sure if reading that memory would actually work. If it wouldn't, ignore this comment.
Attachment #8787218 - Flags: review?(sphink)
(Oh, and by the way, I noticed that duplication too but didn't want to deal with it. So thank you for making them more parallel; it was *very* unclear what the difference was and whether the differences were intentional or not.)
(Assignee)

Comment 7

2 years ago
Created attachment 8787650 [details] [diff] [review]
Part 2 v2: Clean up AllocateMappedContent and make it consistent across all OSes.

Thanks for the quick reviews!

(In reply to Steve Fink [:sfink] [:s:] from comment #5)
> I'm going to disagree with the API abuse argument, because in practice it
> seems like it would be impossible to avoid in a race-free way. You can't
> know that someone didn't truncate the file in between checks.

Hm, good point.

> But by the same argument, checking and returning nullptr is kind of wrong
> too. I think it'd probably be better to either (1) remove all the checking
> code and let the mapping call fail; or (2) go the other way: leave the
> checks in, but only warn if they fail. Continue on and do the (very
> probably) failing mapping call anyway.
> 
> Otherwise, it just feels like it's a recipe for intermittents.

OK, I decided to test this, and it turns out that mmap will happily give you a mapping that extends (or even starts!) past the end of the file. So we definitely need those tests on Linux, racy or not. Hopefully writing these resources simply isn't interleaved with reading from them - that sounds like a recipe for disaster to me in any case. On Windows we can rely on MapViewOfFile to catch out-of-bounds access, so I removed the checks there.

> ::: js/src/gc/Memory.cpp
> @@ +307,2 @@
> >      if (!hMap)
> >          return nullptr;
> 
> Do the callers do anything with GetLastError() if this returns nullptr? I
> guess we can't report anything in this code.

The only real caller is ArrayBufferBuilder::mapToFileInPackage() [1], which in turn is only called by XMLHttpRequestMainThread::OnStartRequest() [2]. The former just returns NS_ERROR_FAILURE if the requested mapping is null for any reason, and the latter gives a generic warning on failure (then retries using malloc). I don't think we can do much here since it just falls back anyway.

> @@ -815,5 @@
> > -    // Reset the data before target file, which we don't need to see.
> > -    memset(buf, 0, offset - pa_start);
> > -
> > -    // Reset the data after target file, which we don't need to see.
> > -    memset(buf + (offset - pa_start) + length, 0, pa_end - (offset + length));
> 
> Rereading the original bug where this stuff was added, I think it might be
> better to keep this stuff, but DEBUG only. The reason is that this is used
> to mmap chunks of a file, possibly compressed. If the caller were to
> accidentally pass in too short a length, and depend on stuff outside of its
> mapped window, then it won't show up right away.
> 
> Clearing the stuff before the mapping seems less useful.
> 
> I'm not sure if reading that memory would actually work. If it wouldn't,
> ignore this comment.

Debug only sounds good to me. These functions should allocate at most a page beyond the requested mapping, so I changed things to zero those out those final bytes on Windows and Linux. Since it's debug-only, I decided to keep zeroing out the bytes *before* the requested mapping as well.

[1] https://dxr.mozilla.org/mozilla-central/rev/b7f7ae14590aced450bb0b0469dfb38edd2c0ace/dom/xhr/XMLHttpRequestMainThread.cpp#3743
[2] https://dxr.mozilla.org/mozilla-central/rev/b7f7ae14590aced450bb0b0469dfb38edd2c0ace/dom/xhr/XMLHttpRequestMainThread.cpp#1830
Attachment #8787218 - Attachment is obsolete: true
Attachment #8787650 - Flags: review?(sphink)
Comment on attachment 8787650 [details] [diff] [review]
Part 2 v2: Clean up AllocateMappedContent and make it consistent across all OSes.

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

I'm not overjoyed with the race on Linux, and I half wonder if this should just propagate mmap semantics through (as in, allow a too-large mapping but don't have it affect the backing file). But I really can't come up with a scenario where the behavior you have here would actually cause problems, and it seems like the size check is much more likely to detect problems, so r=me as is.

I like the added comments too.
Attachment #8787650 - Flags: review?(sphink) → review+
(Assignee)

Comment 10

2 years ago
Great, thanks again for the reviews :)
Keywords: checkin-needed

Comment 11

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/844899534edb
Part 1: Fix and enable tests that were still disabled on Windows. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/13af8367325a
Part 2: Clean up AllocateMappedContent and make it consistent across all OSes. r=sfink
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/844899534edb
https://hg.mozilla.org/mozilla-central/rev/13af8367325a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.