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

Small improvement: Make from_c_parts only visible inside crate #236

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jjj-vtm
Copy link

@jjj-vtm jjj-vtm commented Aug 27, 2024

Hi,

I made a (very) small PR to keep the from_c_parts private to the crate. The functions is very unsafe since it is possible to construct MQTTAsync_message from safe rust which will cause UB eg. set payload to point to unitialized memory.

Cheers,

Jan

@fpagliughi
Copy link
Contributor

Hey! Thanks for the PR.

I'll have a look at it, but actually there is a big update that I'm about to drop in a branch to start reworking the library to totally hide the C dependencies as the first stage in moving toward a 100% Rust implementation.

The initial update will be to rework the options data structures. At the moment, they contain private FFI elements that are geared toward the C API, with internal cached data like CString values that are pinned to send to the C lib. These will be replaced by "normal" Rust data structures with public types that are String, u16, Duration, etc.

The existing data structs will remain to perform the actual FFI, but be crate-visible only.

Hopefully this will make things a little nicer.

@jjj-vtm
Copy link
Author

jjj-vtm commented Aug 28, 2024

That sounds great, looking forward to have a look at the branch. If the PR is so to speak included in the next release you can just close it. I was only wondering why from_c_parts was publicly exposed because it is very easy to cause UB from safe rust with this function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants