Closed Bug 829858 Opened 7 years ago Closed 7 years ago

Null dereference in nsHttpChannel::InitOfflineCacheEntry()

Categories

(Core :: Networking: HTTP, defect, critical)

21 Branch
x86_64
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
firefox21 --- fixed
firefox-esr10 --- wontfix
firefox-esr17 --- fixed
b2g18 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

(Whiteboard: [qa-])

Crash Data

Attachments

(1 file)

Attached patch v1Splinter Review
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) !
Attachment #701390 - Flags: review?(jduell.mcbugs)
Attachment #701390 - Flags: approval-mozilla-b2g18?
Severity: normal → critical
Version: Trunk → 21 Branch
Attachment #701390 - Flags: review?(jduell.mcbugs) → review+
Duplicate of this bug: 830080
Comment on attachment 701390 [details] [diff] [review]
v1

b2g18+ - Please land today.
Attachment #701390 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
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?
(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?
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?
Milan, feel free to submit a patch.
Crash Signature: [@ mozilla::net::nsHttpChannel::InitOfflineCacheEntry()]
https://hg.mozilla.org/mozilla-central/rev/620f2cbe08be
Assignee: nobody → honzab.moz
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Attachment #701390 - Flags: approval-mozilla-release?
Attachment #701390 - Flags: approval-mozilla-beta?
The patch causing this bug is not on release channel.  And you are not asking for approval to release on it either.
Comment on attachment 701390 [details] [diff] [review]
v1

Thanks for pointing out my mistake, Honza. I corrected the approval request flags.
Attachment #701390 - Flags: approval-mozilla-release?
Attachment #701390 - Flags: approval-mozilla-esr17?
Attachment #701390 - Flags: approval-mozilla-esr10?
Attachment #701390 - Flags: approval-mozilla-aurora?
Comment on attachment 701390 [details] [diff] [review]
v1

Approving for aurora/beta to go along with bug 761040
Attachment #701390 - Flags: approval-mozilla-beta?
Attachment #701390 - Flags: approval-mozilla-beta+
Attachment #701390 - Flags: approval-mozilla-aurora?
Attachment #701390 - Flags: approval-mozilla-aurora+
(Also, this is in my queue to land on Aurora once it reopens)
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.
Attachment #701390 - Flags: approval-mozilla-esr17?
Attachment #701390 - Flags: approval-mozilla-esr17+
Attachment #701390 - Flags: approval-mozilla-esr10?
Attachment #701390 - Flags: approval-mozilla-esr10-
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
Is there something QA can do to verify this fix ?
Whiteboard: [qa?]
There is an automated test for bug 761040, there is nothing more to do to exercise the incriminated code.

qa-
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
Whiteboard: [qa?] → [qa-]
You need to log in before you can comment on or make changes to this bug.