Closed Bug 1020379 Opened 6 years ago Closed 6 years ago

[RTSP] Potential infinite loop and compile warnings in ARTPConnection.cpp

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S4 (20june)

People

(Reporter: ethan, Assigned: ethan)

Details

(Whiteboard: [p=1])

Attachments

(2 files, 2 obsolete files)

We found a logical error from this changeset.
https://hg.mozilla.org/mozilla-central/rev/7770095ccedf

In ARTPConnection.cpp (line 248):
for (uint16_t port = start; port < 65536; port += 2) {
    ...
}

The for-loop local variable "port" was changed from "unsigned" to "uint16_t" carelessly. This change made the end condition being always true, which means this for-loop could be potentially infinite.
Although it's almost impossible to run into this logical error (e.g. all the UDP sockets are occupied), we should fix it to avoid the potential risk.

In the meanwhile, I found several compile warnings around this changeset. I will fix them as well in this bug.
Assignee: nobody → ettseng
Status: NEW → ASSIGNED
Whiteboard: [p=1]
Target Milestone: --- → 2.0 S4 (20june)
Summary: [RTSP] Follow-up of 996535 - Some logical error and compile warnings → [RTSP] Follow-up of 996535 - Logical error and compile warnings in ARTPConnection.cpp
Summary: [RTSP] Follow-up of 996535 - Logical error and compile warnings in ARTPConnection.cpp → [RTSP] Follow-up of 996535 - Potential infinite loop and compile warnings in ARTPConnection.cpp
Summary: [RTSP] Follow-up of 996535 - Potential infinite loop and compile warnings in ARTPConnection.cpp → [RTSP] Potential infinite loop and compile warnings in ARTPConnection.cpp
Attached patch Fix potential infinite loop (obsolete) — Splinter Review
Fixed the issue mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1018288#c2.
Attachment #8435557 - Flags: review?(sworkman)
Attached patch Fix compile warnings around RTSP (obsolete) — Splinter Review
This patch fixes the following warnings:
- ARTPConnection.cpp:284: warning: comparison between signed and unsigned integer expressions

- ARTSPConnection.h:91: warning: 'android::ARTSPConnection::mSocket' will be initialized after
  ARTSPConnection.h:88: warning: 'int32_t android::ARTSPConnection::mConnectionID'
  ARTSPConnection.cpp:54: warning: when initialized here RTSPSource.o

- RTSPConnectionHandler.h:44:1: warning: "LOGI" redefined
Attachment #8435559 - Flags: review?(sworkman)
Attachment #8435557 - Attachment description: fix-infinite-loop.patch → Fix potential infinite loop
Attachment #8435557 - Flags: review?(sworkman) → review+
Attachment #8435559 - Flags: review?(sworkman) → review+
Thanks for taking care of this Ethan!
(In reply to Steve Workman [:sworkman] from comment #3)
> Thanks for taking care of this Ethan!
You are welcome, Steve.
I am glad I can help!
The result of Try server looks good.
https://tbpl.mozilla.org/?tree=Try&rev=ca63435dae22

Request to check in both patches.
Keywords: checkin-needed
Refresh commit message "r=sworkman".
Attachment #8435557 - Attachment is obsolete: true
Refresh commit message "r=sworkman".
Attachment #8435559 - Attachment is obsolete: true
Sorry I forgot to refresh commit messages before requesting to check in.
Please check in using the updated patches: attachment 8435994 [details] [diff] [review] and 8435996.
You need to log in before you can comment on or make changes to this bug.