Skip to content

Commit

Permalink
Fix thread-safety issue in quickjs-libc (#578)
Browse files Browse the repository at this point in the history 
`JS_NewClassID(rt, &class_id)` where `class_id` is a global variable
is unsafe when called from multiple threads but that is exactly what
quickjs-libc.c did.

Add a new JS_AddRuntimeFinalizer function that lets quickjs-libc
store the class ids in JSRuntimeState and defer freeing the memory
until the runtime is destroyed. Necessary because object finalizers
such as js_std_file_finalizer need to know the class id and run after
js_std_free_handlers runs.

Fixes: #577
  • Loading branch information
@bnoordhuis
bnoordhuis authored Oct 7, 2024
1 parent 27715a4 commit 9a37c57
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 23 deletions.
58 changes: 35 additions & 23 deletions quickjs-libc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ typedef struct JSThreadState {
JSValue exc; /* current exception from one of our handlers */
/* not used in the main thread */
JSWorkerMessagePipe *recv_pipe, *send_pipe;
JSClassID std_file_class_id;
JSClassID worker_class_id;
} JSThreadState;

static uint64_t os_pending_signals;
Expand Down Expand Up @@ -874,8 +876,6 @@ static JSValue js_evalScript(JSContext *ctx, JSValue this_val,
return ret;
}

static JSClassID js_std_file_class_id;

typedef struct {
FILE *f;
BOOL is_popen;
Expand All @@ -888,7 +888,8 @@ static BOOL is_stdio(FILE *f)

static void js_std_file_finalizer(JSRuntime *rt, JSValue val)
{
JSSTDFile *s = JS_GetOpaque(val, js_std_file_class_id);
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
JSSTDFile *s = JS_GetOpaque(val, ts->std_file_class_id);
if (s) {
if (s->f && !is_stdio(s->f)) {
#if !defined(__wasi__)
Expand Down Expand Up @@ -920,9 +921,11 @@ static JSValue js_std_strerror(JSContext *ctx, JSValue this_val,

static JSValue js_new_std_file(JSContext *ctx, FILE *f, BOOL is_popen)
{
JSRuntime *rt = JS_GetRuntime(ctx);
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
JSSTDFile *s;
JSValue obj;
obj = JS_NewObjectClass(ctx, js_std_file_class_id);
obj = JS_NewObjectClass(ctx, ts->std_file_class_id);
if (JS_IsException(obj))
return obj;
s = js_mallocz(ctx, sizeof(*s));
Expand Down Expand Up @@ -1078,7 +1081,9 @@ static JSValue js_std_printf(JSContext *ctx, JSValue this_val,

static FILE *js_std_file_get(JSContext *ctx, JSValue obj)
{
JSSTDFile *s = JS_GetOpaque2(ctx, obj, js_std_file_class_id);
JSRuntime *rt = JS_GetRuntime(ctx);
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
JSSTDFile *s = JS_GetOpaque2(ctx, obj, ts->std_file_class_id);
if (!s)
return NULL;
if (!s->f) {
Expand Down Expand Up @@ -1117,7 +1122,9 @@ static JSValue js_std_file_puts(JSContext *ctx, JSValue this_val,
static JSValue js_std_file_close(JSContext *ctx, JSValue this_val,
int argc, JSValue *argv)
{
JSSTDFile *s = JS_GetOpaque2(ctx, this_val, js_std_file_class_id);
JSRuntime *rt = JS_GetRuntime(ctx);
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
JSSTDFile *s = JS_GetOpaque2(ctx, this_val, ts->std_file_class_id);
int err;
if (!s)
return JS_EXCEPTION;
Expand Down Expand Up @@ -1633,16 +1640,17 @@ static int js_std_init(JSContext *ctx, JSModuleDef *m)
{
JSValue proto;
JSRuntime *rt = JS_GetRuntime(ctx);
JSThreadState *ts = JS_GetRuntimeOpaque(rt);

/* FILE class */
/* the class ID is created once */
JS_NewClassID(rt, &js_std_file_class_id);
JS_NewClassID(rt, &ts->std_file_class_id);
/* the class is created once per runtime */
JS_NewClass(rt, js_std_file_class_id, &js_std_file_class);
JS_NewClass(rt, ts->std_file_class_id, &js_std_file_class);
proto = JS_NewObject(ctx);
JS_SetPropertyFunctionList(ctx, proto, js_std_file_proto_funcs,
countof(js_std_file_proto_funcs));
JS_SetClassProto(ctx, js_std_file_class_id, proto);
JS_SetClassProto(ctx, ts->std_file_class_id, proto);

JS_SetModuleExportList(ctx, m, js_std_funcs,
countof(js_std_funcs));
Expand Down Expand Up @@ -3278,7 +3286,6 @@ typedef struct {
uint64_t buf[];
} JSSABHeader;

static JSClassID js_worker_class_id;
static JSContext *(*js_worker_new_context_func)(JSRuntime *rt);

static int atomic_add_int(int *ptr, int v)
Expand Down Expand Up @@ -3391,7 +3398,8 @@ static void js_free_port(JSRuntime *rt, JSWorkerMessageHandler *port)

static void js_worker_finalizer(JSRuntime *rt, JSValue val)
{
JSWorkerData *worker = JS_GetOpaque(val, js_worker_class_id);
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
JSWorkerData *worker = JS_GetOpaque(val, ts->worker_class_id);
if (worker) {
js_free_message_pipe(worker->recv_pipe);
js_free_message_pipe(worker->send_pipe);
Expand Down Expand Up @@ -3459,18 +3467,20 @@ static JSValue js_worker_ctor_internal(JSContext *ctx, JSValue new_target,
JSWorkerMessagePipe *recv_pipe,
JSWorkerMessagePipe *send_pipe)
{
JSRuntime *rt = JS_GetRuntime(ctx);
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
JSValue obj = JS_UNDEFINED, proto;
JSWorkerData *s;

/* create the object */
if (JS_IsUndefined(new_target)) {
proto = JS_GetClassProto(ctx, js_worker_class_id);
proto = JS_GetClassProto(ctx, ts->worker_class_id);
} else {
proto = JS_GetPropertyStr(ctx, new_target, "prototype");
if (JS_IsException(proto))
goto fail;
}
obj = JS_NewObjectProtoClass(ctx, proto, js_worker_class_id);
obj = JS_NewObjectProtoClass(ctx, proto, ts->worker_class_id);
JS_FreeValue(ctx, proto);
if (JS_IsException(obj))
goto fail;
Expand Down Expand Up @@ -3574,7 +3584,9 @@ static JSValue js_worker_ctor(JSContext *ctx, JSValue new_target,
static JSValue js_worker_postMessage(JSContext *ctx, JSValue this_val,
int argc, JSValue *argv)
{
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, js_worker_class_id);
JSRuntime *rt = JS_GetRuntime(ctx);
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, ts->worker_class_id);
JSWorkerMessagePipe *ps;
size_t data_len, i;
uint8_t *data;
Expand Down Expand Up @@ -3653,7 +3665,7 @@ static JSValue js_worker_set_onmessage(JSContext *ctx, JSValue this_val,
{
JSRuntime *rt = JS_GetRuntime(ctx);
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, js_worker_class_id);
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, ts->worker_class_id);
JSWorkerMessageHandler *port;

if (!worker)
Expand Down Expand Up @@ -3685,7 +3697,9 @@ static JSValue js_worker_set_onmessage(JSContext *ctx, JSValue this_val,

static JSValue js_worker_get_onmessage(JSContext *ctx, JSValue this_val)
{
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, js_worker_class_id);
JSRuntime *rt = JS_GetRuntime(ctx);
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, ts->worker_class_id);
JSWorkerMessageHandler *port;
if (!worker)
return JS_EXCEPTION;
Expand Down Expand Up @@ -3838,16 +3852,16 @@ static int js_os_init(JSContext *ctx, JSModuleDef *m)
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
JSValue proto, obj;
/* Worker class */
JS_NewClassID(rt, &js_worker_class_id);
JS_NewClass(rt, js_worker_class_id, &js_worker_class);
JS_NewClassID(rt, &ts->worker_class_id);
JS_NewClass(rt, ts->worker_class_id, &js_worker_class);
proto = JS_NewObject(ctx);
JS_SetPropertyFunctionList(ctx, proto, js_worker_proto_funcs, countof(js_worker_proto_funcs));

obj = JS_NewCFunction2(ctx, js_worker_ctor, "Worker", 1,
JS_CFUNC_constructor, 0);
JS_SetConstructor(ctx, obj, proto);

JS_SetClassProto(ctx, js_worker_class_id, proto);
JS_SetClassProto(ctx, ts->worker_class_id, proto);

/* set 'Worker.parent' if necessary */
if (ts->recv_pipe && ts->send_pipe) {
Expand Down Expand Up @@ -3961,7 +3975,7 @@ void js_std_init_handlers(JSRuntime *rt)
{
JSThreadState *ts;

ts = malloc(sizeof(*ts));
ts = js_malloc_rt(rt, sizeof(*ts));
if (!ts) {
fprintf(stderr, "Could not allocate memory for the worker");
exit(1);
Expand All @@ -3976,6 +3990,7 @@ void js_std_init_handlers(JSRuntime *rt)
ts->exc = JS_UNDEFINED;

JS_SetRuntimeOpaque(rt, ts);
JS_AddRuntimeFinalizer(rt, js_free_rt, ts);

#ifdef USE_WORKER
/* set the SharedArrayBuffer memory handlers */
Expand Down Expand Up @@ -4015,9 +4030,6 @@ void js_std_free_handlers(JSRuntime *rt)
js_free_message_pipe(ts->recv_pipe);
js_free_message_pipe(ts->send_pipe);
#endif

free(ts);
JS_SetRuntimeOpaque(rt, NULL); /* fail safe */
}

static void js_dump_obj(JSContext *ctx, FILE *f, JSValue val)
Expand Down
27 changes: 27 additions & 0 deletions quickjs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,12 @@ typedef struct JSMallocState {
void *opaque; /* user opaque */
} JSMallocState;

typedef struct JSRuntimeFinalizerState {
struct JSRuntimeFinalizerState *next;
JSRuntimeFinalizer *finalizer;
void *arg;
} JSRuntimeFinalizerState;

struct JSRuntime {
JSMallocFunctions mf;
JSMallocState malloc_state;
Expand Down Expand Up @@ -292,6 +298,7 @@ struct JSRuntime {
JSShape **shape_hash;
bf_context_t bf_ctx;
void *user_opaque;
JSRuntimeFinalizerState *finalizers;
};

struct JSClass {
Expand Down Expand Up @@ -1816,6 +1823,19 @@ void JS_SetRuntimeOpaque(JSRuntime *rt, void *opaque)
rt->user_opaque = opaque;
}

int JS_AddRuntimeFinalizer(JSRuntime *rt, JSRuntimeFinalizer *finalizer,
void *arg)
{
JSRuntimeFinalizerState *fs = js_malloc_rt(rt, sizeof(*fs));
if (!fs)
return -1;
fs->next = rt->finalizers;
fs->finalizer = finalizer;
fs->arg = arg;
rt->finalizers = fs;
return 0;
}

static void *js_def_calloc(void *opaque, size_t count, size_t size)
{
return calloc(count, size);
Expand Down Expand Up @@ -2196,6 +2216,13 @@ void JS_FreeRuntime(JSRuntime *rt)
}
#endif

while (rt->finalizers) {
JSRuntimeFinalizerState *fs = rt->finalizers;
rt->finalizers = fs->next;
fs->finalizer(rt, fs->arg);
js_free_rt(rt, fs);
}

{
JSMallocState *ms = &rt->malloc_state;
rt->mf.js_free(ms->opaque, rt);
Expand Down
7 changes: 7 additions & 0 deletions quickjs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,11 @@ typedef struct JSMallocFunctions {
size_t (*js_malloc_usable_size)(const void *ptr);
} JSMallocFunctions;

// Finalizers run in LIFO order at the very end of JS_FreeRuntime.
// Intended for cleanup of associated resources; the runtime itself
// is no longer usable.
typedef void JSRuntimeFinalizer(JSRuntime *rt, void *arg);

typedef struct JSGCObjectHeader JSGCObjectHeader;

JS_EXTERN JSRuntime *JS_NewRuntime(void);
Expand All @@ -306,6 +311,8 @@ JS_EXTERN JSRuntime *JS_NewRuntime2(const JSMallocFunctions *mf, void *opaque);
JS_EXTERN void JS_FreeRuntime(JSRuntime *rt);
JS_EXTERN void *JS_GetRuntimeOpaque(JSRuntime *rt);
JS_EXTERN void JS_SetRuntimeOpaque(JSRuntime *rt, void *opaque);
JS_EXTERN int JS_AddRuntimeFinalizer(JSRuntime *rt,
JSRuntimeFinalizer *finalizer, void *arg);
typedef void JS_MarkFunc(JSRuntime *rt, JSGCObjectHeader *gp);
JS_EXTERN void JS_MarkValue(JSRuntime *rt, JSValue val, JS_MarkFunc *mark_func);
JS_EXTERN void JS_RunGC(JSRuntime *rt);
Expand Down

0 comments on commit 9a37c57

Please sign in to comment.