AsyncClient::subscribe_many has UB if topics.len() > qos.len()

[Arnavion]

paho.mqtt.rust/src/async_client.rs

Lines 878 to 896 in cf953b9

	let n = topics.len();
	let ver = self.mqtt_version();
	// TOOD: Make sure topics & qos are same length (or use min)
	let tok = Token::from_request(None, ServerRequest::SubscribeMany(n));
	let mut rsp_opts = ResponseOptions::new(ver, tok.clone());
	let topics = StringCollection::new(topics);
	debug!("Subscribe to '{:?}' @ QOS {:?}", topics, qos);
	let rc = unsafe {
	ffi::MQTTAsync_subscribeMany(
	self.inner.handle,
	n as c_int,
	topics.as_c_arr_mut_ptr(),
	qos.as_ptr(),
	&mut rsp_opts.copts,
	)
	};

The Rust code passes in topics.len() as the count parameter of MQTTAsync_subscribeMany, which then assumes that qos also has the same number of elements. If the caller passes in a smaller slice of qos, then the C library will read past the end of the slice.

Since the function is infallible, you could at least add an assert like:

 debug!("Subscribe to '{:?}' @ QOS {:?}", topics, qos);
 
+assert_eq!(topics.len(), qos.len(), "number of `qos` must be equal to number of `topics`");
+
 let rc = unsafe {
     ffi::MQTTAsync_subscribeMany(

[fpagliughi]

Yeah, that's just not a good API. I don't think an assert is the best. Maybe debug_assert. But at least it should return an error if their lengths don't match.

But really, the function should take them as pairs, like:

pub fn subscribe_many<T: AsRef<str>>(&self, topics_qos: &[(T, i32)]) -> SubscribeManyToken { ... }

Right? Maybe at the next breaking change update?

Although, 90% of the time that I use this function, I give all the topics the same QoS. So perhaps a version that takes multiple topics bust just one QoS value:

pub fn subscribe_many<T: AsRef<str>>(&self, topics: &[T], qos: i32) -> SubscribeManyToken { ... }