Make GetStringTableData native binary-safe#1232
Make GetStringTableData native binary-safe#1232asherkin merged 9 commits intoalliedmodders:masterfrom
Conversation
…binary compatible
asherkin
left a comment
asherkin
left a comment
There was a problem hiding this comment.
This looks good, thanks for fixing it!
I think the only thing here is the behaviour when the userdata is null - it would be a bit more user-friendly to return 0 in that case. There is a slight behaviour change where I think before the return value wouldn't have included the nul terminator, but I think we're fine to be making that change.
How about something like:
char *addr;
pContext->LocalToString(params[3], &addr);
size_t maxBytes = params[4];
if (userdata) {
memcpy(addr, userdata, MIN(numBytes, maxBytes));
} else if (maxBytes > 0) {
addr[0] = '\0';
}
return numBytes;
extensions/sdktools/vstringtable.cpp
Outdated
| char *addr; | ||
| pContext->LocalToString(params[3], &addr); | ||
|
|
||
| datalen = params[4]; |
There was a problem hiding this comment.
You don't want to do this:
datalenis the length of the real data the engine has.params[4]is the length of the buffer in the plugin.
So the MIN() before was the right way to calculate what to copy, it just had to use datalen rather than numBytes.
|
Do we have to worry about people relying on the return value to SetStringTableData being the bytes written or are we fine to change that behavior? |
|
@Headline we don't have to worry as the bug was already there, only the docs were faulty. So if we would change the behaviour to actually return the bytes written, I'd worry. |
It hasn't changed - as usual the docs were wrong. @thewavelength as it only returns not-1 when throwing an error, it should actually be |
3 participants
Replaced StringToLocalUTF8 with LocalToString and memcpy to make this binary-safe. Discussed this with @asherkin on Discord.
This probably won't hurt as it looks like the function was originally intended to be binary safe.
resolves #1230