Closed Bug 996688 Opened 6 years ago Closed 6 years ago

[RTSP] Prevent potential crash in RtspConnectionHandler::parsePlayResponse

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
1.4 S6 (25apr)
tracking-b2g backlog

People

(Reporter: ethan, Assigned: ethan)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file, 1 obsolete file)

There is a potential crash in RtspConnectionHandler::parsePlayResponse.
Though it doesn't happen in a normal condition, the risk does exist.

The crash happens when a "Play" operation is triggered from the media resource, and then a "Stop" is followed in a very short time.
The sequence of "Play":
RtspController::Play() -> RTSPSource::play() -> RTSPSource::performPlay() -> RtspConnectionHandler::play()

The sequence of "Stop":
RtspController::Stop() -> RTSPSource::stop() -> RTSPSource::finishDisconnectIfPossible() -> RtspConnectionHandler::disconnect()

The problem is, RtspConnectionHandler::play() sends an RTSP PLAY request to the server and will call parsePlayResponse() when the response from the server is received. Of course there is a latency between sending PLAY request and parsePlayResponse().
If RtspConnectionHandler::disconnect() is called between them, it will clean up a lot of stuff and cause parsePlayResponse() crash somewhere, such as an assertion.

This bug is aimed to avoid such crash.

p.s. This crash is found when we work on bug 992568.
If an RTSP web page is suspended (for example, by tapping the URL bar) first, and then the user navigate away the page, RtspMediaResource::Resume() and RtspMediaResource::Close() would happen successively in a very short time.
Assignee: nobody → ettseng
Blocks: 992568
Hi Steve,
I found this crash while working on bug 992568 (refactoring RtspChannel); nevertheless, I think this is an independent issue from bug 992568. Because from the point of view of RtspConnectionHandler, it's not guaranteed disconnect() will never be called between play() and parsePlayResponse().
So I filed this bug.

So far my idea is simply to add a variable, e.g. mAborted, and to check this value before parsing the play response. It will be a quick (and hope not so dirty) solution.
How do you think? Do you have any suggestion?
Flags: needinfo?(sworkman)
Attached patch bug-996688-wip.patch (obsolete) — Splinter Review
A WIP patch to demonstrate my idea of solution.
*** Note ***
If the proposed solution is reasonable, I will refine the patch to:
1. Set mAborted to false wherever needed, e.g. in the case of reconnect.
2. Consider the necessity to do the same mAborted check in other places.
Status: NEW → ASSIGNED
Comment on attachment 8406955 [details] [diff] [review]
bug-996688-wip.patch

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

This seems fine to me. But maybe we should check in more places?

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +714,5 @@
>                       result, strerror(-result));
> +                if (mAborted) {
> +                  LOGV("we're aborted, dropping stale packet.");
> +                  break;
> +                }

Would it be better to put this at the top of the function? In this way, any message received after 'abor' would be dropped.
Attachment #8406955 - Flags: feedback+
(In reply to Ethan Tseng [:ethan] from comment #3)
> *** Note ***
> If the proposed solution is reasonable, I will refine the patch to:
> 1. Set mAborted to false wherever needed, e.g. in the case of reconnect.
> 2. Consider the necessity to do the same mAborted check in other places.

Oh, I missed this comment when I was reviewing the patch. Good ideas ;)
Flags: needinfo?(sworkman)
blocking-b2g: --- → backlog
(In reply to Steve Workman [:sworkman] from comment #4)
> Would it be better to put this at the top of the function? In this way, any
> message received after 'abor' would be dropped.
Negative.
The 'abor' message will trigger a sequence messages to complete the disconnection process.
'abor'  -> 'tear' -> 'disc' -> 'quit'
Addressed the issues in comment 3.
1. Set mAborted to false in 'conn', which will happen not only in the first time but also in the case of reconnect (for try TCP interleaving).
2. Beside 'play', add mAborted checks in the following cases since they all involve with parsing response packets:
   'desc', 'setu', 'accu', 'see2'
Attachment #8406955 - Attachment is obsolete: true
Attachment #8407290 - Flags: review?(sworkman)
Attachment #8407290 - Flags: review?(sworkman) → review+
Steve, thanks for you review. :)
Whiteboard: [p=1]
Target Milestone: --- → 1.4 S6 (25apr)
Duplicate of this bug: 964911
https://hg.mozilla.org/mozilla-central/rev/ca90cc514b09
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.