Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make GetStringTableData native binary-safe #1232

Merged
merged 9 commits into from
Apr 14, 2020

Conversation

thewavelength
Copy link
Contributor

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

Copy link
Member

@asherkin asherkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

char *addr;
pContext->LocalToString(params[3], &addr);

datalen = params[4];
Copy link
Member

@asherkin asherkin Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't want to do this:

  • datalen is 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.

@Headline
Copy link
Member

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?

@thewavelength
Copy link
Contributor Author

@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.

@asherkin
Copy link
Member

asherkin commented Apr 14, 2020

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?

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 void with no @return in the docs. int -> void would be a breaking change, but tbh I can't find anything using the return value anyway.

Copy link
Member

@Headline Headline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lgtm, thanks!

Copy link
Member

@asherkin asherkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, LGTM - thanks!

@asherkin asherkin merged commit 2546207 into alliedmodders:master Apr 14, 2020
@thewavelength thewavelength deleted the fix-getstringtabledata branch April 14, 2020 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetStringTableData uses string conversion function but can contain raw bytes
3 participants