Soundness issue

[Freax13]

Sdp::get_mlines and Sdp::deref dereference the user accessible raw pointer Sdp::ptr and pass it to ffi. SessionWrapper::drop dereferences the user accessible raw pointer SessionWrapper::handle.

This is unsound because a user could safely modify those pointer fields and call on of the methods to cause UB.

[vincentfretin]

Did you see this comment? https://github.com/mozilla/janus-plugin-rs/blob/448d86ab62ac1f6f7b4c7f03cd0cf867e128a74d/src/sdp.rs#L135

What I understand from this comment is that Sdp::ptr is meant to be private, but needs to be public for the answer_sdp macro https://github.com/mozilla/janus-plugin-rs/blob/448d86ab62ac1f6f7b4c7f03cd0cf867e128a74d/src/sdp.rs#L324-L335

For SessionWrapper::handle https://github.com/mozilla/janus-plugin-rs/blob/448d86ab62ac1f6f7b4c7f03cd0cf867e128a74d/src/session.rs#L25 I don't know why it is public. We have a getter for it: https://github.com/mozilla/janus-plugin-rs/blob/448d86ab62ac1f6f7b4c7f03cd0cf867e128a74d/src/session.rs#L68-L70

But yeah this can surely cause Undefined Behavior (UB) if you modify the Sdp::ptr field or SessionWrapper::handle after creating the struct.

Did you encounter the issue in your own code using this lib or you were just reviewing this code?

[Freax13]

Did you see this comment?

https://github.com/mozilla/janus-plugin-rs/blob/448d86ab62ac1f6f7b4c7f03cd0cf867e128a74d/src/sdp.rs#L135

I did.

What I understand from this comment is that Sdp::ptr is meant to be private, but needs to be public for the answer_sdp macro

https://github.com/mozilla/janus-plugin-rs/blob/448d86ab62ac1f6f7b4c7f03cd0cf867e128a74d/src/sdp.rs#L324-L335

Couldn't you also use a getter for ptr in the answer_sdp macro? The macro doesn't seem to modify it, so I'm not sure why public access to the field is needed. Even public (unsafe) access was needed, you could still use an unsafe setter set_ptr or unsafe mutable reference getter ptr_mut.

Did you encounter the issue in your own code using this lib or you were just reviewing this code?

I was specifically looking for this kind of unsoundness issue in crates on crates.io and stumbled upon it.

[vincentfretin]

I think you're right, Sdp::ptr could be a getter here if we want to forbid modifying the field. The two projects that I know janus-plugin-sfu and janus-conference that use this lib doesn't access Sdp::ptr at all, they only use answer_sdp macro.

And for SessionWrapper::handle only janus-plugin-sfu is using session.handle to get the handle, and sometimes session.as_ptr(). They never set session.handle. janus-conference is only using session.as_ptr() as far as I can see.

So I guess that SessionWrapper::handle and Sdp::ptr could be made private and add a reference getter with the same name? https://users.rust-lang.org/t/public-getter-method-vs-pub-field/20147

[Freax13]

So I guess that SessionWrapper::handle and Sdp::ptr could be made private and add a reference getter with the same name? https://users.rust-lang.org/t/public-getter-method-vs-pub-field/20147

Yes, but I'd suggest returning the pointers by value instead of by reference.