Closed Bug 1198730 Opened 5 years ago Closed 5 years ago

DataChannel PMTUD disable clears other flags by accident

Categories

(Core :: WebRTC: Networking, defect, P1)

Product:
Core
Component:
WebRTC: Networking
Version:
42 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox40 --- unaffected
firefox41 + fixed
firefox42 --- fixed
firefox43 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1320439])

Attachments

(1 file)

fix simple bug in PMTUD disable that clears all other flags
1.50 KB, patch
tuexen
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #1194817 +++

From bug 1194817 comment 11:
> Coverity complains about this patch:
> CID 1320439 (#1 of 1): Logical vs. bitwise operator
> (CONSTANT_EXPRESSION_RESULT)logical_vs_bitwise: paddrparams.spp_flags &= 0U /* !8 */ always assigns 0 to paddrparams.spp_flags.
>    Did you intend to use '~' rather than '!'?

yes, it should be ~
backlog: --- → webrtc/webaudio+
Rank: 10
Keywords: coverity
Whiteboard: [CID 1320439]
Attachment #8652836 - Flags: review?(tuexen) → review+
Summary: Certain high-traffic uses of DataChannels/sctp fail when the packets get too large due to PMTU → DataChannel PMTUD disable clears other flags by accident
Wow, that was fast.  :-)
Sitting in a meeting...
Tracked for 41 to make sure we don't forget the uplift (but knowing Randell, I know this won't be necessary)
tracking-firefox41: --- → +
Comment on attachment 8652836 [details] [diff] [review]
fix simple bug in PMTUD disable that clears all other flags

Approval Request Comment
[Feature/regressing bug #]: bug 1194817

[User impact if declined]: Kills the heartbeat, which catches (eventually) link failure.  Lack of heartbeat will not cause major loss of functionality, however.

[Describe test coverage new/current, TreeHerder]: DataChannels are hit in testing

[Risks and why]: virtually no risk
Note: there's an upstream bug with the spp flags reported by sctp_getopt() that could cause problem if we *later* did getopt/setopt on a different flag; however we don't do that (confirmed with GDB), so confirmed working and safe in operation.

[String/UUID change made/needed]: none
Attachment #8652836 - Flags: approval-mozilla-beta?
Attachment #8652836 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/dda56b1c5210
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8652836 [details] [diff] [review]
fix simple bug in PMTUD disable that clears all other flags

Safe to uplift to Beta and Aurora. Nice to this was caught by Coverity's static code analysis.
Attachment #8652836 - Flags: approval-mozilla-beta?
Attachment #8652836 - Flags: approval-mozilla-beta+
Attachment #8652836 - Flags: approval-mozilla-aurora?
Attachment #8652836 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.