Commit graph

53 commits

Author SHA1 Message Date
Danil Chapovalov
46cc32d89f Replace ABSL_FALLTHROUGH_INTENDED with c++17 attribute
the new spelling is more standard and more compact, in particular doesn't need extra include and thus dependency

Bug: None
Change-Id: Iaea69d2154e4d9eff2468514f5734cb3fe016ff8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/245080
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35709}
2022-01-17 14:55:02 +00:00
Victor Boivie
4b7024b572 Revert "dcsctp: Use rtc::CopyOnWriteBuffer"
This reverts commit 2db59a6584.

Reason for revert: Causes msan-issue in crc32c, reading uninitialized
memory.

Bug: webrtc:12943, chromium:1275559
Change-Id: I05f1012d896aeaca86c4562e0df15fa7ea326d60
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/239560
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35461}
2021-12-02 12:33:46 +00:00
Victor Boivie
2db59a6584 dcsctp: Use rtc::CopyOnWriteBuffer
This avoids copying the payload at all. Future CL will change the
transport.

In performance tests, memcpy was visible in the performance profiles
prior to this change.

Bug: webrtc:12943
Change-Id: I507a1a316165db748e73cf0d58c1be62cc76a2d2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/236346
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35428}
2021-11-29 11:53:19 +00:00
Victor Boivie
ec19d5ea79 dcsctp: Don't run network tests with TSAN or MSAN
Networks tests were previously disabled if building in debug mode as
debug mode adds DCHECKs, and when DCHECKs are enabled, a lot of the
components in dcSCTP will add consistency checks, and they can be really
expensive to run in these network tests.

However, if running in with TSAN or MSAN sanitizers and with DCHECKs
enabled, they also take a long time.

Current run-time on my relatively fast CPU (with is_debug=false):

(no sanitizer) always_dcheck=false: 2.5s
(no sanitizer) always_dcheck=true: 31s
is_tsan=true, always_dcheck=false: 53s
is_tsan=true, always_dcheck=true: 5m50s <-- too slow
is_asan=true, always_dcheck=false: 13s
is_asan=true, always_dcheck=true: 47s
is_msan=true, always_dcheck=false: 35s
is_msan=true, always_dcheck=true: 1m53s <-- too slow

Note that buildbots may be much slower than my computer.

Bug: webrtc:12943
Change-Id: If044ee9936372d54c9899b4864156c9f680af0b6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/236581
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35276}
2021-10-28 15:01:40 +00:00
Victor Boivie
3179fb2931 dcsctp: Avoid integer overflow in HEARTBEAT-ACK v2
This is a follow-up to change 232904 that also validates that the
timestamp from the heartbeat ack isn't negative (which the fuzzer
managed to set it to).

Bug: chromium:1252515
Change-Id: Idaac570589dbdaaee67b7785f6232b60226e88e1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/234582
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35168}
2021-10-08 11:06:11 +00:00
Victor Boivie
5755f3edaf dcsctp: Add sequence checker to socket
The DcSctpSocket is not thread safe and must be called from a single
thread or from a task queue that serializes access to it. This is now
validated at run-time in debug builds.

Bug: None
Change-Id: I3ed816924c20f6ed7e84a3273bee5a3f8f74112b
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/233420
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35127}
2021-09-30 11:25:18 +00:00
Victor Boivie
82ccdd36aa dcsctp: Add network/throughput tests
This initial version contains a few tests, testing both lossy, non-lossy
and bandwidth limited networks. It uses simulated time, and runs much
faster than wall time on release builds, but slower on debug when there
is a lot of outstanding data (high throughput) as there are consistency
checks on outstanding data. Because of that, some tests had to be
disabled in debug build.

Bug: webrtc:12943
Change-Id: I9323f2dc99bca4e40558d05a283e99ce7dded7f1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/225201
Reviewed-by: Artem Titov <titovartem@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35124}
2021-09-30 07:59:58 +00:00
Victor Boivie
15a0c880cf dcsctp: Ensure callbacks are always triggered
The previous manual way of triggering the deferred callbacks was very
error-prone, and this was also forgotten at a few places.

We can do better.

Using the RAII programming idiom, the callbacks are now ensured to be
called before returning from public methods.

Also added additional debug checks to ensure that there is a
ScopedDeferrer active whenever callbacks are deferred.

Bug: webrtc:13217
Change-Id: I16a8343b52c00fb30acb018d3846acd0a64318e0
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/233242
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35117}
2021-09-29 08:17:27 +00:00
Victor Boivie
0081f1c331 dcsctp: Refactor CallbackDeferrer
Moving the implementation to a source file.

Bug: webrtc:13217
Change-Id: I2929f4af96a9d01d3adfa49b36b021e4b229a025
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/233241
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35116}
2021-09-29 07:13:57 +00:00
Victor Boivie
f4fa166cc5 dcsctp: Detect the peer SCTP implementation
It's to be used for clients to record metrics and to e.g. attribute
metrics to which SCTP implementation the peer was using.

This is not explicitly signaled, so heuristics are used. These are not
guaranteed to come to the correct conclusion, and the data is not always
available.

Note: The behavior of dcSCTP will not change depending on the assumed
implementation - only by explicitly signaled capabilities.

Bug: webrtc:13216
Change-Id: I2f58054d17d53d947ed5845df7a08f974d42f918
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/233100
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35103}
2021-09-28 05:10:45 +00:00
Victor Boivie
f75f9c2b29 dcsctp: Avoid integer overflow in HEARTBEAT-ACK
When a HEARTBEAT is sent out, the current timestamp is stored in the
parameter that will be returned by the HEARTBEAT-ACK. If the timestamp
from the HEARTBEAT-ACK would be from the future (either by jumping
clocks, bit errors, exploiting peer or a fuzzer), the measured RTT would
become really large, and when it was calculated, it would result in an
integer overflow; a wrapping subtraction.

This isn't a problem, as RetransmissionTimeout::ObserveRTT method would
discard measurements that were negative or too large, but it would
trigger an UBSAN violation.

Adding an additional check so that the subtraction doesn't ever wrap.

Bug: chromium:1252515
Change-Id: I1f97b1e773a4639b8193a528716ebd6a27fcb740
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232904
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35095}
2021-09-27 10:52:46 +00:00
Sergey Sukhanov
72435325c6 dcsctp: hand over RRSendQueue streams state
Bug: webrtc:13154
Change-Id: I560b59ad2f5bcd2deafc3a37e3af853108b572b2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232605
Commit-Queue: Sergey Sukhanov <sergeysu@webrtc.org>
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35053}
2021-09-21 12:59:46 +00:00
Sergey Sukhanov
9d2b3cb595 dcsctp: enabled handover in test SendManyFragmentedMessagesWithLimitedRtx
Bug: webrtc:13154
Change-Id: I192d5093de9b7596208d44f4868b413602db473a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232543
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Commit-Queue: Sergey Sukhanov <sergeysu@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35052}
2021-09-21 11:55:43 +00:00
Sergey Sukhanov
d92f8a86b3 dcsctp: move HandoverReadinessStatus::ToString definition to build target public:socket
This allows build targets that need only HandoverReadinessStatus
to depend only on public:socket, and not on socket:dcsctp_socket.

Bug: webrtc:13154
Change-Id: I29f41910cdb5baed96b57fd7284b96fc50a56ba4
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232331
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Commit-Queue: Sergey Sukhanov <sergeysu@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35037}
2021-09-20 12:25:15 +00:00
Sergey Sukhanov
3852698ad9 dcsctp: support handover state serialization testing
dcSCTP library users can set their custom
g_handover_state_transformer_for_test that can serialize and
deserialize the state. All dcSCTP handover tests call
g_handover_state_transformer_for_test. If some part of the state is
serialized incorrectly or is forgotten, high chance that it will
fail the tests.

Bug: webrtc:13154
Change-Id: I251a099be04dda7611e9df868d36e3a76dc7d1e1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232325
Commit-Queue: Sergey Sukhanov <sergeysu@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35035}
2021-09-20 10:08:58 +00:00
Sergey Sukhanov
4397281f38 dcsctp: implement socket handover in the DcSctpSocket class and expose the functionality in the API
Bug: webrtc:13154
Change-Id: Idf4f4028c8e65943cb6b41fab0baef1b3584205d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232126
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Commit-Queue: Sergey Sukhanov <sergeysu@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35029}
2021-09-17 15:19:01 +00:00
Victor Boivie
54e4e35c89 dcsctp: Add consistency check for assembled msgs
The buffer of reassembled messages in ReassemblyQueue is only to be
used while processing a DATA/I-DATA or FORWARD-TSN as processing these
chunks may result in assembling messages.

When the socket is idle - between API calls - it's supposed to be empty.

Instead of having it as a member in ReassemblyQueue, it could be
provided as an argument to ReassemblyQueue::Add and
ReassemblyQueue::Handle(ForwardTSN), but that would be a quite big
refactoring. That will be investigated separately.

Bug: None
Change-Id: I41238de28f32f2a622c1d045debe3ea11e7c23f6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232000
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35014}
2021-09-16 13:19:31 +00:00
Sergey Sukhanov
3b08fe3dcd dssctp: support socket handover in StreamResetHandler
Bug: webrtc:13154
Change-Id: Idafbed4f3c1af8d0cca833ba983c4b4b99118335
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232121
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Commit-Queue: Sergey Sukhanov <sergeysu@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35012}
2021-09-16 11:33:21 +00:00
Sergey Sukhanov
cfee05464c dcsctp: Refactor socket test to allow recreation of the sockets.
This will be useful in future socket handover tests.

Bug: webrtc:13154
Change-Id: Ia789ae971edd9d2832be088f2f8f7dd50c9ce52d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/231222
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Commit-Queue: Sergey Sukhanov <sergeysu@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#34931}
2021-09-06 14:33:31 +00:00
Victor Boivie
cebbff7f58 dcsctp: Specify the max timer backoff duration
By allowing the max timer backoff duration to be limited, a socket can
fast recover in case of intermittent network issues. Before this CL, the
exponential backoff algorithm could result in very long retry durations
(in the order of minutes), when connection has been lost or been flaky
for a long while.

Note that limiting the maximum backoff duration might require
compensating the maximum retransmission limit to avoid closing the
socket prematurely due to reaching the maximum retransmission limit much
faster than previously.

Bug: webrtc:13129
Change-Id: Ib94030d666433e3fa1a2c8ef69750a1afab8ef94
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/230702
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#34913}
2021-09-03 10:26:50 +00:00
Victor Boivie
9680d29e8d dcsctp: Support unlimited max_retransmissions
The restart limit for timers can already be limitless, but the
RetransmissionErrorCounter didn't support this. With this change, the
max_retransmissions and max_init_retransmits can be absl::nullopt to
indicate that there should be infinite retries.

Bug: webrtc:13129
Change-Id: Ia6e91cccbc2e1bb77b3fdd7f37436290adc2f483
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/230701
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#34882}
2021-08-31 10:57:48 +00:00
Victor Boivie
0ca62e3752 dcsctp: Avoid bundling FORWARD-TSN and DATA chunks
dcSCTP seems to be able to provoke usrsctp to send ABORT in some
situations, as described in
https://github.com/sctplab/usrsctp/issues/597. Using a packetdrill
script, it seems as a contributing factor to this behavior is when a
FORWARD-TSN chunk is bundled with a DATA chunk. This is a valid and
recommended pattern in RFC3758:

  "F2) The data sender SHOULD always attempt to bundle an outgoing
       FORWARD TSN with outbound DATA chunks for efficiency."

However, that seems to be a rare event in usrsctp, which generally sends
each FORWARD-TSN in a separate packet.

By doing the same, the assumption is that this scenario will generally
be avoided.

With many browsers and other clients using usrsctp, and which will not
be upgraded for a long time, there is an advantage of avoiding the issue
even if it is according to specification.

Before this change, a FORWARD-TSN was bundled with outgoing DATA and due
to this, it wasn't rate limited as the overhead was very little. With
this change, a rate limiting behavior has been added to avoid sending
too many FORWARD-TSN in small packets. It will be sent every RTT, or
200 ms, whichever is smallest. This is also described in the RFC.

Bug: webrtc:12961
Change-Id: I3d8036a34f999f405958982534bfa0e99e330da3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/229101
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34801}
2021-08-19 11:28:40 +00:00
Victor Boivie
3ec9e03f73 dcsctp: Removing all references to unordered_map
Replacing with std::map and webrtc::flat_map where applicable.

Bug: webrtc:12689
Change-Id: Id0fdb88bd3d52957b1616911eb487fc581d3b7d8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/229182
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34797}
2021-08-18 19:55:07 +00:00
Victor Boivie
14ef6338b0 dcsctp: Don't send small packets when cwnd full
The congestion window is unlikely to be even divisible by the size
of a packet, so when the congestion window is almost full, there is
often just a few bytes remaining in it. Before this change, a small
packet was created to fill the remaining bytes in the congestion window,
to make it really full.

Small packets don't add much. The cost of sending a small packet is
often the same as sending a large one, and you usually get lower
throughput sending many small packets compared to few larger ones.'

This mode will only be enabled when the congestion window is large, so
if the congestion window is small - e.g. due to poor network conditions,
it will allow packets to become fragmented into small parts, in order to
fully utilize the congestion window.

Bug: webrtc:12943
Change-Id: I8522459174bc72df569edd57f5cc4a494a4b93a8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228526
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34778}
2021-08-17 09:03:36 +00:00
Victor Boivie
abf6188cba dcsctp: Add PacketSender
This is mainly a refactoring commit, to break out packet sending to a
dedicated component.

Bug: webrtc:12943
Change-Id: I78f18933776518caf49737d3952bda97f19ef335
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228565
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34772}
2021-08-16 20:19:53 +00:00
Victor Boivie
600bb8c79f dcsctp: Migrating to using absl::bind_front
It is now allowed in WebRTC, so let's use it.

Bug: webrtc:12943
Change-Id: I74a0f2fd9c1b9e7b5613ae1c592cf26842b8dddd
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228564
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34768}
2021-08-16 11:51:27 +00:00
Victor Boivie
8df32eb0e1 dcsctp: Add API to indicate packet send status
Before this change, there was no way for a client to indicate to the
dcSCTP library if a packet that was supposed to be sent, was actually
sent. It was assumed that it always was.

To handle temporary failures better, such as retrying to send packets
that failed to be sent when the send buffer was full, this information
is propagated to the library.

Note that this change only covers the API and adaptations to clients.
The actual implementation to make use of this information is done as a
follow-up change.

Bug: webrtc:12943
Change-Id: I8f9c62e17f1de1566fa6b0f13a57a3db9f4e7684
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228563
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34767}
2021-08-16 11:29:47 +00:00
Victor Boivie
e0fb45c6d4 dcsctp: Add burst limiter for sent packets
Some deployments, e.g. Chromium, has a limited send buffer. It's
reasonable that it's quite small, as it avoids queuing too much, which
typically results in increased latency for real-time communication. To
avoid SCTP to fill up the entire buffer at once - especially when doing
fast retransmissions - limit the amount of packets that are sent in one
go.

In a typical scenario, SCTP will not send more than three packets for
each incoming packet, which is is the case when a SACK is received which
has acknowledged two large packets, and which also adds the MTU to the
congestion window (due to in slow-start mode), which then may result in
sending three packets. So setting this value to four makes any
retransmission not use that much more of the send buffer.

This is analogous to usrsctp_sysctl_set_sctp_fr_max_burst_default in
usrsctp, which also has the default value of four (4).

Bug: webrtc:12943
Change-Id: Iff76a1668beadc8776fab10312ef9ee26f24e442
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228480
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34744}
2021-08-12 17:22:55 +00:00
Victor Boivie
d4716eaf60 dcsctp: Add metrics support
To support implementing RTCSctpTransportStats, a few metrics are needed.

Some more were added that are useful for metric collection in SFUs.

Bug: webrtc:13052
Change-Id: Idafd49e1084922d01d3e6c5860715f63aea08b7d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228243
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34708}
2021-08-10 20:01:46 +00:00
Victor Boivie
bf15e567e8 dcsctp: Abandon chunks consistently
The previous logic to abandon chunks when partial reliability was used
was a bit too eager and trigger happy.

 * Chunks with limited retransmissions should only be abandoned when a
   chunk is really considered lost. It should follow the same rules as
   for retransmitting chunks - that it must be nacked three times or
   due to a T3-RTX expiration. Before this change, a single SACK not
   referencing it would be enough to abandon it. This resulted in a lot
   of unnecessary sent FORWARD-TSN and undelivered messages - especially
   if running with zero retransmissions.

   The logic to expire chunks by limited retransmissions will now only
   be applied when a chunk is actually nacked.

 * The second partial reliability trigger - expiration time - wasn't
   evaluated when producing a middle chunk of a larger message.

A number of test cases were added and updated as chunks will now be
abandoned immediately instead of first scheduled for retransmission and
later abandoned.

Bug: webrtc:12961
Change-Id: I0ae17b2672568bdbdc32073a99d4c24b09ff5fe9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/225548
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34458}
2021-07-12 22:47:51 +00:00
Victor Boivie
813a087e84 dcsctp: Add packet capture support in unit tests
This is needed to be able to debug test cases when they fail.

Bug: webrtc:12961
Change-Id: I39bfe532709d02acb328ff5fdd005d33be4dc31c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/225544
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34452}
2021-07-10 18:37:46 +00:00
Victor Boivie
8bd26e12ed dcsctp: Only reset paused streams when peer acks
When a single stream is reset, and an outgoing SSN reset request is sent
and later acked by the peer sending a reconfiguration response with
status=Performed, the sender should unpause the paused stream and reset
the SSNs of that (ordered) stream. But only the single stream that was
paused, and not all streams. In this scenario, dcSCTP would - when the
peer acked the SSN reset request - reset the SSN of all streams.

This was found by orphis@webrtc.org using a data channel test
application. The peer, if it's a usrsctp client, will ABORT with
PROTOCOL_VIOLATION as it has already seen that SSN on that stream but
with a different TSN.

This bug was introduced when implementing the Round Robin scheduler in
https://webrtc-review.googlesource.com/c/src/+/219682. The FCFS
scheduler prior to this change was implemented correctly.

Bug: webrtc:12952
Change-Id: I3ea144a1df303145f69a5b03aada7f448c8c8163
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/225266
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34436}
2021-07-08 10:49:11 +00:00
Victor Boivie
c20f1563b6 dcsctp: Don't sent more packets before COOKIE ACK
While in the COOKIE ECHO state, there is a TCB and there might be data
in the send buffer, and RFC4960 allows the COOKIE ECHO chunk to bundle
additional DATA chunks in the same packet, but there mustn't be more
than one such packet sent, and that packet must have a COOKIE ECHO chunk
as the first chunk in it.

When the COOKIE ACK chunk has been received, the socket is allowed to
send multiple packets.

Previously, this was state managed by the socket and not the TCB, as
the socket is responsible for moving between the different states. And
when the COOKIE ECHO chunk was sent, the TCB was instructed to only send
a single packet by the socket.

However, if there were retransmissions or anything else that could
result in calling TransmissionControlBlock::SendBufferedChunks, it would
do as instructed and send those, even if the socket was in a state where
that wasn't allowed.

When the peer was dcSCTP, this didn't cause any issues as dcSCTP tries
to be tolerant in what it receives (but strict in what it sends, except
for when there are bugs). When the peer was usrsctp, it would send an
ABORT for each received packet that didn't have a COOKIE ECHO as the
first chunk, and then restart the handshake (sending an INIT). So this
resulted in a longer handshake, but the connection would eventually be
correctly established and any DATA chunks that resulted in the ABORTs
would've been retransmitted.

By making the TCB aware of that particular state, and to make it
responsible for creating the SCTP packet with the COOKIE ECHO chunk
first, and also to only send a single packet when it is in that state,
there will not be any way to bypass this limitation.

Also, while not explicitly mentioned in the RFC, the retransmission
timer will not affect resending any outstanding DATA chunks that were
bundled together with the COOKIE ECHO chunk, as then there would be two
timers that both would drive resending COOKIE ECHO and DATA chunks.

Bug: webrtc:12880
Change-Id: I76f215a03cceab5bafe9f16eb4775f3dc68a6f05
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/222645
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34329}
2021-06-18 08:50:59 +00:00
Victor Boivie
5429d71022 dcsctp: Allow heartbeats to be disabled
This is useful in tests and in scenarios where the connection is
monitored externally and the heartbeat monitoring would be of no use.

Bug: webrtc:12614
Change-Id: Ida4f4e2e40fc4d2aa0c27ae9431f434da4cc8313
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220766
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34164}
2021-05-31 12:19:38 +00:00
Victor Boivie
236ac50628 dcsctp: Add public API for BufferedAmountLow
This adds native support for the RTCDataChannel properties:
https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/bufferedAmount
https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/bufferedAmountLowThreshold

And the RTCDataChannel event:
https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/onbufferedamountlow

The old callback, NotifyOutgoingMessageBufferEmpty, is deprecated as it
didn't work very well. It will not be triggered and will be removed
as soon as all users of it are gone. There is a new callback,
OnTotalBufferedAmountLow, that serves the same purpose but also allows
setting an arbitrary limit when it should be triggered (See
DcSctpOptions::total_buffered_amount_low_threshold).

Bug: webrtc:12794
Change-Id: Ic1c92f174eff8a1acda0b5fd3dcc45bd1cfa2704
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219691
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34144}
2021-05-27 15:27:27 +00:00
Victor Boivie
bd9031bf22 dcsctp: Add OnTotalBufferedAmountLow in Send Queue
This is similar to Change-Id: I12a16f44f775da3711f3aa52a68a0bf24f70d2f8
but with the entire send buffer as scope, not a single stream.

This can be used by clients to take alternate action (such as delaying
transmission or using other buffering) if the send buffer ever becomes
full, as they can now be notified when the send buffer is no longer
full.

Bug: webrtc:12794
Change-Id: Icf3be3b118888ffb5ced955fd7ba4826a37140f9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220360
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34143}
2021-05-27 14:38:18 +00:00
Victor Boivie
791adafa09 dcsctp: Add OnBufferedAmountLow in Send Queue
This adds the necessary properties and callback to the Send Queue to
support the bufferedAmount & bufferedAmountLowThreshold properties and
the bufferedamountlow event in RTCDataChannel.

The public API changes and socket support comes in a follow-up CL.

Bug: webrtc:12794
Change-Id: I12a16f44f775da3711f3aa52a68a0bf24f70d2f8
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219690
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34142}
2021-05-27 13:16:28 +00:00
Victor Boivie
2440d34075 dcsctp: Rename FCFSSendQueue to RRSendQueue
The current send queue implements SCTP_SS_FCFS as defined in
https://datatracker.ietf.org/doc/html/rfc8260#section-3.1, but that has
always been known to be a temporary solution. The end goal is to
implement a Weighted Fair Queueing Scheduler (SCTP_SS_WFQ), but that's
likely to take some time.

Meanwhile, a round robin scheduler (SCTP_SS_RR) will be used to avoid
some issues with the current scheduler, such as a single data channel
completely blocking all others if it sends a lot of messages.

In this first commit, the code has simply been renamed and is still
implementing first-come-first-served. That will be fixed in follow-up
CLS.

Bug: webrtc:12793
Change-Id: Idc03b1594551bfe1ddbe1710872814b9fdf60cc9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219684
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34090}
2021-05-22 20:56:13 +00:00
Victor Boivie
50a0b1219e dcsctp: Avoid transition back to ShutdownPending
When a socket is shutting down, either explicitly by the ULP calling
Shutdown(), or when the socket receives a SHUTDOWN chunk, the socket
should send all outstanding data and when all is sent and acked,
_then_ it should continue the shutdown protocol.

As it currently doesn't calculate correctly when all data has been sent,
as NACKED chunks are not included in what it believes is remaining in
the retransmission queue, it will shut down prematurely and may go back
to a previous state (ShutdownPending) from ShutdownSent or
ShutdownAckSent.

This is a workaround that just avoids the illegal state transition as
that puts the socket in an inconsistent state. The bug is merely
theoretical as WebRTC doesn't currently gracefully shut down a SCTP
socket, but just terminates the DTLS transport.

As TODOs mention, this will be fixed correctly a bit later.

Bug: webrtc:12739
Change-Id: Ibde2acc3a6aca701ac178d6181028404d470a5d5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/218340
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33982}
2021-05-11 16:04:42 +00:00
Victor Boivie
d3b186e3d6 dcsctp: Support message with low lifetime
While it's not strictly defined, the expectation is that sending a
message with a lifetime parameter set to zero (0) ms should allow it to
be sent if it can be sent without being buffered. If it can't be
directly sent, it should be discarded.

This is initial support for it. Small messages can now be delivered fine
if they are not to be buffered, but fragmented messages could be partly
sent (if this fills up the congestion window), which means that the
message will then fail to be sent whenever the congestion window frees
up again. It would be better to - at a higher level - realize early that
the message can't be sent in full, and discard it without sending
anything. But that's an optimization that can be done later.

A few off-by-one errors were found when strictly defining that the
message is alive during its entire lifetime. It will expire just _after_
its lifetime.

Sending messages with a lifetime of zero may not supported in all
libraries, so a workaround would be to set a very small timeout instead,
which is tested as well.

Bug: webrtc:12614
Change-Id: I9a00bedb639ad7b3b565b750ef2a49c9020745f1
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217562
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33977}
2021-05-11 08:44:14 +00:00
Victor Boivie
914925f51e dcsctp: Don't access TCB when the socket is closed
When the shutdown timer has expired, the socket will abort/close and the
TCB is not valid after InternalClose.

Bug: webrtc:12614
Change-Id: I09a94a049f0cda4577225dd9c80a92a8ec7e0423
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217767
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33956}
2021-05-07 19:04:49 +00:00
Victor Boivie
f95536dd5a dcsctp: Stop connection timers during shutdown
If Shutdown is called when the socket is being established and while the
connection timers are running, it will put the socket in an inconsistent
state, which is verified in debug builds.

Bug: webrtc:12614
Change-Id: I66f07d1170ac8f0ad9fd485d77d6aef4c365f150
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217765
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33949}
2021-05-07 13:51:57 +00:00
Victor Boivie
3dadf8b06f dcsctp: Log socket name also in callbacks
This makes it easier to understand which socket that experience an error
or abort. Aborts are now also logged, which was missed previously.

Bug: webrtc:12614
Change-Id: Ie5e4357b3e5450106cc6cc28c1e9578ad53d073a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217764
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33947}
2021-05-07 11:36:57 +00:00
Victor Boivie
59b802883a dcsctp: Refactor unit tests
Bug: webrtc:12614
Change-Id: I9592f1ec8bec2a045c9d32fda3a723877ae38e58
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217763
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33943}
2021-05-07 09:57:23 +00:00
Victor Boivie
1d2fa9a1c3 dcsctp: Expire timers just before triggering them
In real life, when a Timeout expires, the caller is supposed to call
DcSctpSocket::HandleTimeout directly, as the Timeout that just expired
is stopped (it just expired), but the Timer still believes it's running.
The system is not in a consistent state.

In tests, all timeouts were evaluated at the same time, which, if two
timeouts expired at the same time, would put them both as "not running",
and with their timers believing they were running. So if you would do
any operation on a timer whose timeout had just expired, the timeout
would assert saying that "you can't stop a stopped timeout" or similar.

This isn't relevant in non-test scenarios.

Solved by expiring timeouts one by one.

Bug: webrtc:12614
Change-Id: I79d006f4d3e96854d77cec3eb0080aa23b8569cb
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217560
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33925}
2021-05-05 12:40:21 +00:00
Florent Castelli
0810b05104 dcsctp: Add SetMaxMessageSize() to socket
An SCTP transport for Data Channels allows changing the maximum
message size through SDP.
See https://w3c.github.io/webrtc-pc/#sctp-transport-update-mms

Bug: webrtc:12614
Change-Id: I8cff33c5f9c1d60934a726c546bc9cbdcd9e22d9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217387
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33920}
2021-05-04 21:43:24 +00:00
Mirko Bonadei
b7854e43af Enable GN check on //net.
This should avoid the situation where WebRTC's GN check is green and
Chromium (which turns it ON for //third_party/webrtc) fails.

Bug: webrtc:12614
Change-Id: Id4c06ac57e9faa07c5e43491a61fbc093c68a40d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217221
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33900}
2021-05-03 14:23:09 +00:00
Florent Castelli
6072275e4a dcsctp: Add missing target dependencies
Those were found when trying to build within Chromium's codebase.

Bug: webrtc:12614
Change-Id: Ic3f7a266ad4b5d816a693645e1e909fc39d513c3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217220
Reviewed-by: Victor Boivie <boivie@webrtc.org>
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33896}
2021-05-03 12:19:29 +00:00
Victor Boivie
b6580ccb29 dcsctp: Add Socket
This completes the basic implementation of the dcSCTP library. There
are a few remaining commits to e.g. add compatibility tests and
benchmarks, as well as more support for e.g. RFC8260, but those are not
strictly vital for evaluation of the library.

The Socket contains the connection establishment and teardown sequences
as well as the general chunk dispatcher.

Bug: webrtc:12614
Change-Id: I313b6c8f4accc144e3bb88ddba22269ebb8eb3cd
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/214342
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Reviewed-by: Tommi <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33890}
2021-05-01 07:16:21 +00:00
Victor Boivie
21509566b8 dcsctp: Add Transmission Control Block
This is merely a container of components that have their lifetime
bound to when the socket is connected. If the socket gets disconnected
or restarted, this object (and everything it holds) will be released.

Bug: webrtc:12614
Change-Id: Ibd75760b7bf7efe9c26c4eb7cee62de8bba5410c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/214340
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33869}
2021-04-28 22:45:03 +00:00