Allow scripts to support null characters again (#1984)

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]>

Author: madolson

src/eval.c

@@ -411,6 +411,7 @@ static int evalRegisterNewScript(client *c, robj *body, char **sha) {
         scriptingEngineCallCompileCode(engine,
                                        VMSE_EVAL,
                                        (sds)body->ptr + shebang_len,
+                                       sdslen(body->ptr) - shebang_len,
                                        0,
                                        &num_compiled_functions,
                                        &_err);

src/functions.c

@@ -1012,6 +1012,7 @@ sds functionsCreateWithLibraryCtx(sds code, int replace, sds *err, functionsLibC
         scriptingEngineCallCompileCode(engine,
                                        VMSE_FUNCTION,
                                        md.code,
+                                       sdslen(md.code),
                                        timeout,
                                        &num_compiled_functions,
                                        &compile_error);

src/lua/engine_lua.c

@@ -195,6 +195,7 @@ static compiledFunction **luaEngineCompileCode(ValkeyModuleCtx *module_ctx,
                                                engineCtx *engine_ctx,
                                                subsystemType type,
                                                const char *code,
+                                               size_t code_len,
                                                size_t timeout,
                                                size_t *out_num_compiled_functions,
                                                robj **err) {
@@ -207,7 +208,7 @@ static compiledFunction **luaEngineCompileCode(ValkeyModuleCtx *module_ctx,
     if (type == VMSE_EVAL) {
         lua_State *lua = lua_engine_ctx->eval_lua;
 
-        if (luaL_loadbuffer(lua, code, strlen(code), "@user_script")) {
+        if (luaL_loadbuffer(lua, code, code_len, "@user_script")) {
             sds error = sdscatfmt(sdsempty(), "Error compiling script (new function): %s", lua_tostring(lua, -1));
             *err = createObject(OBJ_STRING, error);
             lua_pop(lua, 1);

src/scripting_engine.c

@@ -217,21 +217,35 @@ static void engineTeardownModuleCtx(scriptingEngine *e) {
 compiledFunction **scriptingEngineCallCompileCode(scriptingEngine *engine,
                                                   subsystemType type,
                                                   const char *code,
+                                                  size_t code_len,
                                                   size_t timeout,
                                                   size_t *out_num_compiled_functions,
                                                   robj **err) {
     serverAssert(type == VMSE_EVAL || type == VMSE_FUNCTION);
-
+    compiledFunction **functions = NULL;
     engineSetupModuleCtx(engine, NULL);
+    if (engine->impl.methods.version == 1) {
+        functions = engine->impl.methods.compile_code_v1(
+            engine->module_ctx,
+            engine->impl.ctx,
+            type,
+            code,
+            timeout,
+            out_num_compiled_functions,
+            err);
+    } else {
+        /* Assume versions greater than 1 use updated interface. */
+        functions = engine->impl.methods.compile_code(
+            engine->module_ctx,
+            engine->impl.ctx,
+            type,
+            code,
+            code_len,
+            timeout,
+            out_num_compiled_functions,
+            err);
+    }
 
-    compiledFunction **functions = engine->impl.methods.compile_code(
-        engine->module_ctx,
-        engine->impl.ctx,
-        type,
-        code,
-        timeout,
-        out_num_compiled_functions,
-        err);
 
     engineTeardownModuleCtx(engine);
 

src/scripting_engine.h

@@ -55,6 +55,7 @@ ValkeyModule *scriptingEngineGetModule(scriptingEngine *engine);
 compiledFunction **scriptingEngineCallCompileCode(scriptingEngine *engine,
                                                   subsystemType type,
                                                   const char *code,
+                                                  size_t code_len,
                                                   size_t timeout,
                                                   size_t *out_num_compiled_functions,
                                                   robj **err);

src/valkeymodule.h

@@ -878,6 +878,8 @@ typedef struct ValkeyModuleScriptingEngineCallableLazyEvalReset {
  *
  * - `code`: string pointer to the source code.
  *
+ * - `code_len`: The length of the code string.
+ *
  * - `timeout`: timeout for the library creation (0 for no timeout).
  *
  * - `out_num_compiled_functions`: out param with the number of objects
@@ -889,6 +891,18 @@ typedef struct ValkeyModuleScriptingEngineCallableLazyEvalReset {
  * occurred.
  */
 typedef ValkeyModuleScriptingEngineCompiledFunction **(*ValkeyModuleScriptingEngineCompileCodeFunc)(
+    ValkeyModuleCtx *module_ctx,
+    ValkeyModuleScriptingEngineCtx *engine_ctx,
+    ValkeyModuleScriptingEngineSubsystemType type,
+    const char *code,
+    size_t code_len,
+    size_t timeout,
+    size_t *out_num_compiled_functions,
+    ValkeyModuleString **err);
+
+/* Version one of source code compilation interface. This API does not allow the compiler to
+ * safely handle binary data. You should use a newer version of the API if possible. */
+typedef ValkeyModuleScriptingEngineCompiledFunctionV1 **(*ValkeyModuleScriptingEngineCompileCodeFuncV1)(
     ValkeyModuleCtx *module_ctx,
     ValkeyModuleScriptingEngineCtx *engine_ctx,
     ValkeyModuleScriptingEngineSubsystemType type,
@@ -985,16 +999,18 @@ typedef ValkeyModuleScriptingEngineMemoryInfo (*ValkeyModuleScriptingEngineGetMe
     ValkeyModuleScriptingEngineSubsystemType type);
 
 /* Current ABI version for scripting engine modules. */
-#define VALKEYMODULE_SCRIPTING_ENGINE_ABI_VERSION 1UL
+#define VALKEYMODULE_SCRIPTING_ENGINE_ABI_VERSION 2UL
 
 typedef struct ValkeyModuleScriptingEngineMethods {
     uint64_t version; /* Version of this structure for ABI compat. */
 
     /* Compile code function callback. When a new script is loaded, this
      * callback will be called with the script code, compiles it, and returns a
      * list of `ValkeyModuleScriptingEngineCompiledFunc` objects. */
-    ValkeyModuleScriptingEngineCompileCodeFunc compile_code;
-
+    union {
+        ValkeyModuleScriptingEngineCompileCodeFuncV1 compile_code_v1;
+        ValkeyModuleScriptingEngineCompileCodeFunc compile_code;
+    };
     /* Function callback to free the memory of a registered engine function. */
     ValkeyModuleScriptingEngineFreeFunctionFunc free_function;
 

tests/modules/helloscripting.c

@@ -349,10 +349,12 @@ static ValkeyModuleScriptingEngineCompiledFunction **createHelloLangEngine(Valke
                                                                            ValkeyModuleScriptingEngineCtx *engine_ctx,
                                                                            ValkeyModuleScriptingEngineSubsystemType type,
                                                                            const char *code,
+                                                                           size_t code_len,
                                                                            size_t timeout,
                                                                            size_t *out_num_compiled_functions,
                                                                            ValkeyModuleString **err) {
     VALKEYMODULE_NOT_USED(module_ctx);
+    VALKEYMODULE_NOT_USED(code_len);
     VALKEYMODULE_NOT_USED(type);
     VALKEYMODULE_NOT_USED(timeout);
     VALKEYMODULE_NOT_USED(err);

tests/unit/scripting.tcl

@@ -2458,4 +2458,10 @@ start_server {tags {"scripting"}} {
             assert { [r memory usage foo] <= $expected_memory};
         }
     }
+
+    test {EVAL - Scripts support NULL byte} {
+        assert_equal [r eval "return \"\x00\";" 0] "\x00"
+        # Using a null byte never seemed to work with functions, so
+        # we don't have a test for that case.
+    }
 }