workerd: windows-debug build fails string-test

[ohodson]

More test configurations for workerd were added this week in #1283.

The PR includes a few lines that remove the windows-debug configuration from the matrix in test.yml.

There is a test failure for windows-debug that repros with:

bazel test --config=debug //src/workerd/jsg:string-test

And for debugging purposes in VSCode through Run and debug -> workerd test case (dbg) -> bazel-bin/src/workerd/jsg/string-test. The test failure is:

[ TEST ] string-test.c++:12: UsvString from kj::String
[ PASS ] string-test.c++:12: UsvString from kj::String (159 ╬╝s)
[ TEST ] string-test.c++:369: JavaScript USVStrings
workerd/jsg/jsg-test.h:113: failed: expected *value == expectedValue [õ╝ÀÝÀØõ╝ÀÝÀØõ╝ÀÝÀØõ╝ÀÝÀØõ╝ÀÝÀØõ╝ÀÝÀØ == hello]; *value = õ╝ÀÝÀØõ╝ÀÝÀØõ╝ÀÝÀØõ╝ÀÝÀØõ╝ÀÝÀØõ╝ÀÝÀØ; expectedValue = hello
stack: 7ff614e15c70 7ff611eb53cb 7ff611e8f26f 7ff611e8e7f3 7ff614cbf87f 7ff614cbe5ef 7ff614cbdbf9 7ff614cbd7e4 7ff614cbd7a9 7ff614cbd773 7ff614e6a371 7ff614e65c4b 7ff614e7421c 7ff614e6ae4a 7ff614e67a44 7ff614e6242f 7ff614e6222c 7ff614cbab3e 7ff61537dc38 7ff61537dd6d 7ff61537dded 7ff61537de0d 7fff1ca3257c 7fff1de8aa77
C:\tmp\rp4ezqc4\execroot\workerd\external\capnp-cpp\src\kj\debug.c++:355
C:\tmp\rp4ezqc4\execroot\workerd\bazel-out\x64_windows-dbg\bin\external\capnp-cpp\src\kj\_virtual_includes\kj\kj\debug.h:527
C:\tmp\rp4ezqc4\execroot\workerd\src\workerd\jsg\jsg-test.h:113
C:\tmp\rp4ezqc4\execroot\workerd\src\workerd\jsg\string-test.c++:373
C:\tmp\rp4ezqc4\execroot\workerd\external\capnp-cpp\src\kj\test.c++:318
C:\tmp\rp4ezqc4\execroot\workerd\external\capnp-cpp\src\kj\exception.h:339
C:\tmp\rp4ezqc4\execroot\workerd\external\capnp-cpp\src\kj\test.c++:318
C:\tmp\rp4ezqc4\execroot\workerd\external\capnp-cpp\src\kj\test.c++:217
C:\tmp\rp4ezqc4\execroot\workerd\external\capnp-cpp\src\kj\function.h:263
...

The point of failure is string-test.c++:373. This is the only test that uses testUsvPtr.

An initial suspicion was that this could be attributable to differences in compiler macros, but no evidence supports this for now. The notable differences are the addition of /DDEBUG /DV8_ENABLE_CHECKS to the V8 build flags and changing from /MD to /MDd.

[ohodson]

Additional note: it is possible to use CodeLLDB on Windows which opens up the possibility of using the V8 lldb debugger extensions. Install CodeLLDB, then add a new target to launch.json:

+    {
+      "name": "string-test (dbg)",
+      "preLaunchTask": "Bazel build all (dbg)",
+      "program": "${workspaceFolder}/bazel-bin/src/workerd/jsg/string-test",
+      "cwd": "${workspaceFolder}",
+      "type": "lldb",
+      "initCommands": ["command script import lldb_commands.py"],
+      "request": "launch"
+    },

Then copy lldb_commands.py from v8 into the workerd workspace. YMMV with this, it seems a bit flakey in practice, but I have flagged it because CodeLLDB is typically better than plain LLDB support in VSCode for Linux / macOS.

The code on the stack is fairly compact / terse with generated code (probably interpreter) in the mix too.

[jasnell]

Looks like a problem with the way UTF8 characters in the source are interpreted on Windows. That said, I'm in the process of phasing out jsg::UsvString. Once the ada-url update lands and we're sure that's all good to go we ought to be able to delete jsg::UsvString and related tests entirely

[ohodson]

Thanks James. I don't dispute the suggestion, but observe that the test only fails for debug builds and the line failing does not encompass anything special in UTF-8:

e.expectEval("testUsvPtr('hello')", "string", "hello");

The preceding line passes:

e.expectEval("testUsv('hello')", "string", "hello");

Looking in the debugger, there is a minor iceberg code contained within the trivial looking function testUstPtr():

  UsvStringPtr testUsvPtr(jsg::Optional<UsvString> str) {
    return kj::mv(str).orDefault(usv("undefined"));
  }

Near the end of the code called here is a call to UsvString::operator UsvStringPtr() which calls the private UsvStringPtr::UsvStringPtr(kj::ArrayPtr<uint32_t> ptr). It looks like the underlying storage is held by str, but now referred to by the ArrayPtr in the UsvStringPtr. As the function exits, str is destructed, releasing the array. The Windows debug build overwrites the released memory with bytes containing 0xdd and exposes the issue.

Looks like this works via UAF on our other builds and we might be able to fix this with an ownership transfer of the memory or keeping the memory around longer. 😨

[ohodson]

Looks like this works via UAF on our other builds and we might be able to fix this with an ownership transfer of the memory or keeping the memory around longer. 😨

Hmm, I'm not so sure there is a good route for this given the method signature:

  UsvStringPtr testUsvPtr(jsg::Optional<UsvString> str) {
    return kj::mv(str).orDefault(usv("undefined"));
  }

The str argument holds the memory and is passed by value so will invoke the destructor for str at the end of the method. The templating used to bind testUsvPtr through type-wrapper does not work with a reference argument so simply changing the input str to a reference:

  UsvStringPtr testUsvPtr(jsg::Optional<UsvString>& str) {
    return kj::mv(str).orDefault(usv("undefined"));
  }

barfs during compilation:

In file included from src/workerd/jsg/string-test.c++:5:
In file included from src/workerd/jsg/jsg-test.h:9:
In file included from src/workerd/jsg/setup.h:10:
src/workerd/jsg/type-wrapper.h(489,14): error: non-const lvalue reference to type 'Optional<...>' cannot bind to a temporary of type 'Optional<...>'
      return kj::fwd<RemoveMaybe<decltype(maybe)>>(result);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/workerd/jsg/type-wrapper.h(532,14): note: in instantiation of function template specialization 'workerd::jsg::TypeWrapper<workerd::jsg::test::(anonymous namespace)::UsvStringIsolate_TypeWrapper, workerd::jsg::DOMException, workerd::jsg::test::(anonymous namespace)::UsvStringContext>::unwrap<workerd::jsg::Optional<workerd::jsg::UsvString> &>' requested here
      return unwrap<U>(context, args[parameterIndex], errorContext);  
             ^
src/workerd/jsg/resource.h(195,30): note: in instantiation of function template specialization 'workerd::jsg::TypeWrapper<workerd::jsg::test::(anonymous namespace)::UsvStringIsolate_TypeWrapper, workerd::jsg::DOMException, workerd::jsg::test::(anonymous namespace)::UsvStringContext>::unwrap<workerd::jsg::Optional<workerd::jsg::UsvString> &>' requested here
            wrapper.template unwrap<Args>(context, args, indexes,     

In conclusion, I'm not able to see a good fix for this other than dropping the test. If we think this pattern might occur in real code, we'll need a better fix.