829858 - Null dereference in nsHttpChannel::InitOfflineCacheEntry()

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: v1

1.11      if (!mResponseHead || mResponseHead->NoStore()) {
    1.12 +        if (mResponseHead->NoStore()) {
    1.13 +            mOfflineCacheEntry->AsyncDoom(nullptr);
    1.14 +        }
    1.15 +
    1.16          CloseOfflineCacheEntry();
    1.17  
    1.18 +        if (mResponseHead->NoStore()) {
    1.19 +            return NS_ERROR_NOT_AVAILABLE;
    1.20 +        }
    1.21 +
    1.22          return NS_OK;
    1.23      }

Clear as the day.

It's not the first time I'm seeing a serious issue went in via a tiny patch.  The smaller, the more dangerous how people don't spot the problem (both devs and reviewers) !

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 701390 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/620f2cbe08be

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 701390 [details] [diff] [review]
v1

b2g18+ - Please land today.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

The logic of the final code looks really strange, it's at least worth a comment, if it is meant to do what it's doing.

  3612     if (!mResponseHead || mResponseHead->NoStore()) {
  3613         if (mResponseHead && mResponseHead->NoStore()) {
  3614             mOfflineCacheEntry->AsyncDoom(nullptr);
  3615         }
  3616 
  3617         CloseOfflineCacheEntry();
  3618 
  3619         if (mResponseHead && mResponseHead->NoStore()) {
  3620             return NS_ERROR_NOT_AVAILABLE;
  3621         }
  3622 
  3623         return NS_OK;
  3624     }

if mResponseHead is null, we will go straight to CloseOfflineCacheEntry call at 3617.  If it is not null, we will call mResponseHead->NoStore(), and if that returns true, we will call the same function again, then proceed with the CloseOfflineCacheEntry() on 3617.

Is this what's meant to happen?  It's probably worth at least a comment in the code if the intent is to call mResponseHead->NoStore() twice in a row?

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 701390 [details] [diff] [review]
v1

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1b11cc982011

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Milan Sreckovic [:milan] from comment #4)
> The logic of the final code looks really strange, it's at least worth a
> comment, if it is meant to do what it's doing.
> 
>   3612     if (!mResponseHead || mResponseHead->NoStore()) {
>   3613         if (mResponseHead && mResponseHead->NoStore()) {
>   3614             mOfflineCacheEntry->AsyncDoom(nullptr);
>   3615         }
>   3616 
>   3617         CloseOfflineCacheEntry();
>   3618 
>   3619         if (mResponseHead && mResponseHead->NoStore()) {
>   3620             return NS_ERROR_NOT_AVAILABLE;
>   3621         }
>   3622 
>   3623         return NS_OK;
>   3624     }
> 
> if mResponseHead is null, we will go straight to CloseOfflineCacheEntry call
> at 3617.  If it is not null, we will call mResponseHead->NoStore(), and if
> that returns true, we will call the same function again, then proceed with
> the CloseOfflineCacheEntry() on 3617.
> 
> Is this what's meant to happen?  It's probably worth at least a comment in
> the code if the intent is to call mResponseHead->NoStore() twice in a row?

I don't understand the concern here.  You mean we are calling that small inline function that will probably be optimized by the compiler too often?

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Concern is a strong word for it, I'm just nit picking. It is probably not worth the time we're spending on this exchange, but let me give you the devil's advocate.  From the point of view of somebody reading the code and trying to understand what the author wanted, this is a bit annoying.  Is the intent to call NoStore() twice?  Does it make a difference? The name "NoStore" doesn't give me a clue if it's an action "don't store" or a test "no need to store", so I now have to go to the definition of NoStore and double check what it does, if it has side-effects, what it means. It is not a bad thing for someone to have to do that, but it seems that reading the code requires more thought than it should.  Doing the pointer and NoStore() check again after CloseOfflineCacheEntry() suggests that mResponseHead could be set or the result of NoStore() could change.  That's probably worth a comment?

Comment 8

[Honza Bambas (:mayhemer)]

Milan, feel free to submit a patch.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/620f2cbe08be

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 701390 [details] [diff] [review]
v1

[Approval Request Comment] 
User impact if declined: Necessary for bug 761040 to be uplifted.
Testing completed (on m-c, etc.): on m-c as of today.
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: none.

Comment 11

[Honza Bambas (:mayhemer)]

The patch causing this bug is not on release channel.  And you are not asking for approval to release on it either.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 701390 [details] [diff] [review]
v1

Thanks for pointing out my mistake, Honza. I corrected the approval request flags.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 701390 [details] [diff] [review]
v1

Approving for aurora/beta to go along with bug 761040

Comment 14

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-beta/rev/542a20ea311c

Comment 15

[(Out Sep 17-23) Ryan VanderMeulen]

(Also, this is in my queue to land on Aurora once it reopens)

Comment 16

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/57862e458e63

Comment 17

[bhavana bajaj [:bajaj]]

Comment on attachment 701390 [details] [diff] [review]
v1


Approving for esr17 as bug 761040 was recently approved and the patch in this bug blocks bug 761040 .And since we will be updating our esr10 users to esr17 in the upcoming release, not approving on esr10.

Comment 18

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-esr17/rev/6d88fc08e25a

Comment 19

[(Out Sep 17-23) Ryan VanderMeulen]

Backed out along with bug 761040 for build bustage. Leaving the approval set for now since the bustage was caused by the other cset, not this one.
https://hg.mozilla.org/releases/mozilla-esr17/rev/cf989dcd7867

Comment 20

[Paul Silaghi, QA [:pauly]]

Is there something QA can do to verify this fix ?

Comment 21

[Honza Bambas (:mayhemer)]

There is an automated test for bug 761040, there is nothing more to do to exercise the incriminated code.

qa-

Comment 22

[(Out Sep 17-23) Ryan VanderMeulen]

Re-landed on esr17 (taking into account that some of the code changed here was removed in the re-landed esr17 patch).
https://hg.mozilla.org/releases/mozilla-esr17/rev/20f9b4b5b4d0

Comment 23

[(Out Sep 17-23) Ryan VanderMeulen]

Backed out again due to problems with bug 761040.
https://hg.mozilla.org/releases/mozilla-esr17/rev/a886fbd0a66c

Comment 24

[(Out Sep 17-23) Ryan VanderMeulen]

Rolled up with bug 761040.
https://hg.mozilla.org/releases/mozilla-esr17/rev/6523fbbd93e5

Comment 25

[(Out Sep 17-23) Ryan VanderMeulen]

Backed out again for xpcshell failures. See bug 761040.
https://hg.mozilla.org/releases/mozilla-esr17/rev/6523fbbd93e5

Comment 26

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-esr17/rev/7682348c875d