Cache API should reject Responses with VARY:*

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bkelly, Assigned: Ehsan)

Tracking

unspecified
mozilla40
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(7 attachments, 4 obsolete attachments)

1.17 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
14.14 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
11.98 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
6.26 KB, patch
Details | Diff | Splinter Review
3.66 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
993 bytes, patch
bkelly
: review+
Details | Diff | Splinter Review
6.42 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
The spec has changed to reject on VARY:* because it semantically means "not cacheable":

  https://github.com/slightlyoff/ServiceWorker/issues/656

We should be able to do this immediately upon getting the Response instead of waiting for the VARY comparison logic in DBSchema.
(Reporter)

Comment 1

4 years ago
Ehsan, do you have any interest in doing this one since you looked at the VARY header stuff previously?
Flags: needinfo?(ehsan)
(Assignee)

Comment 2

4 years ago
Sure.  I may get to it next week though.  Let me know if you need this sooner.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
(Assignee)

Comment 3

4 years ago
Created attachment 8589083 [details] [diff] [review]
Part 1: Expose the ErrNum value for an ErrorResult, and make it serializable through IPC
Attachment #8589083 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

4 years ago
Created attachment 8589084 [details] [diff] [review]
Part 2: Make ErrorResult unassignable

This object is not copyable, so it should probably not be
assignable either, especially since the compiler generated
copy constructor can potentially cause UAF issues on the
mMessage member, for example.
Attachment #8589084 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

4 years ago
Created attachment 8589085 [details] [diff] [review]
Part 3: Give ErrorResult a move constructor and a move assignment operator
Attachment #8589085 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

4 years ago
Created attachment 8589086 [details] [diff] [review]
Part 4: Tag AddAllResponse IPC messages with the corresponding ErrNum

This is needed so that we can throw a TypeError from
FetchPut::FetchComplete.  In order to be able to do this, we need to
store the entire ErrorResult in the FetchPut object and deliver the
corresponding ErrNum over IPC so that the TypeError can be created
when rejecting the promise.
Attachment #8589086 - Flags: review?(bkelly)
(Assignee)

Comment 7

4 years ago
Created attachment 8589088 [details] [diff] [review]
Part 5: Do not store or match Response objects with a Vary:* header
Attachment #8589088 - Flags: review?(bkelly)
Comment on attachment 8589084 [details] [diff] [review]
Part 2: Make ErrorResult unassignable

r=me
Attachment #8589084 - Flags: review?(bzbarsky) → review+
Comment on attachment 8589085 [details] [diff] [review]
Part 3: Give ErrorResult a move constructor and a move assignment operator

No, this will totally explode if IsJSException(), because the mJSException of "this" won't be rooted or anything.  You really do need to handle the different members of the union differently.

You probably also need to manage the mMightHaveUnreportedJSException state properly...
Attachment #8589085 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 10

4 years ago
(In reply to Not doing reviews right now from comment #9)
> No, this will totally explode if IsJSException(), because the mJSException
> of "this" won't be rooted or anything.  You really do need to handle the
> different members of the union differently.

Ah, yes.  Is there any way to move a JS::Value?  If not (and I don't think there is), would you be fine with a custom move method that can be called which only works for the !IsJSException() case?
Flags: needinfo?(bzbarsky)
Comment on attachment 8589083 [details] [diff] [review]
Part 1: Expose the ErrNum value for an ErrorResult, and make it serializable through IPC

OK, now that I see what this is doing (in the later patches), I think it's somewhere between wrong and very dangerous.

Some error numbers require string arguments.  If you serialize those across without serializing the arguments, I bet they will read random memory and stick it into the string.

So what we should really have is a way to serialize a Message, not just an ErrNum.  That might need the ParamTraits specialization for ErrNum, but likely does NOT need the public ErrNum getter per se.

I realize in your case you're sending a message that takes no arguments, but I'd rather not have generic infrastructure with footguns here.
Attachment #8589083 - Flags: review?(bzbarsky) → review-
> Ah, yes.  Is there any way to move a JS::Value?

Sure.  Like so:

  myValue = otherValue;
  otherValue.setUndefined();

There, you've moved it.  ;)

The caveat here is that you need to not only move the value but also move the rooting.  That is, js::RemoveRawValueRoot(cx, &otherValue) and js::AddRawValueRoot(cx, &myValue, "ErrorResult::mJSException"); see the existing calls in ThrowJSException and ReportJSException.  You should be able to use nsContentUtils::GetDefaultJSContextForThread to get the cx; it happens that the runtime is all the rooting _really_ cares about.

So the general order of things should be:

1)  Set myValue to undefined.
2)  Root myValue.
3)  Copy otherValue to myValue.
4)  Set otherValue to undefined.
5)  Unroot otherValue.
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 13

4 years ago
I think we could avoid the complexity of doing ErrorResult IPC serialization if we do bug 1120501 first.
(Reporter)

Updated

4 years ago
Attachment #8589086 - Flags: review?(bkelly) → review+
(Reporter)

Comment 14

4 years ago
Comment on attachment 8589088 [details] [diff] [review]
Part 5: Do not store or match Response objects with a Vary:* header

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

r=me with comments addressed.  Mainly, I don't think we should ever see a VARY:* in DBSchema or the later parts of FetchPut with these changes.

Thanks!

::: dom/cache/DBSchema.cpp
@@ +944,5 @@
>      for (; token;
>           token = nsCRT::strtok(rawBuffer, NS_HTTP_HEADER_SEPS, &rawBuffer)) {
>        nsDependentCString header(token);
>        if (header.EqualsLiteral("*")) {
> +        varyHeadersMatch = false;

Can't this be an assert since we fail at the TypeUtils::ToPCacheResponseWithoutBody() call?

::: dom/cache/FetchPut.cpp
@@ +260,5 @@
>                                    *aInternalResponse, rv);
>        if (rv.Failed()) {
> +        mResult = Move(rv);
> +      } else {
> +        aInternalResponse->GetBody(getter_AddRefs(mStateList[i].mResponseStream));

Thanks for fixing the error logic here.

@@ +377,5 @@
>        for (; token;
>             token = nsCRT::strtok(rawBuffer, NS_HTTP_HEADER_SEPS, &rawBuffer)) {
>          nsDependentCString header(token);
>          if (header.EqualsLiteral("*")) {
> +          varyHeadersMatch = false;

Can't this be an assert since we fail at the TypeUtils::ToPCacheResponseWithoutBody() call?

::: dom/cache/test/mochitest/test_cache_match_vary.js
@@ +133,5 @@
> +            .then(function(response) {
> +              return cache.put(requestURL + "4", response);
> +            })),
> +        ensurePromiseRejected(
> +          cache.add(new Request(requestURL + "5", {headers: {"WhatToVary": "*,User-Agent"}}))),

Can you add a case where the * is after another vary header value?  Like "User-Agent,*"?  Just to better exercise the parse loop.
Attachment #8589088 - Flags: review?(bkelly) → review+
(Assignee)

Comment 15

4 years ago
Created attachment 8589339 [details] [diff] [review]
Part 1: Make it possible to send an ErrorResult that doesn't encode a JS exception through the IPDL layer
Attachment #8589083 - Attachment is obsolete: true
Attachment #8589339 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

4 years ago
Created attachment 8589340 [details] [diff] [review]
Part 1: Make it possible to send an ErrorResult that doesn't encode a JS exception through the IPDL layer
Attachment #8589339 - Attachment is obsolete: true
Attachment #8589339 - Flags: review?(bzbarsky)
Attachment #8589340 - Flags: review?(bzbarsky)
Comment on attachment 8589340 [details] [diff] [review]
Part 1: Make it possible to send an ErrorResult that doesn't encode a JS exception through the IPDL layer

>+  Message(Message&& aRHS)

Or you could just use a heap-allocated Message, with nsAutoPtr and forget() in ErrorResult::DeserializeMessage.  Seems like it would be simpler...

>+    mozilla::ErrorResult readValue;

Can just use paramType here?

A question about the mMightHaveUnreportedJSException handling here.  What are the semantics of sending an ErrorResult over IPC?  Should it be considered as "reporting" it in some sense or not?  Because if it is, then seems like serialization or its caller should just WouldReportJSException, or assert that mMightHaveUnreportedJSException is false...

r=me, but that part needs sorting out.
Attachment #8589340 - Flags: review?(bzbarsky) → review+
In fact, maybe we should just MOZ_CRASH if mMightHaveUnreportedJSException?  mMightHaveUnreportedJSException means "this is on a codepath where it could have a JS exception, but it happens to not have one right now because the JS happened not to throw".  Therefore, it should be the case that IsJSException() is true only if mMightHaveUnreportedJSException is true.  Of course we don't have mMightHaveUnreportedJSException in opt builds, so we want to keep the IsJSException() check in practice in case we have no debug coverage at all for the codepaths in question.
(Assignee)

Comment 19

4 years ago
Comment on attachment 8589088 [details] [diff] [review]
Part 5: Do not store or match Response objects with a Vary:* header

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

::: dom/cache/DBSchema.cpp
@@ +944,5 @@
>      for (; token;
>           token = nsCRT::strtok(rawBuffer, NS_HTTP_HEADER_SEPS, &rawBuffer)) {
>        nsDependentCString header(token);
>        if (header.EqualsLiteral("*")) {
> +        varyHeadersMatch = false;

Hmm, which call to ToPCacheResponseWithoutBody?

::: dom/cache/FetchPut.cpp
@@ +377,5 @@
>        for (; token;
>             token = nsCRT::strtok(rawBuffer, NS_HTTP_HEADER_SEPS, &rawBuffer)) {
>          nsDependentCString header(token);
>          if (header.EqualsLiteral("*")) {
> +          varyHeadersMatch = false;

Yes, you're right here.

::: dom/cache/test/mochitest/test_cache_match_vary.js
@@ +133,5 @@
> +            .then(function(response) {
> +              return cache.put(requestURL + "4", response);
> +            })),
> +        ensurePromiseRejected(
> +          cache.add(new Request(requestURL + "5", {headers: {"WhatToVary": "*,User-Agent"}}))),

Good idea!
(Assignee)

Updated

4 years ago
Flags: needinfo?(bkelly)
(Assignee)

Comment 20

4 years ago
Created attachment 8589430 [details] [diff] [review]
Part 1: Make it possible to send an ErrorResult that doesn't encode a JS exception through the IPDL layer

This addresses comment 18.
Attachment #8589340 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Created attachment 8589431 [details] [diff] [review]
Part 3: Give ErrorResult a move constructor and a move assignment operator
Attachment #8589085 - Attachment is obsolete: true
Attachment #8589431 - Flags: review?(bzbarsky)
Comment on attachment 8589430 [details] [diff] [review]
Part 1: Make it possible to send an ErrorResult that doesn't encode a JS exception through the IPDL layer

>+    MOZ_ASSERT_IF(aParam.mMightHaveUnreportedJSException, aParam.IsJSException());

This is backwards.  IsJSException() implies mMightHaveUnreportedJSException, but not vice versa.
Comment on attachment 8589431 [details] [diff] [review]
Part 3: Give ErrorResult a move constructor and a move assignment operator

> +    unused << js::AddRawValueRoot(cx, &mJSException, "ErrorResult::mJSException");

If it fails, please MOZ_CRASH.  Otherwise you'll end up with a dangling pointer to an unrooted gcthing (when it gets gced), which seems much worse than just safely crashing.

r=me with that fixed.
Attachment #8589431 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 24

4 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #19)
> Comment on attachment 8589088 [details] [diff] [review]
> Part 5: Do not store or match Response objects with a Vary:* header
> 
> Review of attachment 8589088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cache/DBSchema.cpp
> @@ +944,5 @@
> >      for (; token;
> >           token = nsCRT::strtok(rawBuffer, NS_HTTP_HEADER_SEPS, &rawBuffer)) {
> >        nsDependentCString header(token);
> >        if (header.EqualsLiteral("*")) {
> > +        varyHeadersMatch = false;
> 
> Hmm, which call to ToPCacheResponseWithoutBody?

The one triggered in Cache::Put().  Basically, you can't get to the DBSchema code without converting from Response to PCacheResponse.  So the ToPCacheResponseWithoutBody validation must have been executed before here.
Flags: needinfo?(bkelly)
(Assignee)

Comment 25

4 years ago
(In reply to Not doing reviews right now from comment #22)
> Comment on attachment 8589430 [details] [diff] [review]
> Part 1: Make it possible to send an ErrorResult that doesn't encode a JS
> exception through the IPDL layer
> 
> >+    MOZ_ASSERT_IF(aParam.mMightHaveUnreportedJSException, aParam.IsJSException());
> 
> This is backwards.  IsJSException() implies mMightHaveUnreportedJSException,
> but not vice versa.

D'oh!  Will fix, thanks for catching.
(Assignee)

Comment 27

4 years ago
I investigated this today but I still can't figure out what's going on here exactly.

But these three parts can land for now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fcc5890a770
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dee5173e641
https://hg.mozilla.org/integration/mozilla-inbound/rev/55fefc16edba
Keywords: leave-open
(Assignee)

Updated

4 years ago
Keywords: leave-open
(Assignee)

Comment 29

4 years ago
Created attachment 8590538 [details] [diff] [review]
Part 6: Hold a kungfu death grip in FetchPut::MaybeNotifyListener

This seems to be enough to fix the issues on the try server.
Attachment #8590538 - Flags: review?(bkelly)
(Reporter)

Updated

4 years ago
Attachment #8590538 - Flags: review?(bkelly) → review+
(Assignee)

Comment 30

4 years ago
So, the recent cache rewrite destroyed my patches beyond repair.  Someone needs to reimplement the fix from scratch.  Note that parts 1-3 have landed, but none of the cache specific code has landed yet.
Assignee: ehsan → nobody
(Assignee)

Comment 31

4 years ago
Created attachment 8592932 [details] [diff] [review]
Part 8: Do not attempt to delete ErrorResult::mMessage when deserializing the object from IPDL

In opt builds, we do not initialize mMessage, so the null check is going
to be ineffective to test whether mMessage has a value that we should
delete.  But in IPDL, the ParamTraits Read method is only called with
freshly allocated objects, so we don't need to free anything here.
In order to be safe and ensure that we don't leak this memory, this
patch adds a number of assertions to check whether mMessage is
indeed only initialized where we expect it to.
Attachment #8592932 - Flags: review?(bzbarsky)
Comment on attachment 8592932 [details] [diff] [review]
Part 8: Do not attempt to delete ErrorResult::mMessage when deserializing the object from IPDL

r=me

I wonder whether we can remove that init of mMessage now that we no longer support gcc 4.6.  Followup?
Attachment #8592932 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 33

4 years ago
Filed follow-up: bug 1154831
Assignee: nobody → ehsan
You need to log in before you can comment on or make changes to this bug.