Cherry-pick upstream fix #8
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were using the address of the SctpTransport object as the sconn_addr field in usrsctp, which is used to get access to the SctpTransport object in various callbacks. However, this address is sent in the clear in the SCTP cookie, which is undesirable. This change uses a monotonically increasing id instead, which is mapped back to a SctpTransport using a SctpTransportMap helper class. Bug: chromium:1076703 Change-Id: Iffb23fdbfa13625e921a9fd5500fe772b4d4015f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176422 Reviewed-by: Harald Alvestrand <hta@webrtc.org> Reviewed-by: Tommi <tommi@webrtc.org> Commit-Queue: Taylor <deadbeef@webrtc.org> Cr-Commit-Position: refs/heads/master@{#31449}
cbrune-signal
approved these changes
Jun 19, 2020
There was a problem hiding this comment.
I reviewed the original context patch and this PR.
The functionality of the original patch seems good. I search for other uses of this and found no problems with that patch.
Then I compared this PR here to the original -- no problems found with the application of the patch.
Looks good. 👍
peter-signal
pushed a commit
that referenced
this pull request
Jul 18, 2020
…excessive rounding (cherry picked from commit 2899b3b) Bug: webrtc:11652 Change-Id: I8bfa91c3115d6ebb17beefbb2a5e51efbbd599e0 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177000 Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org> Reviewed-by: Erik Språng <sprang@webrtc.org> Cr-Original-Commit-Position: refs/heads/master@{#31502} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177105 Cr-Commit-Position: refs/branch-heads/4147@{#8} Cr-Branched-From: 2b7d969-refs/heads/master@{#31262}
peter-signal
pushed a commit
that referenced
this pull request
Apr 16, 2021
Loosen the restrictions for ice-char by allowing '-' and '='. Being spec-compliant breaks interoperability. The spec-behaviour will be restored with a notice period. BUG=chromium:1053756,chromium:1044521 (cherry picked from commit 48e849f) No-Try: True Change-Id: I880babd0869302bd713912ddfcfa48866fad32c1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168820 Commit-Queue: Steve Anton <steveanton@webrtc.org> Reviewed-by: Steve Anton <steveanton@webrtc.org> Reviewed-by: Harald Alvestrand <hta@webrtc.org> Cr-Original-Commit-Position: refs/heads/master@{#30560} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/169663 Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org> Cr-Commit-Position: refs/branch-heads/4044@{#8} Cr-Branched-From: be99ee8-refs/heads/master@{#30432}
rashad-signal
pushed a commit
that referenced
this pull request
Jan 3, 2023
This converts all P2PTransportChannel unit tests to parameterized tests, with a string parameter for the field_trials which is used to enable the refactor. This adds a variation of each existing test using the refactored code path. Tests are initialized twice, once for legacy and refactored path each, to strike a balance between file name length and descriptiveness. Bug: webrtc:14367, webrtc:14131 Change-Id: I0469550d571ed389804eb486fe5bd22504e59373 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275303 Commit-Queue: Sameer Vijaykar <samvi@google.com> Reviewed-by: Jonas Oreland <jonaso@webrtc.org> Cr-Commit-Position: refs/heads/main@{#38162}
jim-signal
pushed a commit
that referenced
this pull request
Jan 17, 2024
This was a fun bug which proved to be challenging to find a good solution for. The issue comes from the combination of partial reliability and stream resetting, which are covered in different RFCs, and where they don't refer to each other... Stream resetting (RFC 6525) is used in WebRTC for closing a Data Channel, and is done by signaling to the receiver that the stream sequence number (SSN) should be set to zero (0) at some time. Partial reliability (RFC 3758) - and expiring messages that will not be retransmitted - is done by signaling that the SSN should be set to a certain value at a certain TSN, as the messages up until the provided SSN are not to be expected to be sent again. As these two functionalities both work by signaling to the receiver what the next expected SSN should be, they need to do it correctly not to overwrite each others' intent. And here was the bug. An example scenario where this caused issues, where we are Z (the receiver), getting packets from the sender (A): 5 A->Z DATA (TSN=30, B, SID=2, SSN=0) 6 Z->A SACK (Ack=30) 7 A->Z DATA (TSN=31, E, SID=2, SSN=0) 8 A->Z RE_CONFIG (REQ=30, TSN=31, SID=2) 9 Z->A RE_CONFIG (RESP=30, Performed) 10 Z->A SACK (Ack=31) 11 A->Z DATA (TSN=32, SID=1) 12 A->Z FORWARD_TSN (TSN=32, SID=2, SSN=0) Let's assume that the path Z->A had packet loss and A never really received our responses (#6, #9, #10) in time. At #5, Z receives a DATA fragment, which it acks, and at #7 the end of that message. The stream is then reset (#8) which it signals that it was performed (#9) and acked (#10), and data on another stream (2) was received (#11). Since A hasn't received any ACKS yet, and those chunks on SID=2 all expired, A sends a FORWARD-TSN saying that "Skip to TSN=32, and don't expect SID=2, SSN=0". That makes the receiver expect the SSN on SID=2 to be SSN=1 next time at TSN=32. But that's not good at all - A reset the stream at #8 and will want to send the next message on SID=2 using SSN=0 - not 1. The FORWARD-TSN clearly can't have a TSN that is beyond the stream reset TSN for that stream. This is just one example - combining stream resetting and partial reliability, together with a lossy network, and different variants of this can occur, which results in the receiver possibly not delivering packets because it expects a different SSN than the one the sender is later using. So this CL adds "breakpoints" to how far a FORWARD-TSN can stretch. It will simply not cross any Stream Reset last assigned TSNs, and only when a receiver has acked that all TSNs up till the Stream Reset last assigned TSN has been received, it will proceed expiring chunks after that. Bug: webrtc:14600 Change-Id: Ibae8c9308f5dfe8d734377d42cce653e69e95731 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/321600 Commit-Queue: Victor Boivie <boivie@webrtc.org> Reviewed-by: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#40829}
jim-signal
pushed a commit
that referenced
this pull request
Sep 5, 2024
The new version of MSan (rolled by [1]) detects the following:
```
==39908==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x5591400a52ef in GetPlayoutDelayMs ./../../modules/audio_coding/neteq/decision_logic.cc:466:35
#1 0x5591400a52ef in webrtc::DecisionLogic::ExpectedPacketAvailable(webrtc::NetEqController::NetEqStatus) ./../../modules/audio_coding/neteq/decision_logic.cc:311:36
#2 0x5591400a39e9 in webrtc::DecisionLogic::GetDecision(webrtc::NetEqController::NetEqStatus const&, bool*) ./../../modules/audio_coding/neteq/decision_logic.cc:0:0
#3 0x55913cf590c9 in webrtc::DecisionLogicTest_PreemptiveExpand_Test::TestBody() ./../../modules/audio_coding/neteq/decision_logic_unittest.cc:139:3
#4 0x55913ef28283 in HandleExceptionsInMethodIfSupported<testing::Test, void> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:3
#5 0x55913ef28283 in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2710:5
#6 0x55913ef2ab46 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2856:11
#7 0x55913ef2da34 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3034:30
#8 0x55913ef621e8 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5964:44
#9 0x55913ef60f54 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0
#10 0x55913ef60f54 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5543:10
#11 0x55913ee1a944 in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2334:73
#12 0x55913ee1a944 in webrtc::(anonymous namespace)::TestMainImpl::Run(int, char**) ./../../test/test_main_lib.cc:203:21
#13 0x55913cbd36b8 in main ./../../test/test_main.cc:72:16
#14 0x7fdb18c73082 in __libc_start_main /build/glibc-LcI20x/glibc-2.31/csu/../csu/libc-start.c:308:16
#15 0x55913cb3e1a9 in _start ??:0:0
```
[1] - https://webrtc-review.googlesource.com/c/src/+/353620
Bug: b/344970813
Change-Id: I9b5d7791e68b4c494168ba9f007a3099ae21fed4
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/353581
Auto-Submit: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Jakob Ivarsson <jakobi@webrtc.org>
Commit-Queue: Jakob Ivarsson <jakobi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42433}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.