Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Arnavion opened this issue Dec 6, 2023 · 1 comment
Open

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

Arnavion opened this issue Dec 6, 2023 · 1 comment
Labels
enhancement fix added A fix was added to an unreleased branch
Milestone

Comments

@Arnavion
Copy link

Arnavion commented Dec 6, 2023

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(
@Arnavion Arnavion changed the title AsyncClient::susbcribe_many has UB if topics.len() > qos.len() AsyncClient::subscribe_many has UB if topics.len() > qos.len() Dec 6, 2023
@fpagliughi
Copy link
Contributor

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

@fpagliughi fpagliughi added this to the v0.13 milestone May 25, 2024
@fpagliughi fpagliughi added the fix added A fix was added to an unreleased branch label May 25, 2024
@fpagliughi fpagliughi modified the milestones: v0.13, v0.12.5 May 25, 2024
fpagliughi added a commit that referenced this issue May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement fix added A fix was added to an unreleased branch
Projects
None yet
Development

No branches or pull requests

2 participants