Closed Bug 1270328 Opened 5 years ago Closed 5 years ago

Use-after-forget in WebSocketChannel::ProcessInput

Categories

(Core :: Networking: WebSockets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

The static analysis in bug 1186706 points out that WebSocketChannel::ProcessInput can use the value of a RefPtr after forget() is called on it, because of this:

   1757         if (frame) {
   1758           // We send the frame immediately becuase we want to have it dispatched
   1759           // before the CallOnServerClose.
   1760           mService->FrameReceived(mSerial, mInnerWindowID, frame.forget());
   1761         }

And this:

   1798       if (frame) {
   1799         mService->FrameReceived(mSerial, mInnerWindowID, frame.forget());
   1800       }

This looks harmless in practice because the first forget() sets the pointer to null, but the analysis treats it like Move(), as being invalid to access — and I tend to agree that it would be clearer to have an explicit `frame = nullptr;` there if something is going to depend on that.
Comment on attachment 8748974 [details] [diff] [review]
bug1270328-websocket-frame-hg0.diff

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

any chance of teaching the analysis tool the semantics of forget? I doubt this is the only place.
Attachment #8748974 - Flags: review?(mcmanus) → review+
Whiteboard: [necko-active]
(In reply to Patrick McManus [:mcmanus] from comment #2)
> any chance of teaching the analysis tool the semantics of forget? I doubt
> this is the only place.

It seems to be the only place — there were two regressions that turned up when I tried to dust off bug 1186706 to test bug 1191452, and the other one looks like it actually would dereference a null pointer (but only if the right non-default logging option is set).
https://hg.mozilla.org/mozilla-central/rev/0972ccf3d27a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.