allow keys with internal nul chars in rust bindings #220

gmossessian · 2021-05-10T23:06:45Z

Due to difference between Rust string and C string, it is possible to cause panic by submitting a string with a nul character in the middle to dictionary::get.

This PR returns empty match instead.

hendrikmuhs

@narekgharibyan is the maintainer of the C and rust binding.

However, I have reservations regarding the approach. The limitation is a bug in the rust binding, keyvi supports zero bytes. Returning an empty match as proposed silences a bug. I would understand if the rust binding errors in some way and this error is handled on client side without causing panic.

Would it be possible to provide an additional get method in the C-API that takes a pointer and a length? I saw a container called keyvi_bytes in the code used in a different context but basically implementing this approach.

hendrikmuhs · 2021-05-11T06:43:57Z

rust/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "keyvi"
-version = "0.5.3"
+version = "0.5.4"


this shouldn't be part of this PR.

We bump versions as part of a release, there are some more things to do for that (branching, tagging, ...)

Having that said, we can create a patch release soonish if you need the fix.

thank you, removed version bump in 5dec183

narekgharibyan · 2021-05-11T11:16:03Z

@gmossessian thanks for submitting this PR.

I agree with @hendrikmuhs that better approach to solve this would be to make use of keyvi_bytes and pass a binary representation of str down to keyvi cpp layer. Moreover this will also eliminate additional memory allocation of creating a CString.

Would it be possible to provide an additional get method in the C-API that takes a pointer and a length?

I think there is no need for additional API, and changing existing one is good enough. I don't think we really need backward compatibility here, but it's not hard to add an overload like:

struct keyvi_match* keyvi_dictionary_get(const struct keyvi_dictionary*, const keyvi_bytes);

@hendrikmuhs wdyt ?

And one semi related question.

keyvi itself does support arbitrary binary key, but rust API currently limits that to utf-8 strings by using str type.
On the other hand on python side we already have limitation of accepting only utf-8 strings as keys.

So I think it's fine to leave rust API as is for now.

hendrikmuhs · 2021-05-11T12:15:23Z

Would it be possible to provide an additional get method in the C-API that takes a pointer and a length?

I think there is no need for additional API, and changing existing one is good enough. I don't think we really need backward compatibility here, but it's not hard to add an overload like:
struct keyvi_match* keyvi_dictionary_get(const struct keyvi_dictionary*, const keyvi_bytes);
@hendrikmuhs wdyt ?

sounds good to me. I don't have strong feelings regarding keeping the old API. It depends on whether somebody uses the C-API for something else. I am not aware of such a case, neither do we advocate for it. Therefore I leave it up to you.

And one semi related question.

keyvi itself does support arbitrary binary key, but rust API currently limits that to utf-8 strings by using str type.
On the other hand on python side we already have limitation of accepting only utf-8 strings as keys.

Good point, I think the python API was designed to be as convenient as possible. Not too long ago we dropped python 2 support. That means string and binary handling became much easier and we could actually add support for byte objects, if we have a usecase for it.

narekgharibyan · 2021-05-11T12:26:49Z

Therefore I leave it up to you.

Then I'd say no need for a new one, and we can just change existing one.

That means string and binary handling became much easier and we could actually add support for byte objects, if we have a usecase for it.

Cool, then let's keep rust API part as is for now, and change it in the future if need be.

narekgharibyan · 2021-05-11T12:29:49Z

rust/tests/tests.rs

@@ -37,6 +37,14 @@ mod tests {
        assert_eq!(m.matched_string(), "a");
    }

+    #[test]
+   fn dictionary_get_nulerror() { 


@gmossessian seems this is the formatting issue, failing CI.

Note: you can format the by simply doing:

cargo fmt

gmossessian · 2021-05-11T19:22:53Z

Thanks for the discussion @hendrikmuhs and @narekgharibyan, I agree this is a better approach. I'll take a look at the library and re-request review when it's ready :)

~~@nararekgharibyan cargo fmt does not return any errors for me locally, furthermore I cannot see CI, I'm not sure it can be run on this PR without approval as I am a first-time contributor.~~ nvm

hendrikmuhs

sorry, it was my suggestion to use keyvi_bytes, actually we don't need it.

(FWIW: its odd that we still create a copy, because the the API requires a std::string, but lets ignore that for now, we can switch the dictionary getter to a string_view, but that requires C++17)

hendrikmuhs · 2021-05-12T18:22:44Z

keyvi/bin/keyvi_c/c_api.cpp

@@ -109,8 +109,12 @@ char* keyvi_dictionary_get_statistics(const keyvi_dictionary* dict) {
  return std_2_c_string(dict->obj_->GetStatistics());
 }

-keyvi_match* keyvi_dictionary_get(const keyvi_dictionary* dict, const char* key) {
-  return new keyvi_match(dict->obj_->operator[](key));
+keyvi_match* keyvi_dictionary_get(const keyvi_dictionary* dict, const keyvi_bytes key) {


now that I see the API, I realize we don't need to wrap and unwrap it:

keyvi_match* keyvi_dictionary_get(const keyvi_dictionary* dict, const char* key, size_t length)

good point! done in 6073fdd

hendrikmuhs

Thanks! LGTM

gmossessian · 2021-05-12T19:54:31Z

Thank you @hendrikmuhs! I'll wait for @narekgharibyan's final approval as well.

narekgharibyan

@gmossessian thanks for fixing it. LGTM!

gmossessian added 3 commits May 10, 2021 18:05

return empty match instead of NulError panic

6aa5c51

bump version

7b56d69

no newline for formatting

c0d6e7b

hendrikmuhs reviewed May 11, 2021

View reviewed changes

narekgharibyan requested changes May 11, 2021

View reviewed changes

remove version bumnp

5dec183

gmossessian added 2 commits May 11, 2021 14:23

rust fmt

deb9ee8

modify to allow internal nulchars

56d5a78

gmossessian changed the title ~~return empty match instead of NulError panic in rust dictionary binding~~ allow keys with internal nul chars in rust bindings May 12, 2021

cpp formatting

fda7dc8

gmossessian requested review from hendrikmuhs and narekgharibyan May 12, 2021 17:06

hendrikmuhs reviewed May 12, 2021

View reviewed changes

gmossessian added 2 commits May 12, 2021 14:08

fix cpplint errors

3445ca4

no need for keyvi_bytes intermediate

6073fdd

hendrikmuhs approved these changes May 12, 2021

View reviewed changes

narekgharibyan approved these changes May 13, 2021

View reviewed changes

narekgharibyan merged commit b6dcf45 into KeyviDev:master May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow keys with internal nul chars in rust bindings #220

allow keys with internal nul chars in rust bindings #220

gmossessian commented May 10, 2021

hendrikmuhs left a comment

hendrikmuhs May 11, 2021

gmossessian May 11, 2021

narekgharibyan commented May 11, 2021

hendrikmuhs commented May 11, 2021

narekgharibyan commented May 11, 2021

narekgharibyan May 11, 2021

gmossessian commented May 11, 2021 •

edited

Loading

hendrikmuhs left a comment

hendrikmuhs May 12, 2021

gmossessian May 12, 2021

hendrikmuhs left a comment

gmossessian commented May 12, 2021

narekgharibyan left a comment

allow keys with internal nul chars in rust bindings #220

allow keys with internal nul chars in rust bindings #220

Conversation

gmossessian commented May 10, 2021

hendrikmuhs left a comment

Choose a reason for hiding this comment

hendrikmuhs May 11, 2021

Choose a reason for hiding this comment

gmossessian May 11, 2021

Choose a reason for hiding this comment

narekgharibyan commented May 11, 2021

hendrikmuhs commented May 11, 2021

narekgharibyan commented May 11, 2021

narekgharibyan May 11, 2021

Choose a reason for hiding this comment

gmossessian commented May 11, 2021 • edited Loading

hendrikmuhs left a comment

Choose a reason for hiding this comment

hendrikmuhs May 12, 2021

Choose a reason for hiding this comment

gmossessian May 12, 2021

Choose a reason for hiding this comment

hendrikmuhs left a comment

Choose a reason for hiding this comment

gmossessian commented May 12, 2021

narekgharibyan left a comment

Choose a reason for hiding this comment

gmossessian commented May 11, 2021 •

edited

Loading