`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: https://github.com/FFmpeg/FFmpeg/commit/63b31565584e7aaef9c43ca9c0c7a44c6cbc9e97 https://github.com/FFmpeg/FFmpeg/commit/c54eef46f990722ed65fd1ad1da3d0fc50806eb5 https://github.com/FFmpeg/FFmpeg/commit/f077ad69c682c13ab75a72aec11a61cac53f0c91 https://github.com/FFmpeg/FFmpeg/commit/47b6ca0b022a413e392707464f2423795aa89bfb

And cpython: https://github.com/python/cpython/commit/1f9531764cc0f8dbca1d8f429d162dc28282f4b4 https://github.com/python/cpython/commit/a3070d530c70477273cacbc61660b318582fff44

And curl: https://github.com/curl/curl/commit/b65f79d9e84277594f0df98ae4b997053176983d https://github.com/curl/curl/commit/7fb7f2413194e7f7a21716f86c765ad47c66fadf

And openssl: https://github.com/openssl/openssl/commit/0760d132da046063f6ac3c28bd2ee1d8505e6fcd https://github.com/openssl/openssl/commit/eeab356c298248108b82157ef51172ba040646f7 https://github.com/openssl/openssl/commit/7eeceeaab24aea16027cdc1f9df92366094893b7 https://github.com/openssl/openssl/commit/a925e7dbf4c3bb01365c961df86da3ebfa1a6c27

And qemu: https://github.com/qemu/qemu/commit/feba23b14340965b9dad9251ec9a7a47313fbf69 https://github.com/qemu/qemu/commit/0647d47cc184da587c76743546b6af6dfdb8f1da https://github.com/qemu/qemu/commit/6b1de1484ee2f80d5795358ea79e90c3ceb64167

(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