Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2157843002:
Fix crash which happens when there's reordering in the beginning of a call. (Closed)
|
Created:
4 years, 5 months ago by stefan-webrtc Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, sprang Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix crash which happens when there's reordering in the beginning of a call.
The added unittest triggers this CHECK:
https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e8122ef34901519a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc#146
It happens because the unwrap of the sequence number fails if the unwrappers last sequence number is small, but the newly added sequence number is large (greater than last seq num + 2^15), and therefore should have been interpreted as a reordering and a backwards wrap. Since that would mean the sequence number returned from the unwrapper would be negative, it simply returns the original sequence number instead. This causes problems later where the wrap is correctly handled, and everything breaks.
The real solution should be to correctly handle wraps, but to prevent the crash this is a reasonable workaround for now.
BUG=
Committed: https://crrev.com/159a2fe9da7194492d688f6b46f6e665821ddb8a
Cr-Commit-Position: refs/heads/master@{#13496}
Patch Set 1 #Patch Set 2 : nits #
Total comments: 5
Patch Set 3 : Addressed comment. #
Messages
Total messages: 24 (13 generated)
stefan@webrtc.org changed reviewers: + sprang@webrtc.org, terelius@webrtc.org
Terelius, can you take a look at this since Erik is on vacation?
nits
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix crash which happens when there's reordering in the beginning of a call. BUG= ========== to ========== Fix crash which happens when there's reordering in the beginning of a call. The added unittest triggers this CHECK: https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e... BUG= ==========
Description was changed from ========== Fix crash which happens when there's reordering in the beginning of a call. The added unittest triggers this CHECK: https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e... BUG= ========== to ========== Fix crash which happens when there's reordering in the beginning of a call. The added unittest triggers this CHECK: https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e... It happens because the unwrap of the sequence number fails if the unwrappers last sequence number is small, but the newly added sequence number is large (>2^15), and therefore should have been interpreted as a reordering and a backwards wrap. Since that would mean the sequence number returned from the unwrapper would be negative, it simply returns the original sequence number instead. This causes problems later where the wrap is correctly handled, and everything breaks. The real solution should be to correctly handle wraps, but to prevent the crash this is a reasonable workaround for now. BUG= ==========
Description was changed from ========== Fix crash which happens when there's reordering in the beginning of a call. The added unittest triggers this CHECK: https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e... It happens because the unwrap of the sequence number fails if the unwrappers last sequence number is small, but the newly added sequence number is large (>2^15), and therefore should have been interpreted as a reordering and a backwards wrap. Since that would mean the sequence number returned from the unwrapper would be negative, it simply returns the original sequence number instead. This causes problems later where the wrap is correctly handled, and everything breaks. The real solution should be to correctly handle wraps, but to prevent the crash this is a reasonable workaround for now. BUG= ========== to ========== Fix crash which happens when there's reordering in the beginning of a call. The added unittest triggers this CHECK: https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e... It happens because the unwrap of the sequence number fails if the unwrappers last sequence number is small, but the newly added sequence number is large (greater than last seq num + 2^15), and therefore should have been interpreted as a reordering and a backwards wrap. Since that would mean the sequence number returned from the unwrapper would be negative, it simply returns the original sequence number instead. This causes problems later where the wrap is correctly handled, and everything breaks. The real solution should be to correctly handle wraps, but to prevent the crash this is a reasonable workaround for now. BUG= ==========
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/2157843002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2157843002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:97: if (seq > window_start_seq_ + 0xFFFF / 2) { Should this check be using IsNewerSequenceNumber? https://codereview.webrtc.org/2157843002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:100: "failed to handle it.Feedback window starts at " ". F"
https://codereview.webrtc.org/2157843002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2157843002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:97: if (seq > window_start_seq_ + 0xFFFF / 2) { On 2016/07/18 08:53:57, pbos-webrtc wrote: > Should this check be using IsNewerSequenceNumber? That wouldn't be that simple to write I think. We want to capture if there was reordering which also happened around a wrap, and if that happened when the window_start_seq_ was <= 0xFFFF, as that would require seq to be negative. An alternative would be to simply allow the unwrapper to return a negative seq (or -1 because it failed), but then I'll have to go over more code to see that nothing else breaks. If you prefer that I can certainly do that. https://codereview.webrtc.org/2157843002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:100: "failed to handle it.Feedback window starts at " On 2016/07/18 08:53:57, pbos-webrtc wrote: > ". F" Acknowledged.
fwiw lgtm if you fix the ".F" typo. https://codereview.webrtc.org/2157843002/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2157843002/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:97: if (seq > window_start_seq_ + 0xFFFF / 2) { On 2016/07/18 09:30:20, stefan-webrtc (holmer) wrote: > On 2016/07/18 08:53:57, pbos-webrtc wrote: > > Should this check be using IsNewerSequenceNumber? > > That wouldn't be that simple to write I think. We want to capture if there was > reordering which also happened around a wrap, and if that happened when the > window_start_seq_ was <= 0xFFFF, as that would require seq to be negative. An > alternative would be to simply allow the unwrapper to return a negative seq (or > -1 because it failed), but then I'll have to go over more code to see that > nothing else breaks. If you prefer that I can certainly do that. Nah, no strong preferences if it doesn't make things simpler.
Addressed comment.
lgtm, though I would prefer to handle this in SequenceNumberUnwrapper.
On 2016/07/18 10:22:11, terelius wrote: > lgtm, though I would prefer to handle this in SequenceNumberUnwrapper. Me too, but we'll try to do that in a follow-up later as we solve https://bugs.chromium.org/p/webrtc/issues/detail?id=6118.
The CQ bit was checked by stefan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2157843002/#ps40001 (title: "Addressed comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Fix crash which happens when there's reordering in the beginning of a call. The added unittest triggers this CHECK: https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e... It happens because the unwrap of the sequence number fails if the unwrappers last sequence number is small, but the newly added sequence number is large (greater than last seq num + 2^15), and therefore should have been interpreted as a reordering and a backwards wrap. Since that would mean the sequence number returned from the unwrapper would be negative, it simply returns the original sequence number instead. This causes problems later where the wrap is correctly handled, and everything breaks. The real solution should be to correctly handle wraps, but to prevent the crash this is a reasonable workaround for now. BUG= ========== to ========== Fix crash which happens when there's reordering in the beginning of a call. The added unittest triggers this CHECK: https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e... It happens because the unwrap of the sequence number fails if the unwrappers last sequence number is small, but the newly added sequence number is large (greater than last seq num + 2^15), and therefore should have been interpreted as a reordering and a backwards wrap. Since that would mean the sequence number returned from the unwrapper would be negative, it simply returns the original sequence number instead. This causes problems later where the wrap is correctly handled, and everything breaks. The real solution should be to correctly handle wraps, but to prevent the crash this is a reasonable workaround for now. BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix crash which happens when there's reordering in the beginning of a call. The added unittest triggers this CHECK: https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e... It happens because the unwrap of the sequence number fails if the unwrappers last sequence number is small, but the newly added sequence number is large (greater than last seq num + 2^15), and therefore should have been interpreted as a reordering and a backwards wrap. Since that would mean the sequence number returned from the unwrapper would be negative, it simply returns the original sequence number instead. This causes problems later where the wrap is correctly handled, and everything breaks. The real solution should be to correctly handle wraps, but to prevent the crash this is a reasonable workaround for now. BUG= ========== to ========== Fix crash which happens when there's reordering in the beginning of a call. The added unittest triggers this CHECK: https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e... It happens because the unwrap of the sequence number fails if the unwrappers last sequence number is small, but the newly added sequence number is large (greater than last seq num + 2^15), and therefore should have been interpreted as a reordering and a backwards wrap. Since that would mean the sequence number returned from the unwrapper would be negative, it simply returns the original sequence number instead. This causes problems later where the wrap is correctly handled, and everything breaks. The real solution should be to correctly handle wraps, but to prevent the crash this is a reasonable workaround for now. BUG= Committed: https://crrev.com/159a2fe9da7194492d688f6b46f6e665821ddb8a Cr-Commit-Position: refs/heads/master@{#13496} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/159a2fe9da7194492d688f6b46f6e665821ddb8a Cr-Commit-Position: refs/heads/master@{#13496} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
