search

algesten / str0m

A Sans I/O WebRTC implementation in Rust.
MIT License
317 stars 49 forks source link

Panic during TWCC interim building #523

Closed OxleyS closed 4 months ago

OxleyS commented 4 months ago

A server running str0m recently panicked with assertion failed: t >= -32_765 && t <= 32_767. The stack trace was a little busted due to being Release mode, but from the message it must have been this line: https://github.com/algesten/str0m/blob/08562d74683c66431dcd389b0248fc67bd4396fa/src/rtp/rtcp/twcc.rs#L533 Because it was likely the result of janky network data, I don't have a reliable repro.

Digging into it, I was a little unsure what the intended bounds on t were. The context of this line is as follows: https://github.com/algesten/str0m/blob/08562d74683c66431dcd389b0248fc67bd4396fa/src/rtp/rtcp/twcc.rs#L527-L539

Here, diff_time is the difference in timestamp between two received packets. The result needs to fit in an i16, so our absolute bounds would be [-32768, 32767], but here the panicking assertion instead checks for -32765 on the lower end. In addition, the first if branch here checks for < -8_192_000, which when divided by 250 yields -32768. Is the intended lower bound -32768 or -32765?

algesten commented 4 months ago

This does indeed look broken

lolgesten commented 4 months ago

@OxleyS I say it's -32768. Want to make a PR?

OxleyS commented 4 months ago

-32768 also seemed like the intended value to me. Sure, I'll make a PR for it.