Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update SCTP version that matches Chromium M83 #9

Merged
merged 1 commit into from
Jun 22, 2020
Merged

Update SCTP version that matches Chromium M83 #9

merged 1 commit into from
Jun 22, 2020

Conversation

peter-signal
Copy link
Contributor

No description provided.

jim-signal
jim-signal approved these changes Jun 22, 2020
View reviewed changes
Copy link
Contributor

@jim-signal jim-signal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@peter-signal peter-signal merged commit 257a77e into signalapp:master Jun 22, 2020
peter-signal pushed a commit that referenced this pull request Jul 18, 2020
Merge to M84: Prevent pointer from being sent in the clear over SCTP.
963cc1e 
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.

TBR=hta@webrtc.org

Bug: chromium:1076703
Change-Id: I5c6a44801293e3b0aacd032f16f41802f4fecf6d
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-Original-Commit-Position: refs/heads/master@{#31449}
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/177330
Reviewed-by: Taylor <deadbeef@webrtc.org>
Cr-Commit-Position: refs/branch-heads/4147@{#9}
Cr-Branched-From: 2b7d969-refs/heads/master@{#31262}
peter-signal pushed a commit that referenced this pull request Apr 16, 2021
@fippo
[Merge to M81] loosen ice-ufrag/ice-pwd ice-char restrictions further
601eb2d 
Loosen the restrictions for ice-char by also allowing
'#' (known to break) and '_' (urlsafe base64) in addition
to the existing exceptions for '-' and '='.
Also fixes typo in log message.

BUG=chromium:1053756

(cherry picked from commit 98d5bbb)

Change-Id: I8f254a7c25f780276452fa3e27245b6b7ad1a3ce
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168943
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Cr-Original-Commit-Position: refs/heads/master@{#30596}
No-Try: True
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/169664
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/branch-heads/4044@{#9}
Cr-Branched-From: be99ee8-refs/heads/master@{#30432}
jim-signal pushed a commit that referenced this pull request Jan 17, 2024
dcsctp: Don't generate FORWARD-TSN across stream resets
a93f581 
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
@MirkoBonadei
Fix use-of-uninitialized-value in NetEq tests.
876d0c9 
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
Reviewers

@jim-signal jim-signal jim-signal approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@peter-signal @jim-signal