eclipse / paho.mqtt.rust

paho.mqtt.rust
Other
511 stars 102 forks source link

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

Open Arnavion opened 7 months ago

Arnavion commented 7 months ago

https://github.com/eclipse/paho.mqtt.rust/blob/cf953b937eaa66c436e746dcfde98d2bf61b36ac/src/async_client.rs#L878-L896

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 commented 2 months ago

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 { ... }