Skip to content

[BUG] Valkey 8.1 does not accept null bytes in script source code #1942

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

Closed
gmbnomis opened this issue Apr 11, 2025 · 2 comments · Fixed by #1984
Closed

[BUG] Valkey 8.1 does not accept null bytes in script source code #1942

gmbnomis opened this issue Apr 11, 2025 · 2 comments · Fixed by #1984
Assignees

Comments

@gmbnomis
Copy link
Contributor

gmbnomis commented Apr 11, 2025

Describe the bug

Since commit b58088e "Adds support for running EVAL with different scripting engines (#1497)" Valkey
does not support null bytes in scripts anymore. (To be clear: this is not about handling strings with null bytes in scripts. It is about a script's source code containing null bytes)

To reproduce

In valkey-cli:

127.0.0.1:6379> eval "return '\\0';" 0
"\x00"
127.0.0.1:6379> eval "return '\x00';" 0
(error) ERR Error compiling script (new function): user_script:1: unfinished string near '<eof>'

Expected behavior

Prior to this change, a lua embedded zero and a real null byte both work:

127.0.0.1:6379> eval "return '\\0';" 0
"\x00"
127.0.0.1:6379> eval "return '\x00';" 0
"\x00"

Additional information

It is unclear to me whether this is an intentional change (that was not described in the release nodes)

(For us, it is not a required use case, but it broke one of our tests. We intended to use a lua zero in a string literal and because of the library we use, this ended up becoming a null byte)

@hpatro
Copy link
Collaborator

hpatro commented Apr 11, 2025

Tagging @rjd15372 to check if anything changed with the refactoring.

@madolson
Copy link
Member

I was thinking it might be related to the fact the "code" object is a c string, which doesn't handle binary data. I suggested a patch because it would be a module breaking change, so we should prioritize it for the upcoming patch release if possible.

@github-project-automation github-project-automation bot moved this from Todo to Done in Valkey 8.1 Apr 22, 2025
murphyjacob4 pushed a commit to murphyjacob4/valkey that referenced this issue Apr 22, 2025
We made a change during the refactoring of the engine changes that used
a strlen on an SDS string. SDS are resizable, but they are binary safe
and support NULL characters, which means we are truncating the string at
the null character.

This is a breaking change for modules, so wanted to raise it for the
upcoming patch.

An alternative is we could expose the script code as a server object,
which would require some construction of the object in the function
pathway and freeing. I opted to keep the code flow as similar as
possible for the patch.

There is a test which only passes now.

For some reasons, functions don't support binary data. So as of right
now this patch doesn't fix that case.

Resolves valkey-io#1942

---------

Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Jacob Murphy <[email protected]>
murphyjacob4 pushed a commit to murphyjacob4/valkey that referenced this issue Apr 23, 2025
We made a change during the refactoring of the engine changes that used
a strlen on an SDS string. SDS are resizable, but they are binary safe
and support NULL characters, which means we are truncating the string at
the null character.

This is a breaking change for modules, so wanted to raise it for the
upcoming patch.

An alternative is we could expose the script code as a server object,
which would require some construction of the object in the function
pathway and freeing. I opted to keep the code flow as similar as
possible for the patch.

There is a test which only passes now.

For some reasons, functions don't support binary data. So as of right
now this patch doesn't fix that case.

Resolves valkey-io#1942

---------

Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Jacob Murphy <[email protected]>
zuiderkwast pushed a commit that referenced this issue Apr 23, 2025
We made a change during the refactoring of the engine changes that used
a strlen on an SDS string. SDS are resizable, but they are binary safe
and support NULL characters, which means we are truncating the string at
the null character.

This is a breaking change for modules, so wanted to raise it for the
upcoming patch.

An alternative is we could expose the script code as a server object,
which would require some construction of the object in the function
pathway and freeing. I opted to keep the code flow as similar as
possible for the patch.

There is a test which only passes now.

For some reasons, functions don't support binary data. So as of right
now this patch doesn't fix that case.

Resolves #1942

---------

Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Jacob Murphy <[email protected]>
hwware pushed a commit to wuranxx/valkey that referenced this issue Apr 24, 2025
We made a change during the refactoring of the engine changes that used
a strlen on an SDS string. SDS are resizable, but they are binary safe
and support NULL characters, which means we are truncating the string at
the null character.

This is a breaking change for modules, so wanted to raise it for the
upcoming patch.

An alternative is we could expose the script code as a server object,
which would require some construction of the object in the function
pathway and freeing. I opted to keep the code flow as similar as
possible for the patch.

There is a test which only passes now.

For some reasons, functions don't support binary data. So as of right
now this patch doesn't fix that case.

Resolves valkey-io#1942

---------

Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: hwware <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants