Skip to content

Add last_var_atom for better error messages #1382

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.

Sign up for GitHub

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

Open
speedskater1610 wants to merge 8 commits into
base: master
Choose a base branch
from
+156 −45
Open

Add last_var_atom for better error messages #1382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3deee48
Add last_var_atom for better error messages
speedskater1610 Mar 4, 2026
bcf06fe
Attempt to fix the build and remove some debugging I forgot to remove
speedskater1610 Mar 4, 2026
3c5a9c5
Add range validation & Protect all opcode path for test262
speedskater1610 Mar 5, 2026
a62a0d9
fix use-after-free bug in JS_CallInternal with last_var_atom; & remov…
speedskater1610 Mar 5, 2026
a838408
Undo changes
speedskater1610 Mar 7, 2026
5973868
Create test1231.js
speedskater1610 Mar 7, 2026
541dbd3
Fix CI
speedskater1610 Mar 10, 2026
5f6d8ab
fix OP_get_loc8 missing stack push and pc advance
speedskater1610 Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 154 additions & 45 deletions quickjs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ struct JSRuntime {
void *user_opaque;
void *libc_opaque;
JSRuntimeFinalizerState *finalizers;
JSAtom last_var_atom; /* stores the last accessed variable atom for better error messages */
};

struct JSClass {
Expand Down Expand Up @@ -535,6 +536,7 @@ struct JSContext {
const char *input, size_t input_len,
const char *filename, int line, int flags, int scope_idx);
void *user_opaque;
JSAtom last_var_atom; /* stores the last accessed variable atom for better error messages */
};

typedef union JSFloat64Union {
Expand Down Expand Up @@ -1989,6 +1991,7 @@ JSRuntime *JS_NewRuntime2(const JSMallocFunctions *mf, void *opaque)
JS_UpdateStackTop(rt);

rt->current_exception = JS_UNINITIALIZED;
rt->last_var_atom = JS_ATOM_NULL;

return rt;
fail:
Expand Down Expand Up @@ -8058,6 +8061,17 @@ static JSValue JS_ThrowTypeErrorNotAFunction(JSContext *ctx)
return JS_ThrowTypeError(ctx, "not a function");
}

static JSValue JS_ThrowTypeErrorNotAFunctionAtom(JSContext *ctx, JSAtom atom)
{
if (atom == JS_ATOM_NULL) {
/* fallback if no atom provided */
return JS_ThrowTypeError(ctx, "not a function");
}
char buf[ATOM_GET_STR_BUF_SIZE];
JS_AtomGetStr(ctx, buf, sizeof(buf), atom);
return JS_ThrowTypeError(ctx, "%s is not a function", buf);
}

static JSValue JS_ThrowTypeErrorNotAnObject(JSContext *ctx)
{
return JS_ThrowTypeError(ctx, "not an object");
Expand Down Expand Up @@ -17294,6 +17308,7 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
JSValue *local_buf, *stack_buf, *var_buf, *arg_buf, *sp, ret_val, *pval;
JSVarRef **var_refs;
size_t alloca_size;
JSAtom saved_last_var_atom; /* save and restore last_var_atom to avoid use-after-free */

#ifdef ENABLE_DUMPS // JS_DUMP_BYTECODE_STEP
#define DUMP_BYTECODE_OR_DONT(pc) \
Expand All @@ -17320,6 +17335,9 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
#define BREAK SWITCH(pc)
#endif

/* Save last_var_atom to avoid use-after-free bugs */
saved_last_var_atom = rt->last_var_atom;

if (js_poll_interrupts(caller_ctx))
return JS_EXCEPTION;
if (unlikely(JS_VALUE_GET_TAG(func_obj) != JS_TAG_OBJECT)) {
Expand Down Expand Up @@ -17354,7 +17372,12 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
call_func = rt->class_array[p->class_id].call;
if (!call_func) {
not_a_function:
return JS_ThrowTypeErrorNotAFunction(caller_ctx);
if (rt->last_var_atom != JS_ATOM_NULL) {
JSAtom atom = rt->last_var_atom;
rt->last_var_atom = JS_ATOM_NULL; /* clear for next call */
return JS_ThrowTypeErrorNotAFunctionAtom(caller_ctx, atom);
}
return JS_ThrowTypeError(caller_ctx, "not a function");
}
return call_func(caller_ctx, func_obj, this_obj, argc,
argv, flags);
Expand Down Expand Up @@ -18060,6 +18083,10 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
atom = get_u32(pc);
pc += 4;
sf->cur_pc = pc;
/* only store atom if it looks valid */
if (atom != JS_ATOM_NULL && atom < JS_ATOM_END) {
Copy link
Copy Markdown
Contributor

@bnoordhuis bnoordhuis Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS_ATOM_END is the end of the static/named atoms (i.e., the ones from quickjs-atom.h) but many more are created at runtime so this won't work, or at least be very limiting.

On the flip side, atoms >= JS_ATOM_END are reference-counted so you have to be very careful not to introduce use-after-free bugs. In particular, one problem I see is:

  1. atom is assigned to rt->last_var_atom
  2. the corresponding JSFunctionDef object is freed
  3. Sometime later rt->last_var_atom is accessed

rt->last_var_atom is now either invalid (use-after-free) or, if the atom's slot in the runtime atom table has been reused, points to the wrong atom.

In JS_CallInternal you can maybe avoid that by caching rt->last_var_atom in a local variable at function entry, then restoring it at function exit.

rt->last_var_atom = atom; /* store atom for error messages */
}

val = JS_GetGlobalVar(ctx, atom, opcode - OP_get_var_undef);
if (unlikely(JS_IsException(val)))
Expand Down Expand Up @@ -18125,6 +18152,13 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
int idx;
idx = get_u16(pc);
pc += 2;
/* only store atom if it looks valid */
if (b && b->vardefs && idx < b->arg_count + b->var_count) {
JSAtom atom = b->vardefs[idx].var_name;
if (atom != JS_ATOM_NULL && atom < JS_ATOM_END) {
rt->last_var_atom = atom;
}
}
sp[0] = js_dup(var_buf[idx]);
sp++;
}
Expand Down Expand Up @@ -18173,7 +18207,7 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
}
BREAK;

CASE(OP_get_loc8): *sp++ = js_dup(var_buf[*pc++]); BREAK;
CASE(OP_get_loc8): if (b && b->vardefs) { JSAtom a = b->vardefs[*pc].var_name; if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a; } *sp++ = js_dup(var_buf[*pc++]); BREAK;
CASE(OP_put_loc8): set_value(ctx, &var_buf[*pc++], *--sp); BREAK;
CASE(OP_set_loc8): set_value(ctx, &var_buf[*pc++], js_dup(sp[-1])); BREAK;

Expand All @@ -18182,13 +18216,14 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
// making them ideal candidates for opcode fusion.
CASE(OP_get_loc0_loc1):
*sp++ = js_dup(var_buf[0]);
if (b && b->vardefs) { JSAtom a = b->vardefs[1].var_name; if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a; }
*sp++ = js_dup(var_buf[1]);
BREAK;

CASE(OP_get_loc0): *sp++ = js_dup(var_buf[0]); BREAK;
CASE(OP_get_loc1): *sp++ = js_dup(var_buf[1]); BREAK;
CASE(OP_get_loc2): *sp++ = js_dup(var_buf[2]); BREAK;
CASE(OP_get_loc3): *sp++ = js_dup(var_buf[3]); BREAK;
CASE(OP_get_loc0): if (b && b->vardefs) { JSAtom a = b->vardefs[0].var_name; if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a; } *sp++ = js_dup(var_buf[0]); BREAK;
Copy link
Copy Markdown
Contributor

@bnoordhuis bnoordhuis Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OP_get_loc opcodes are very performance sensitive and adding conditionals likely introduces performance regressions. Can you check with https://github.com/quickjs-ng/web-tooling-benchmark?

There's also a func_call benchmark in tests/microbench.js but it doesn't really exercise these code paths. You're welcome to add one that does though.

CASE(OP_get_loc1): if (b && b->vardefs) { JSAtom a = b->vardefs[1].var_name; if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a; } *sp++ = js_dup(var_buf[1]); BREAK;
CASE(OP_get_loc2): if (b && b->vardefs) { JSAtom a = b->vardefs[2].var_name; if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a; } *sp++ = js_dup(var_buf[2]); BREAK;
CASE(OP_get_loc3): if (b && b->vardefs) { JSAtom a = b->vardefs[3].var_name; if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a; } *sp++ = js_dup(var_buf[3]); BREAK;
CASE(OP_put_loc0): set_value(ctx, &var_buf[0], *--sp); BREAK;
CASE(OP_put_loc1): set_value(ctx, &var_buf[1], *--sp); BREAK;
CASE(OP_put_loc2): set_value(ctx, &var_buf[2], *--sp); BREAK;
Expand All @@ -18209,10 +18244,47 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
CASE(OP_set_arg1): set_value(ctx, &arg_buf[1], js_dup(sp[-1])); BREAK;
CASE(OP_set_arg2): set_value(ctx, &arg_buf[2], js_dup(sp[-1])); BREAK;
CASE(OP_set_arg3): set_value(ctx, &arg_buf[3], js_dup(sp[-1])); BREAK;
CASE(OP_get_var_ref0): *sp++ = js_dup(*var_refs[0]->pvalue); BREAK;
CASE(OP_get_var_ref1): *sp++ = js_dup(*var_refs[1]->pvalue); BREAK;
CASE(OP_get_var_ref2): *sp++ = js_dup(*var_refs[2]->pvalue); BREAK;
CASE(OP_get_var_ref3): *sp++ = js_dup(*var_refs[3]->pvalue); BREAK;
CASE(OP_get_var_ref0):
{
*sp++ = js_dup(*var_refs[0]->pvalue);
if (b && b->vardefs && 0 < b->arg_count + b->var_count) {
JSAtom a = b->vardefs[0].var_name;
if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a;
} else if (b && b->closure_var && 0 < b->closure_var_count) {
JSAtom a = b->closure_var[0].var_name;
if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a;
}
}
BREAK;

CASE(OP_get_var_ref1):
{
*sp++ = js_dup(*var_refs[1]->pvalue);
if (b && b->vardefs && 1 < b->arg_count + b->var_count) {
JSAtom a = b->vardefs[1].var_name;
if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a;
}
}
BREAK;

CASE(OP_get_var_ref2):
{
*sp++ = js_dup(*var_refs[2]->pvalue);
if (b && b->vardefs && 2 < b->arg_count + b->var_count) {
JSAtom a = b->vardefs[2].var_name;
if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a;
}
}
BREAK;
CASE(OP_get_var_ref3):
{
*sp++ = js_dup(*var_refs[3]->pvalue);
if (b && b->vardefs && 3 < b->arg_count + b->var_count) {
JSAtom a = b->vardefs[3].var_name;
if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a;
}
}
BREAK;
CASE(OP_put_var_ref0): set_value(ctx, var_refs[0]->pvalue, *--sp); BREAK;
CASE(OP_put_var_ref1): set_value(ctx, var_refs[1]->pvalue, *--sp); BREAK;
CASE(OP_put_var_ref2): set_value(ctx, var_refs[2]->pvalue, *--sp); BREAK;
Expand All @@ -18231,7 +18303,20 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
val = *var_refs[idx]->pvalue;
sp[0] = js_dup(val);
sp++;

/* store atom for error messages - with validity checks */
if (b && b->vardefs && idx < b->arg_count + b->var_count) {
JSAtom a = b->vardefs[idx].var_name;
if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a;
} else if (b && b->closure_var && idx >= b->arg_count + b->var_count) {
int closure_idx = idx - (b->arg_count + b->var_count);
if (closure_idx < b->closure_var_count) {
JSAtom a = b->closure_var[closure_idx].var_name;
if (a != JS_ATOM_NULL && a < JS_ATOM_END) rt->last_var_atom = a;
}
}
}

BREAK;
CASE(OP_put_var_ref):
{
Expand Down Expand Up @@ -19815,6 +19900,7 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
}
switch (opcode) {
case OP_with_get_var:
rt->last_var_atom = atom; /* store atom for error messages */
val = JS_GetProperty(ctx, obj, atom);
if (unlikely(JS_IsException(val)))
goto exception;
Expand Down Expand Up @@ -19934,6 +20020,8 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
}
}
exception:
/* Restore last_var_atom before handling exception */
rt->last_var_atom = saved_last_var_atom;
if (needs_backtrace(rt->current_exception)
|| JS_IsUndefined(ctx->error_back_trace)) {
sf->cur_pc = pc;
Expand Down Expand Up @@ -19982,6 +20070,8 @@ static JSValue JS_CallInternal(JSContext *caller_ctx, JSValueConst func_obj,
}
}
rt->current_stack_frame = sf->prev_frame;
/* Restore last_var_atom on normal return */
rt->last_var_atom = saved_last_var_atom;
return ret_val;
}

Expand Down Expand Up @@ -20099,6 +20189,11 @@ static JSValue JS_CallConstructorInternal(JSContext *ctx,
call_func = ctx->rt->class_array[p->class_id].call;
if (!call_func) {
not_a_function:
if (ctx->rt->last_var_atom != JS_ATOM_NULL) {
JSAtom atom = ctx->rt->last_var_atom;
ctx->rt->last_var_atom = JS_ATOM_NULL; /* clear for next call */
return JS_ThrowTypeErrorNotAFunctionAtom(ctx, atom);
}
return JS_ThrowTypeErrorNotAFunction(ctx);
}
return call_func(ctx, func_obj, new_target, argc,
Expand Down Expand Up @@ -37339,21 +37434,18 @@ static int JS_WriteRegExp(BCWriterState *s, JSRegExp regexp)
assert(!bc->is_wide_char);

JS_WriteString(s, regexp.pattern);

if (is_be()) {
Copy link
Copy Markdown
Contributor

@saghul saghul Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please undo this change.

if (is_be()) {
if (lre_byte_swap(str8(bc), bc->len, /*is_byte_swapped*/false)) {
fail:
JS_ThrowInternalError(s->ctx, "regex byte swap failed");
return -1;
return -1;
}
}
}

JS_WriteString(s, bc);

if (is_be()) {
if (lre_byte_swap(str8(bc), bc->len, /*is_byte_swapped*/true))
goto fail;
}
if (is_be())
lre_byte_swap(str8(bc), bc->len, /*is_byte_swapped*/true);

return 0;
}
Expand Down Expand Up @@ -38614,14 +38706,17 @@ static JSValue JS_ReadRegExp(BCReaderState *s)
}

if (is_be()) {
Copy link
Copy Markdown
Contributor

@saghul saghul Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please undo this change.

if (lre_byte_swap(str8(bc), bc->len, /*is_byte_swapped*/true)) {
if (lre_byte_swap(str8(bc), bc->len, /* is_byte_swapped */true)) {
js_free_string(ctx->rt, pattern);
js_free_string(ctx->rt, bc);
return JS_ThrowInternalError(ctx, "bad regexp bytecode");
}
}

return js_regexp_constructor_internal(ctx, JS_UNDEFINED,
/* Pass ctx->regexp_ctor (not JS_UNDEFINED) to bypass regexp_shape
fast path during deserialization*/
return js_regexp_constructor_internal(ctx,
ctx->class_proto[JS_CLASS_REGEXP],
JS_MKPTR(JS_TAG_STRING, pattern),
JS_MKPTR(JS_TAG_STRING, bc));
}
Expand Down Expand Up @@ -38949,7 +39044,16 @@ static int check_function(JSContext *ctx, JSValueConst obj)
{
if (likely(JS_IsFunction(ctx, obj)))
return 0;
JS_ThrowTypeErrorNotAFunction(ctx);

if (ctx->rt->last_var_atom != JS_ATOM_NULL) {
JSAtom atom = ctx->rt->last_var_atom;
ctx->rt->last_var_atom = JS_ATOM_NULL; /* clear for next call */

JS_ThrowTypeErrorNotAFunctionAtom(ctx, atom);
} else {
JS_ThrowTypeErrorNotAFunction(ctx);
}

return -1;
}

Expand Down Expand Up @@ -47586,18 +47690,23 @@ static JSValue js_regexp_exec(JSContext *ctx, JSValueConst this_val,
goto fail;
}
} else {
// if (rc == LRE_RET_TIMEOUT) {
// JS_ThrowInterrupted(ctx);
// } else {
// JS_ThrowInternalError(ctx, "out of memory in regexp execution");
// }
switch(rc) {
Copy link
Copy Markdown
Contributor

@saghul saghul Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please undo this change.

case LRE_RET_TIMEOUT:
JS_ThrowInterrupted(ctx);
break;
case LRE_RET_MEMORY_ERROR:
JS_ThrowInternalError(ctx, "out of memory in regexp execution");
break;
case LRE_RET_BYTECODE_ERROR:
JS_ThrowInternalError(ctx, "corrupted bytecode in regexp execution");
break;
default:
abort();
case LRE_RET_TIMEOUT:
JS_ThrowInterrupted(ctx);
break;
case LRE_RET_MEMORY_ERROR:
JS_ThrowInternalError(ctx, "out of memory in regexp execution");
break;
case LRE_RET_BYTECODE_ERROR:
JS_ThrowInternalError(ctx, "corrupted bytecode in regexp execution");
break;
default:
abort();
}
goto fail;
}
Expand Down Expand Up @@ -47785,18 +47894,18 @@ static JSValue JS_RegExpDelete(JSContext *ctx, JSValueConst this_val, JSValue ar
}
} else {
switch(ret) {
Copy link
Copy Markdown
Contributor

@saghul saghul Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please undo this change,

case LRE_RET_TIMEOUT:
JS_ThrowInterrupted(ctx);
break;
case LRE_RET_MEMORY_ERROR:
JS_ThrowInternalError(ctx, "out of memory in regexp execution");
break;
case LRE_RET_BYTECODE_ERROR:
JS_ThrowInternalError(ctx, "corrupted bytecode in regexp execution");
break;
default:
abort();
}
case LRE_RET_TIMEOUT:
JS_ThrowInterrupted(ctx);
break;
case LRE_RET_MEMORY_ERROR:
JS_ThrowInternalError(ctx, "out of memory in regexp execution");
break;
case LRE_RET_BYTECODE_ERROR:
JS_ThrowInternalError(ctx, "corrupted bytecode in regexp execution");
break;
default:
abort();
}
goto fail;
}
break;
Expand Down
2 changes: 2 additions & 0 deletions tests/test1231.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
var a = 0;
a();
Loading