Pass char* to Go and convert with C.GoString, avoiding unsafe use #288
Conversation
Using the char* passed by Mosquitto in a GoString struct is unsafe. The memory it points to is managed by Mosquitto, but Go will keep the pointer around for an indefinite duration, even when Mosquitto might free the memory. By passing the actual char* to Go, we can use C.GoString to convert it, which copies the bytes into a buffer managed by Go. That way we can use it safely at any time in the future.
|
I encountered this issue in mosquitto-go-auth-oauth2, which keeps a cache keyed by the username. The cache is used to store access and refresh tokens acquired at login for future queries to the userinfo endpoint. With long lived sessions this cache can become corrupted. It is possible to work around it in mosquitto-go-auth-oauth2 by passing the cache keys through strings.Clone, but fixing it at the source seems to be the correct solution. |
|
Thanks, @ssiegel, I'll take a look during this week. |
| GoString go_clientid = {clientid, strlen(clientid)}; | ||
| GoString go_username = {username, strlen(username)}; | ||
| GoString go_topic = {topic, strlen(topic)}; | ||
| GoInt32 go_access = access; |
There was a problem hiding this comment.
This is the one thing that caught my eye.
Why pass acc directly and accept a C.int on AuthAclCheck instead of defining a GoInt32 as we were doing before?
It's more surprising when you don't give the same treatment to opts_count, also defined as a GoInt32 and passed along to AuthPluginInit, which receives a Go int instead of a C.int.
There was a problem hiding this comment.
I don't think it really matters technically. I changed this in AuthAclCheck for mostly aesthetical reasons, to consistently have C types as parameters. AuthPluginInit/opts_count is an oversight. Would you prefer to use C.int there as well, or should I revert the acc parameter of AuthAclCheck to GoInt32?
There was a problem hiding this comment.
I don't have a preference really, was just asking if there was a particular reason to treat them differently.
Since there's not, I think we're good to merge.
Thanks!
Using the char* passed by Mosquitto in a GoString struct is unsafe. The memory it points to is managed by Mosquitto, but Go will keep the pointer around for an indefinite duration, even when Mosquitto might free the memory.
By passing the actual char* to Go, we can use C.GoString to convert it, which copies the bytes into a buffer managed by Go. That way we can use it safely at any time in the future.