1149987 - Cache API should reject Responses with VARY:*

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Ehsan, do you have any interest in doing this one since you looked at the VARY header stuff previously?

Comment 2

[(no longer active)]

Sure.  I may get to it next week though.  Let me know if you need this sooner.

Comment 3

[(no longer active)]

Attachment: Part 1: Expose the ErrNum value for an ErrorResult, and make it serializable through IPC

Comment 4

[(no longer active)]

Attachment: 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.

Comment 5

[(no longer active)]

Attachment: Part 3: Give ErrorResult a move constructor and a move assignment operator

Comment 6

[(no longer active)]

Attachment: 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.

Comment 7

[(no longer active)]

Attachment: Part 5: Do not store or match Response objects with a Vary:* header

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8589084 [details] [diff] [review]
Part 2: Make ErrorResult unassignable

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[(no longer active)]

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

Comment 11

[Boris Zbarsky [: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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I think we could avoid the complexity of doing ErrorResult IPC serialization if we do bug 1120501 first.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 15

[(no longer active)]

Attachment: Part 1: Make it possible to send an ErrorResult that doesn't encode a JS exception through the IPDL layer

Comment 16

[(no longer active)]

Attachment: Part 1: Make it possible to send an ErrorResult that doesn't encode a JS exception through the IPDL layer

Comment 17

[Boris Zbarsky [: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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

[(no longer active)]

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!

Comment 20

[(no longer active)]

Attachment: Part 1: Make it possible to send an ErrorResult that doesn't encode a JS exception through the IPDL layer

This addresses comment 18.

Comment 21

[(no longer active)]

Attachment: Part 3: Give ErrorResult a move constructor and a move assignment operator

Comment 22

[Boris Zbarsky [: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 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 25

[(no longer active)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for ASAN u-a-f and similar crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fbf03927859

https://treeherder.mozilla.org/logviewer.html#?job_id=8598647&repo=mozilla-inbound

Comment 27

[(no longer active)]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7fcc5890a770
https://hg.mozilla.org/mozilla-central/rev/1dee5173e641
https://hg.mozilla.org/mozilla-central/rev/55fefc16edba

Comment 29

[(no longer active)]

Attachment: Part 6: Hold a kungfu death grip in FetchPut::MaybeNotifyListener

This seems to be enough to fix the issues on the try server.

Comment 30

[(no longer active)]

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.

Comment 31

[(no longer active)]

Attachment: 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.

Comment 32

[Boris Zbarsky [: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?

Comment 33

[(no longer active)]

Filed follow-up: bug 1154831

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0109a745ab9c
https://hg.mozilla.org/mozilla-central/rev/3f0d0d999510
https://hg.mozilla.org/mozilla-central/rev/df5a78250315
https://hg.mozilla.org/mozilla-central/rev/9d0a0318dd68
https://hg.mozilla.org/mozilla-central/rev/b50c0715615e

Comment 35

[Pulsebot]

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

Comment 36

[Carsten Book [:Tomcat]]

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