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

Remove nul chars from string before parsing to numerical values #688

Merged

Conversation

KowalczykBartek
Copy link
Contributor

Looks like this change 353812f is causing following error (when SimpleConsumer is being run) - at least on OSX

librdkafka validated config value is valid u64: ParseIntError { kind: InvalidDigit }

That is because rd_kafka_conf_get returns null terminated string,

// Allocate a buffer of that size and call again to get the actual
// string.
let mut buf = vec![0_u8; size];
let res = unsafe {
    rdsys::rd_kafka_conf_get(
        self.ptr(),
        key_c.as_ptr(),
        buf.as_mut_ptr() as *mut c_char,
        &mut size,
    )
};

ends with buf equal:

[51, 48, 48, 48, 48, 48, 0];

I guess reverting this change will cause issues on windows, so maybe my PR will be good enough to handle this.

This pr should also fix this case #683

@chklauser
Copy link

Just as a heads up for maintainers: there seem to be 3 PRs for this issue

  1. Use CStr::from_bytes_until_nul to read rdkafka config #671
  2. Remove nul chars from string before parsing to numerical values #688 (this PR)
  3. Fix null characters in configuration values #691

@fede1024 fede1024 merged commit 036e0bb into fede1024:master Aug 3, 2024
@mati865
Copy link

mati865 commented Aug 6, 2024

This PR feels like a workaround that is doomed to break in the future. Aby chance to revisit #671?

@fede1024
Copy link
Owner

fede1024 commented Aug 6, 2024

I agree that #671 is a better solution. @mati865 any chance you could file a PR with from_bytes_until_nul? #671 has a few other changes that are conflicting with the master branch. Thanks

@mati865
Copy link

mati865 commented Aug 6, 2024

I'll wait for @chklauser to reply if he wants to revive his PR, otherwise I'll rebase hit work.

@chklauser
Copy link

I have rebased the change as #705 (forgot to re-open before pushing the new branch). With the unrelated bump of MSRV to 1.70 we can now just use CStr::from_bytes_until_nul 🥳

Thanks for tagging me @mati865

@andrei-ionescu
Copy link

andrei-ionescu commented Aug 7, 2024

I also created #706 for this issue, not knowing that there is this. Not sure which one would get merged to fix the problem.

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.

5 participants