search

org-arl / fjage

Framework for Java and Groovy Agents
https://fjage.readthedocs.io/en/latest/
Other
26 stars 13 forks source link

Fix get_param_str in fjage.c #264

Closed notthetup closed 1 year ago

notthetup commented 1 year ago

This is a bug fix for a bug in fjage.c where fjage_get_param_str causes a null pointer to be returned even if there is a valid string available.

The reason is :

https://github.com/org-arl/fjage/blob/master/gateways/c/src/fjage.c#L1365..L1366 v = fjage_msg_get_string(msg, "value"); v is a pointer into the message's body to the string.. And then we free the message body with fjage_msg_destroy(msg); So v is now pointing into freed memory.

The fix is to return a copy of the string instead.

mchitre commented 1 year ago

This is a BREAKING change, since it changes the API and leads to memory leaks for all previous users of fjåge! We need to tag the release as such and mention it clearly in the release notes. Are there any other APIs we need to break? If so, we should do it at the same time.

notthetup commented 1 year ago

True. I am not sure if there are any other breaking changes pending for the fjage.c Gateway.

P.S. Also happy to change this fix in a way that doesn't break the API, but I couldn't think of any other approach.

mchitre commented 1 year ago

Wondering if it makes more sense to have an array-style API where a pointer to a buffer & length is passed in, and if there is enough space, the string is copied into it. If NULL is passed in as the buffer, the length of the necessary buffer is returned.

Not necessarily what I'm advocating, but just thinking aloud.

notthetup commented 1 year ago

I do prefer that style of API too. It makes memory ownership more transparent. But that'll also be a breaking change.

Also, if that's the approach we're going for, we should use that style across the board for other APIs which return pointers etc.

mchitre commented 1 year ago

It's better breaking, in the sense that it'll fail at compilation and force a user to make changes with understanding. The other breaking is dangerous as the compilation will succeed and they'll get memory leaks silently.

notthetup commented 1 year ago

If const char* strval = NULL, then ideally it shouldn't copy ANY characters into the buffer. Similarly if int len=0. In such a scenario, should it return 0?

That way one can use fjage_param_get_string to check if a string exists but doesn't need to make/pass a buffer to read out the value.

mchitre commented 1 year ago

It should return the actual length so user knows what size buffer needs to be passed in.