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

Replace '\0' only when parsing key, not change data in value. #537

Closed
wants to merge 1 commit into from

Conversation

dota17
Copy link
Member

@dota17 dota17 commented Jan 21, 2020

Before the hash value is calculated, the replacement "\ 0" operation is placed first, and then the parsing of the value will preserve the original logic. #528

@hawicz
Copy link
Member

hawicz commented Apr 3, 2020

I really dislike these half-assed hacks to work around the problem of null bytes. It's inefficient, and it just replaces one problem with a new one. If you swap out a null byte with any kind of "magic value", you then introduce the problem of how to handle input with that magic value. As you can see in your test output, the re-serialized value is still wrong, just in a different way.

Yes, the current behavior is broken, but at least it's broken in a way that is understandable to a C programmer, namely that an embedded null byte will cause truncation of a string.

If you want to do something to improve the situation, sketch out what would be needed to start passing a key length down to json_object_object_add, and from there down to the linkhash functions like lh_get_hash. Exposing that in a public API would be quite intrusive, but perhaps we could at least get internal serialization code to handle it.

@hawicz
Copy link
Member

hawicz commented Jul 7, 2023

I won't be merging this PR. See #715 for a potential alternate solution (though even that one needs another look before merging)

@hawicz hawicz closed this Jul 7, 2023
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.

None yet

2 participants