1270328 - Use-after-forget in WebSocketChannel::ProcessInput

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug1270328-websocket-frame-hg0.diff

Comment 2

[Patrick McManus [:mcmanus]]

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.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0972ccf3d27a

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0972ccf3d27a