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

Skyhook clobbers binary data #13

Open
shanipribadi opened this issue May 29, 2021 · 5 comments
Open

Skyhook clobbers binary data #13

shanipribadi opened this issue May 29, 2021 · 5 comments

Comments

@shanipribadi
Copy link

Skyhook stores values as Aerospike StringValue, using
ByteArray.toString(Charsets.UTF_8)

This causes binary data to be clobbered/corrupted when stored and read again.

I propose to store them as BytesValue instead, so that no conversion is done and data is stored as is.

this aligns with the specification of redis "string" as binary safe
https://redis.io/topics/data-types

@rbotzer
Copy link
Member

rbotzer commented May 30, 2021

The only possible binary data would be one that contains the null byte \0. Do you some code reproduction of how anything else is clobbered? That would be helpful.

@reugn
Copy link
Member

reugn commented May 31, 2021

@shanipribadi please add an example to reproduce the data corruption.

@rbotzer
Copy link
Member

rbotzer commented Jun 9, 2021

@reugn Shani reports that the problem is when he persists messagepacked data, it gets identified as a string and mangled a bit. Instead, it should be identified as a blob and saved as one.

@shanipribadi we still need some sample code, please.

@shanipribadi
Copy link
Author

shanipribadi commented Oct 13, 2021

Hello, apologies for the very late update

Please take a look at this repo: https://github.com/shanipribadi/skyhookcomp
it has the necessary test case to reproduce the issue.

it's implemented using the same library we're using in production (go-redis + msgpack).

You can try inserting \x83 or any other invalid utf-8 bytes, and you will see that skyhook+aerospike clobbers it, and converts it to \xef\xbf\xbd when returning the value again.

@reugn
Copy link
Member

reugn commented Oct 21, 2021

Thanks @shanipribadi. The issue has been resolved. Check out the main branch to have the fix.

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

No branches or pull requests

3 participants