JS_Eval UBSAN Error: Null pointer passed to memcpy

[andrjohns]

Programs with JS_Eval that are compiled with -fsanitize=undefined throw an error due to a null pointer being passed to memcpy in this line.

You can replicate using:

#include "quickjs.h"
#include <string.h>

int main() {
  JSRuntime* rt = JS_NewRuntime();
  JSContext* ctx = JS_NewContext(rt);

  const char* str = "1 + 2\0";
  JSValue val = JS_Eval(ctx, str, strlen(str), "", 0);


  JS_FreeValue(ctx, val);
  JS_FreeContext(ctx);
  JS_FreeRuntime(rt);

  return 0;
}

Compiled and called:

CFLAGS="-fsanitize=undefined" make libquickjs.a
gcc -fsanitize=undefined ubsan_test.c libquickjs.a -lm -o ubsan_test
./ubsan_test

Results in:

quickjs.c:33268:13: runtime error: null pointer passed as argument 2, which is declared to never be null

Reproduces on linux (also on debian:sid-slim docker container)

[bellard]

QuickJS may pass NULL pointers to memcpy with zero size. The C spec tells it is an undefined behavior but most C code do it, so the spec should be fixed instead.

[kenballus]

most C code do it

I'm not sure about this.

See these commits from ffmpeg:
FFmpeg/FFmpeg@63b3156
FFmpeg/FFmpeg@c54eef4
FFmpeg/FFmpeg@f077ad6
FFmpeg/FFmpeg@47b6ca0

And cpython:
python/cpython@1f95317
python/cpython@a3070d5

And curl:
curl/curl@b65f79d
curl/curl@7fb7f24

And openssl:
openssl/openssl@0760d13
openssl/openssl@eeab356
openssl/openssl@7eeceea
openssl/openssl@a925e7d

And qemu:
qemu/qemu@feba23b
qemu/qemu@0647d47
qemu/qemu@6b1de14

(These were just projects off the top of my head in which I searched "memcpy null" in the commit logs.)

GCC currently optimizes around the assumption that the src and dst arguments to memcpy are not NULL, which leads to unintuitive optimizations like this: (this is from gcc -O2 -fno-builtin)

#include <string.h>

int f(char *src) {
    memcpy(NULL, src, 0);
    return src == NULL; // Note that this gets optimized into "return 0"
}

f:
  sub rsp, 8
  mov rsi, rdi
  xor edx, edx
  xor edi, edi
  call memcpy
  xor eax, eax
  add rsp, 8
  ret

the spec should be fixed instead.

There is a proposal to change the standard here if you're interested in contributing.

[chqrlie]

@kenballus: the example is compelling! A blatant case of compiler perversion. I did not find such a potential problems in the source code, but we should probably either:

• find a proper flag to disable such optimisations for gcc and clang.
• use an inline wrapper for memcpy and/or memset that performs the test to prevent this optimisation at compile time.

[kenballus]

There is a flag to disable this optimization, but UB can manifest itself in strange ways, so I wouldn't be 100% confident that the flag eliminates all potential weird behavior.

OpenSSL (at least in some cases) uses wrappers: https://github.com/openssl/openssl/blob/62ecad5378067ab1f702ef2381c2f4a279d15250/ssl/ssl_asn1.c#L243