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
bad CString conversion corrupt data #86
Comments
I seems the bug is in the cpp driver itself: They ignore the value_length parameter and try to deduct the length looking for a NULL byte str terminator. Separately, the usage of the pointer for the name is not correct. The name reference should outlive the lifetime of the statement as the cpp driver stores the pointer. |
Oh no! Have you reported the CPP driver bug at https://datastax-oss.atlassian.net/jira/software/c/projects/CPP/issues/?filter=allissues&= ? I've found them moderately responsive, especially if you offer a patch. Thanks for the workaround. Please can you raise a separate issue for the usage problem? |
I made a PR and they merged it, but they haven't released it.
I will. |
Great! Leaving this issue open to track the Cassandra driver fix. Can you link the Cassandra issue? |
Issue is here: datastax/cpp-driver#484 |
Good catch on the bug. Should the code be rolled back to using a null terminated string until the cpp driver is fixed? Being correct is better than fast and broken.
I think this is ok. The name is used to calculate an index and the index is what is stored. So the name only needs to live for the statement->set() call inside of cass_statement_bind_string_by_name_n(), which it does. |
You're absolutely right. Good, we're safe then. |
I see there has still been no point release of the CPP driver :-(. Our master code has the workaround cassandra-rs/src/cassandra/statement.rs Line 610 in c243469
so cassandra-rs is safe - strings with NULs in will be truncated (unavoidably), but non-NUL-terminated strings are handled correctly.
This issue still remains open, to track DataStax releasing the fixed driver, and us (possibly) reverting the workaround. |
I looked for a version in the cpp driver library but couldn't find one. Since we don't know the library version we are executing on I guess we're stuck with the workaround for a very long time. Or maybe someone has a clever idea ? |
It's undocumented, but the C++ driver does expose a function |
This is fixed in 2.16.0 of the DataStax C/C++ driver - see datastax/cpp-driver@2.15.3...2.16.0 . @gwik do you want to investigate version detection? |
If I believe this: https://doc.rust-lang.org/nomicon/ffi.html it doesn't look good.
Am I missing something ? |
That's a bit of an overstatement. There's nothing magic about a C++ function. bindgen handles C++ to some degree: https://rust-lang.github.io/rust-bindgen/cpp.html and it should certainly cope with this - the only C++ thing about it is the name-mangling. The bigger problem is avoiding a link error if the symbol is absent; I'm not sure how to do that. |
Hi,
It looks like inserting data using
bind_by_name
was broken by the series of patch labeled as CString conversion optimizations.When inserting data, it looks the length of the string is incorrect and it writes subsequent memory to cassandra:
Value should be "Paul" but the following is inserted:
I bisect'ed with git with the following example (add to
examples/repro.rs) : https://gist.github.com/gwik/2d793da01a88cd6a5280f4a152f61a44
The first failing commit is cd5f02c. But it's already in version 0.15.1 and I the first occurence of this bug I found with 0.15.2-pre were fixed with 0.15.1 so I suppose that similar commits 8ca2f15 and f8793b6 have the same issue (introduced after 0.15.1).The text was updated successfully, but these errors were encountered: