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?

janus-plugin-rs/src/sdp.rs

Line 135 in 448d86a

pub ptr: *mut RawSdp, // annoyingly pub because of answer_sdp macro

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

janus-plugin-rs/src/sdp.rs

Lines 324 to 335 in 448d86a

	macro_rules! answer_sdp {
	($sdp:expr $(, $param:expr, $value:expr)* $(,)*) => {
	unsafe {
	let result = $crate::sdp::generate_answer(
	$sdp.ptr,
	$($param, $value,)*
	$crate::sdp::OfferAnswerParameters::Done
	);
	$crate::sdp::Sdp::new(result).expect("Mysterious error generating SDP answer :(")
	}
	}
	}

For SessionWrapper::handle

janus-plugin-rs/src/session.rs

Line 25 in 448d86a

pub handle: *mut PluginSession,

I don't know why it is public. We have a getter for it:

janus-plugin-rs/src/session.rs

Lines 68 to 70 in 448d86a

	pub fn as_ptr(&self) -> *mut PluginSession {
	self.handle
	}

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?

janus-plugin-rs/src/sdp.rs

Line 135 in 448d86a

pub ptr: *mut RawSdp, // annoyingly pub because of answer_sdp macro

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

janus-plugin-rs/src/sdp.rs

Lines 324 to 335 in 448d86a

	macro_rules! answer_sdp {
	($sdp:expr $(, $param:expr, $value:expr)* $(,)*) => {
	unsafe {
	let result = $crate::sdp::generate_answer(
	$sdp.ptr,
	$($param, $value,)*
	$crate::sdp::OfferAnswerParameters::Done
	);
	$crate::sdp::Sdp::new(result).expect("Mysterious error generating SDP answer :(")
	}
	}
	}

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.