Skip to content

JS_NewRuntime gives GCC-UBSAN error with bounds-checking #928

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
andrjohns opened this issue Feb 24, 2025 · 7 comments · Fixed by #930
Closed

JS_NewRuntime gives GCC-UBSAN error with bounds-checking #928

andrjohns opened this issue Feb 24, 2025 · 7 comments · Fixed by #930

Comments

@andrjohns
Copy link
Contributor

I'm getting an odd error when compiling quickjs with gcc -fsanitize=bounds-strict (newly required as part of R's packaging checks) and calling JS_NewRuntime():

quickjs.c:2946:14: runtime error: index 4 out of bounds for type 'uint8_t [*]'

It can be reproduced with the debian:sid-slim docker image with default gcc (gcc 14), using dummy program:

#include "cutils.c"
#include "libbf.c"
#include "libregexp.c"
#include "libunicode.c"
#include "quickjs.c"

int main() {
  JSRuntime* rt = JS_NewRuntime();
  JS_FreeRuntime(rt);
  return 0;
}

Compiled with:

~/quickjs# gcc -fsanitize=bounds-strict -D_GNU_SOURCE -std=gnu11 -funsigned-char gcc_ubsan_test.c -lm -o gcc_ubsan_test
~/quickjs# ./gcc_ubsan_test 
quickjs.c:2946:14: runtime error: index 4 out of bounds for type 'uint8_t [*]'

The error is pointing to this function:

// XXX: `str` must be pure ASCII. No UTF-8 encoded strings
// XXX: `str` must not be the string representation of a small integer
static JSAtom __JS_NewAtomInit(JSRuntime *rt, const char *str, int len,
                               int atom_type)
{
    JSString *p;
    p = js_alloc_string_rt(rt, len, 0);
    if (!p)
        return JS_ATOM_NULL;
    memcpy(p->u.str8, str, len);
    p->u.str8[len] = '\0';
    return __JS_NewAtom(rt, p, atom_type);
}

And specifically the indexing:

p->u.str8[len] = '\0';

Any chance that there's a simple/obvious fix here? Thanks!

@andrjohns
Copy link
Contributor Author

andrjohns commented Feb 24, 2025

I'm guessing the issue is the string alloc and the memcpy size aren't accounting for the null-terminator (being assigned at position len+1)?

@saghul
Copy link
Contributor

saghul commented Feb 24, 2025

I think it's because of how JSString is defined:

struct JSString {
    JSRefCountHeader header; /* must come first, 32-bit */
    uint32_t len : 31;
    uint8_t is_wide_char : 1; /* 0 = 8 bits, 1 = 16 bits characters */
    /* for JS_ATOM_TYPE_SYMBOL: hash = 0, atom_type = 3,
       for JS_ATOM_TYPE_PRIVATE: hash = 1, atom_type = 3
       XXX: could change encoding to have one more bit in hash */
    uint32_t hash : 30;
    uint8_t atom_type : 2; /* != 0 if atom, JS_ATOM_TYPE_x */
    uint32_t hash_next; /* atom_index for JS_ATOM_TYPE_SYMBOL */
    JSWeakRefRecord *first_weak_ref;
#ifdef ENABLE_DUMPS // JS_DUMP_LEAKS
    struct list_head link; /* string list */
#endif
    union {
        __extension__ uint8_t str8[0]; /* 8 bit strings will get an extra null terminator */
        __extension__ uint16_t str16[0];
    } u;
};

The last element is a flexible array. I wonder if UBSAN will be appeased if we make it size [1] instead?

@bnoordhuis
Copy link
Contributor

I'm guessing the issue is the string alloc and the memcpy size aren't accounting for the null-terminator (being assigned at position len+1)?

It's a false positive. There's no actual write-after-end; js_alloc_string_rt allocates len+1 bytes.

@andrjohns
Copy link
Contributor Author

Ah thanks both, yeah it looks like a false-positive caused by the use of [0] to denote a flexible array member. Here's a minimal godbolt reprex for the behaviour: https://godbolt.org/z/Gnv8eThYr

@andrjohns
Copy link
Contributor Author

Looks like fsanitize=bounds-strict intentionally only recognises [] as a flexible array member: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113304

I'm not sure if this can even be handled from the quickjs side, since a union of [] is not allowed (as used in JSString):

/home/andrew/Git/quickjs/quickjs.c:488:31: error: flexible array member in union
  488 |         __extension__ uint8_t str8[];

bnoordhuis added a commit to bnoordhuis/quickjs that referenced this issue Feb 25, 2025
It's been reported that UBSan's `-fsanitize=bounds-strict` does not
like empty arrays. Remove them and replace their uses with old school
pointer arithmetic.

Fixes: quickjs-ng#928
@bnoordhuis
Copy link
Contributor

Yeah, I don't think we can easily fix that. As you mention, we can't use []. We can switch to [1] but I understand that won't appease ubsan either.

I've opened #930 where I remove the union and replace uses of str->u.str8 and str->u.str16 with, essentially, (uint8_t *)&str[1] and (uint16_t *)&str[1]. I'll let @saghul be the judge of whether it's an improvement.

bnoordhuis added a commit that referenced this issue Feb 25, 2025
It's been reported that UBSan's `-fsanitize=bounds-strict` does not
like empty arrays. Remove them and replace their uses with old school
pointer arithmetic.

Fixes: #928
@andrjohns
Copy link
Contributor Author

Fantastic, thanks so much for reworking all of that handling so quickly - it's very much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants