MediaTime: fix negative value into Duration bug #400

david-flok · 2023-11-14T16:21:03Z

as u64 of i64 is dangerous. Duration is non-negative but MediaTime is not necessarily. This fixes what is effectively an integer underflow.

david-flok · 2023-11-14T16:26:14Z

Actually, I'm not sure that MediaTime should have From<MediaTime> for Duration at all -- this conversion is not lossless. I think I am in favor of an explicit conversion instead or TryFrom or something.

lolgesten · 2023-11-14T16:37:04Z

I don't feel strongly one way or the other.

Have you experienced a bug due to this conversion?

david-flok · 2023-11-20T08:50:49Z

Yes. I had two MediaTime values that I expected to almost always be ordered tsA < tsB and so I used Duration::max(Duration::ZERO, (tsB - tsA).into()) but this results in underflow. Instead, I must use MediaTime::max(MediaTime::ZERO, tsB - tsA).into() so the timestamp doesn't get corrupted. I would expect _.into() and max to commute.

I will prepare a proposed MediaTime patch to catch this issue statically.

algesten · 2023-11-20T09:02:09Z

Sounds good!

david-flok · 2023-11-20T14:33:55Z

@algesten I've revised the patch. While I was there, I also fixed some defects regarding handling of negative and zero denominators. Those cases are now statically disallowed. This should eliminate a possible DoS vulnerability resulting from SDP parsing of streams with 0 frequency resulting in division by zero panics.

You can, of course, cherry-pick the commit(s) you want or ask me to prepare a reduced changeset but I would very much like this basic data type to be as safe as possible.

lolgesten · 2023-11-20T16:00:58Z

Thanks!

I'm in two minds about this Frequency newtype. On the one hand, it provides some really nice guarantees in the SDP parsing, on the other, it is Another Thing a user of the library need to get their head around. It doesn't look like Frequency is visible in that many places in the public API though. Could you maybe try making it pub(crate) and see what compilation errors you get?

For now we could potentially keep it internal and decide later if we want to have it public.

david-flok · 2023-11-21T08:38:02Z

Unfortunately, it is indeed rather heavyweight compared to just plugging in literals but I have tried to make it as straightforward as possible to use with frequency-specific constructors and constants.

As far as I can see, the design space for this division by zero restriction is:

Do nothing and expect the user to ensure non-zero frequency.
Check at construction time and panic.
Check at construction time and return a Result/Option.
Check at construction time and map 0 to 1 silently.
Use NonZeroU32 directly in interfaces.
Wrap NonZeroU32 with a newtype (Frequency).

Perhaps you'd be happier with 4?

I have added AddAssign and SubAssign implementations because doing t = t + x instead of t += x was bothering me. I've also rebased onto main as there were some conflicts.

david-flok · 2023-11-21T08:45:08Z

Another option would be to do both the Frequency newtype and the 0 to 1 mapping with the larger type on input and the smaller type on output (Postel style).

The Frequency type only shows up in signatures for things like clock_rate and the denominator of these MediaTime terms -- it's there entirely to guarantee that there are only 2^32 - 1 values.

algesten · 2023-11-21T09:49:28Z

@david-flok I'm coming around to this being a newtype. Having it in Format changes things a bit for me.

algesten · 2023-11-21T10:02:15Z

@xnorpx you had thoughts on MediaTime (passing it to write_rtp, rather than just the RTP u32 value).

My idea for API ergonomics here is that

Get the Format for whatever you're sending (Rtc -> CodecConfig -> PayloadParam -> CodecSpec -> Format)
Grab the Frequency from that Format.
Make a MediaTime using the Frequency (this could potentially be expanded with more helpers)

Since you need to know the negotiated PT, you already need to dig out the PayloadParam.

xnorpx · 2023-11-21T16:13:46Z

It feels a little clunky to need to create a MediaTime everytime I call write. Pass in Pt and RtpTimestamp and MediaTime could be created inside. But it could just be the way I use it.

We probably need to add 8000 and 16000 at some point to the frequency struct, but I like having the new type.

algesten

Ok, let's land this. I want some documentation is all.

algesten · 2023-11-22T07:53:08Z

src/rtp/mtime.rs

@@ -1,36 +1,127 @@
 #![allow(missing_docs)]

 use std::cmp::Ordering;
-use std::ops::{Add, Sub};
+use std::fmt::Display;


This is not your problem, really, but it's expanding with the increased amount of code in this file.

Could you remove the #![allow(missing_docs)] at the top and document the new and old code?

Also, In the Frequency doc on struct, and MediaTime struct, I'd like to see some code examples how to get the Frequency for a certain Format from the chain Rtc -> CodecConfig -> PayloadParam -> CodecSpec -> Format -> Frequency and how to subsequenty use that to construct a MediaTime. You can duplicate the code example.

I'm not sure what you have in mind for this example so I've not guessed, sorry.

algesten · 2023-11-22T08:34:33Z

@david-flok also, note #338 which I want to land before this, which will clean up in MediaTime a bit.

algesten · 2023-11-24T09:13:27Z

Hi @david-flok, I know I've created some more work in this PR.

Would you prefer if I took the PR to completion, or do you want to finish it?

These each indicate a defect of some severity.

david-flok · 2023-11-27T10:57:02Z

Hi @algesten, no worries. I'll polish this now, sorry for the delay.

This enforces the signedness invariant in the type so that negative denominators are unrepresentable. This saves complexity in managing improper ratios with cancelling signs.

MediaTime supports negative durations but Duration does not. Expose the partiality of the conversion in the signature.

Ensure that we can never have a delayed divide by zero panic due to carrying a zero denominator around. This removes a denial of service vulnerability from SDP parsing and makes the MediaTime type safer for API users. The change from 64-bit to 32-bit is justified by the observation that it is unlikely applications will need to represent times with precision smaller than quarters of nanoseconds.

david-flok · 2023-11-27T14:44:01Z

Ok, I've rebased and resolved the conflicts with #338. I also did some documentation but I haven't provided the example(s) you wanted, sorry.

lolgesten

Let's ship it!

Thanks!

algesten

From my private user

algesten · 2023-11-27T15:03:06Z

@david-flok I added some code example: a319da9

david-flok force-pushed the mediatime-negative-duration branch from 3aaf33b to 1495560 Compare November 20, 2023 14:30

david-flok force-pushed the mediatime-negative-duration branch from 56bc26c to 0659282 Compare November 21, 2023 08:31

algesten requested changes Nov 22, 2023

View reviewed changes

MediaTime: add 4 failing data type tests

00eb33e

These each indicate a defect of some severity.

david-flok added 6 commits November 27, 2023 11:00

MediaTime: make denominator non-negative

70e5443

This enforces the signedness invariant in the type so that negative denominators are unrepresentable. This saves complexity in managing improper ratios with cancelling signs.

MediaTime: impl TryFrom<MediaTime> for Duration instead of From

4dfdc53

MediaTime supports negative durations but Duration does not. Expose the partiality of the conversion in the signature.

MediaTime: implement += and -=

63eb6a1

MediaTime: add +/- for Instant

a94fbe0

MediaTime: document the interface

342a384

david-flok force-pushed the mediatime-negative-duration branch from c09474e to 342a384 Compare November 27, 2023 14:42

lolgesten approved these changes Nov 27, 2023

View reviewed changes

algesten approved these changes Nov 27, 2023

View reviewed changes

algesten merged commit 7565523 into algesten:main Nov 27, 2023
22 checks passed

david-flok deleted the mediatime-negative-duration branch November 27, 2023 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MediaTime: fix negative value into Duration bug #400

MediaTime: fix negative value into Duration bug #400

david-flok commented Nov 14, 2023

david-flok commented Nov 14, 2023

lolgesten commented Nov 14, 2023

david-flok commented Nov 20, 2023

algesten commented Nov 20, 2023

david-flok commented Nov 20, 2023

lolgesten commented Nov 20, 2023

david-flok commented Nov 21, 2023

david-flok commented Nov 21, 2023

algesten commented Nov 21, 2023

algesten commented Nov 21, 2023 •

edited

Loading

xnorpx commented Nov 21, 2023

algesten left a comment

algesten Nov 22, 2023

david-flok Nov 27, 2023

algesten commented Nov 22, 2023

algesten commented Nov 24, 2023

david-flok commented Nov 27, 2023 •

edited

Loading

david-flok commented Nov 27, 2023

lolgesten left a comment

algesten left a comment

algesten commented Nov 27, 2023

MediaTime: fix negative value into Duration bug #400

MediaTime: fix negative value into Duration bug #400

Conversation

david-flok commented Nov 14, 2023

david-flok commented Nov 14, 2023

lolgesten commented Nov 14, 2023

david-flok commented Nov 20, 2023

algesten commented Nov 20, 2023

david-flok commented Nov 20, 2023

lolgesten commented Nov 20, 2023

david-flok commented Nov 21, 2023

david-flok commented Nov 21, 2023

algesten commented Nov 21, 2023

algesten commented Nov 21, 2023 • edited Loading

xnorpx commented Nov 21, 2023

algesten left a comment

Choose a reason for hiding this comment

algesten Nov 22, 2023

Choose a reason for hiding this comment

david-flok Nov 27, 2023

Choose a reason for hiding this comment

algesten commented Nov 22, 2023

algesten commented Nov 24, 2023

david-flok commented Nov 27, 2023 • edited Loading

david-flok commented Nov 27, 2023

lolgesten left a comment

Choose a reason for hiding this comment

algesten left a comment

Choose a reason for hiding this comment

algesten commented Nov 27, 2023

algesten commented Nov 21, 2023 •

edited

Loading

david-flok commented Nov 27, 2023 •

edited

Loading