C api bug (strings not null-terminated)

[starlitxiling]

When I use the C API, I find that some unpredictable id appear in read_dora_input_id, and it will be the same string I create after that. If I don't create it after that, it will be a segment of id plus a strange hexadecimal number (like BCAG). I think there is a pointer error phenomenon. I don't know if this is related to #540

[starlitxiling]

if you want to reproduce my problem, you can refer this:https://github.com/dora-rs/autoware.universe/tree/feature/autoware_dora/tools/read_id_test

[XxChang]

Because String does not contain \0 char (nul terminator) which is very different from C-string.

You can push a '\0' at this line to see how these been changed.

dora/libraries/core/src/config.rs

Lines 86 to 90 in 01c4047

	impl From<String> for DataId {
	fn from(id: String) -> Self {
	Self(id)
	}
	}

 impl From<String> for DataId { 
     fn from(id: String) -> Self { 
         let mut id = id;
         id.push('\0');
         Self(id) 
     } 
 }

[XxChang]

by the way, please use strncmp for possibly null-terminator string instead of strcmp

[starlitxiling]

 impl From<String> for DataId { 
     fn from(id: String) -> Self { 
         let mut id = id;
         id.push('\0');
         Self(id) 
     } 
 }

I tried this, but it fails when parsing dataflow.yml. I think pushing 0 directly will cause many problems.

[XxChang]

Or learn from
https://github.com/eclipse-zenoh/zenoh-c/blob/775e66586d105cbb42882ace58286f0cb2d967b2/src/commons.rs#L551-L568

I thing that let user release id-string is error-prone.

[phil-opp]

Only C uses null-terminated strings, all other languages (including C++) use length-terminated strings. So I don't think that we should force all other languages to reallocate their DataIds just so that things are more convenient for C.

The proper way to use strings returned by dora in C is to use length-aware functions (e.g. fwrite(id, id_len, 1, stdout)) . Alternatively, you can copy the ID into a new C-like string with null-termination (use malloc to allocate id_len + 1 bytes, then use strncpy` to copy the string).

If you're using the C-API from C++, you can simply do:

char *id_ptr;
size_t id_len;
read_dora_input_id(event, &id_ptr, &id_len);
std::string id(id_ptr, id_len);

I'm open to add convenience functions for these conversions to the API, but I don't think that we should change the From<String> for DataId implementation or do hidden mallocs.

What do you think?

[starlitxiling]

Thanks for your reply, I am not sure it is a good idea. When we get the Input_id in read_dora_input_id, we have already got a in unexpected input_id, like this:

So maybe user should specify the DataId len, such as if ( strncmp(id, "pointcloud",10) == 0) {}.

[starlitxiling]

And there is a interesting phenomenon. we have a node which input_id is "pointcloud" and output_id is "pointcloud_no_ground". When we check the input_id of this node, sometimes its input_id is "pointcloud_no_ground". If we change output_id from "pointcloud_no_ground" to "no_ground", sometimes its input_id turn into "pointcloud1B0C168F5ACDF".

So there is a problem with DataId, I think maybe this a problem about C Thread-Safety api #540

[haixuanTao]

By re-reading those message it seems @starlitxiling is not using:

std::string id(id_ptr, id_len);

Such that it looks like this:

char *id_ptr;
size_t id_len;
read_dora_input_id(event, &id_ptr, &id_len);
std::string id(id_ptr, id_len)

Could you try to add this to the code?

As philipp mentioned, this should fix your problem

[starlitxiling]

Ok，i will do this.

[haixuanTao]

This has been solved with:

char *id_ptr;
size_t id_len;
read_dora_input_id(event, &id_ptr, &id_len);
std::string id(id_ptr, id_len)