Skip to content

Commit c902348

Browse files
bnoordhuisbnoordhuis
committed
Fix thread-safety issue in quickjs-libc
`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
1 parent 27715a4 commit c902348

File tree

3 files changed

+69
-23
lines changed

3 files changed

+69
-23
lines changed

‎quickjs-libc.c

+35-23
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,8 @@ typedef struct JSThreadState {
164164
JSValue exc; /* current exception from one of our handlers */
165165
/* not used in the main thread */
166166
JSWorkerMessagePipe *recv_pipe, *send_pipe;
167+
JSClassID std_file_class_id;
168+
JSClassID worker_class_id;
167169
} JSThreadState;
168170

169171
static uint64_t os_pending_signals;
@@ -874,8 +876,6 @@ static JSValue js_evalScript(JSContext *ctx, JSValue this_val,
874876
return ret;
875877
}
876878

877-
static JSClassID js_std_file_class_id;
878-
879879
typedef struct {
880880
FILE *f;
881881
BOOL is_popen;
@@ -888,7 +888,8 @@ static BOOL is_stdio(FILE *f)
888888

889889
static void js_std_file_finalizer(JSRuntime *rt, JSValue val)
890890
{
891-
JSSTDFile *s = JS_GetOpaque(val, js_std_file_class_id);
891+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
892+
JSSTDFile *s = JS_GetOpaque(val, ts->std_file_class_id);
892893
if (s) {
893894
if (s->f && !is_stdio(s->f)) {
894895
#if !defined(__wasi__)
@@ -920,9 +921,11 @@ static JSValue js_std_strerror(JSContext *ctx, JSValue this_val,
920921

921922
static JSValue js_new_std_file(JSContext *ctx, FILE *f, BOOL is_popen)
922923
{
924+
JSRuntime *rt = JS_GetRuntime(ctx);
925+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
923926
JSSTDFile *s;
924927
JSValue obj;
925-
obj = JS_NewObjectClass(ctx, js_std_file_class_id);
928+
obj = JS_NewObjectClass(ctx, ts->std_file_class_id);
926929
if (JS_IsException(obj))
927930
return obj;
928931
s = js_mallocz(ctx, sizeof(*s));
@@ -1078,7 +1081,9 @@ static JSValue js_std_printf(JSContext *ctx, JSValue this_val,
10781081

10791082
static FILE *js_std_file_get(JSContext *ctx, JSValue obj)
10801083
{
1081-
JSSTDFile *s = JS_GetOpaque2(ctx, obj, js_std_file_class_id);
1084+
JSRuntime *rt = JS_GetRuntime(ctx);
1085+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
1086+
JSSTDFile *s = JS_GetOpaque2(ctx, obj, ts->std_file_class_id);
10821087
if (!s)
10831088
return NULL;
10841089
if (!s->f) {
@@ -1117,7 +1122,9 @@ static JSValue js_std_file_puts(JSContext *ctx, JSValue this_val,
11171122
static JSValue js_std_file_close(JSContext *ctx, JSValue this_val,
11181123
int argc, JSValue *argv)
11191124
{
1120-
JSSTDFile *s = JS_GetOpaque2(ctx, this_val, js_std_file_class_id);
1125+
JSRuntime *rt = JS_GetRuntime(ctx);
1126+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
1127+
JSSTDFile *s = JS_GetOpaque2(ctx, this_val, ts->std_file_class_id);
11211128
int err;
11221129
if (!s)
11231130
return JS_EXCEPTION;
@@ -1633,16 +1640,17 @@ static int js_std_init(JSContext *ctx, JSModuleDef *m)
16331640
{
16341641
JSValue proto;
16351642
JSRuntime *rt = JS_GetRuntime(ctx);
1643+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
16361644

16371645
/* FILE class */
16381646
/* the class ID is created once */
1639-
JS_NewClassID(rt, &js_std_file_class_id);
1647+
JS_NewClassID(rt, &ts->std_file_class_id);
16401648
/* the class is created once per runtime */
1641-
JS_NewClass(rt, js_std_file_class_id, &js_std_file_class);
1649+
JS_NewClass(rt, ts->std_file_class_id, &js_std_file_class);
16421650
proto = JS_NewObject(ctx);
16431651
JS_SetPropertyFunctionList(ctx, proto, js_std_file_proto_funcs,
16441652
countof(js_std_file_proto_funcs));
1645-
JS_SetClassProto(ctx, js_std_file_class_id, proto);
1653+
JS_SetClassProto(ctx, ts->std_file_class_id, proto);
16461654

16471655
JS_SetModuleExportList(ctx, m, js_std_funcs,
16481656
countof(js_std_funcs));
@@ -3278,7 +3286,6 @@ typedef struct {
32783286
uint64_t buf[];
32793287
} JSSABHeader;
32803288

3281-
static JSClassID js_worker_class_id;
32823289
static JSContext *(*js_worker_new_context_func)(JSRuntime *rt);
32833290

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

33923399
static void js_worker_finalizer(JSRuntime *rt, JSValue val)
33933400
{
3394-
JSWorkerData *worker = JS_GetOpaque(val, js_worker_class_id);
3401+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
3402+
JSWorkerData *worker = JS_GetOpaque(val, ts->worker_class_id);
33953403
if (worker) {
33963404
js_free_message_pipe(worker->recv_pipe);
33973405
js_free_message_pipe(worker->send_pipe);
@@ -3459,18 +3467,20 @@ static JSValue js_worker_ctor_internal(JSContext *ctx, JSValue new_target,
34593467
JSWorkerMessagePipe *recv_pipe,
34603468
JSWorkerMessagePipe *send_pipe)
34613469
{
3470+
JSRuntime *rt = JS_GetRuntime(ctx);
3471+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
34623472
JSValue obj = JS_UNDEFINED, proto;
34633473
JSWorkerData *s;
34643474

34653475
/* create the object */
34663476
if (JS_IsUndefined(new_target)) {
3467-
proto = JS_GetClassProto(ctx, js_worker_class_id);
3477+
proto = JS_GetClassProto(ctx, ts->worker_class_id);
34683478
} else {
34693479
proto = JS_GetPropertyStr(ctx, new_target, "prototype");
34703480
if (JS_IsException(proto))
34713481
goto fail;
34723482
}
3473-
obj = JS_NewObjectProtoClass(ctx, proto, js_worker_class_id);
3483+
obj = JS_NewObjectProtoClass(ctx, proto, ts->worker_class_id);
34743484
JS_FreeValue(ctx, proto);
34753485
if (JS_IsException(obj))
34763486
goto fail;
@@ -3574,7 +3584,9 @@ static JSValue js_worker_ctor(JSContext *ctx, JSValue new_target,
35743584
static JSValue js_worker_postMessage(JSContext *ctx, JSValue this_val,
35753585
int argc, JSValue *argv)
35763586
{
3577-
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, js_worker_class_id);
3587+
JSRuntime *rt = JS_GetRuntime(ctx);
3588+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
3589+
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, ts->worker_class_id);
35783590
JSWorkerMessagePipe *ps;
35793591
size_t data_len, i;
35803592
uint8_t *data;
@@ -3653,7 +3665,7 @@ static JSValue js_worker_set_onmessage(JSContext *ctx, JSValue this_val,
36533665
{
36543666
JSRuntime *rt = JS_GetRuntime(ctx);
36553667
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
3656-
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, js_worker_class_id);
3668+
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, ts->worker_class_id);
36573669
JSWorkerMessageHandler *port;
36583670

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

36863698
static JSValue js_worker_get_onmessage(JSContext *ctx, JSValue this_val)
36873699
{
3688-
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, js_worker_class_id);
3700+
JSRuntime *rt = JS_GetRuntime(ctx);
3701+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
3702+
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, ts->worker_class_id);
36893703
JSWorkerMessageHandler *port;
36903704
if (!worker)
36913705
return JS_EXCEPTION;
@@ -3838,16 +3852,16 @@ static int js_os_init(JSContext *ctx, JSModuleDef *m)
38383852
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
38393853
JSValue proto, obj;
38403854
/* Worker class */
3841-
JS_NewClassID(rt, &js_worker_class_id);
3842-
JS_NewClass(rt, js_worker_class_id, &js_worker_class);
3855+
JS_NewClassID(rt, &ts->worker_class_id);
3856+
JS_NewClass(rt, ts->worker_class_id, &js_worker_class);
38433857
proto = JS_NewObject(ctx);
38443858
JS_SetPropertyFunctionList(ctx, proto, js_worker_proto_funcs, countof(js_worker_proto_funcs));
38453859

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

3850-
JS_SetClassProto(ctx, js_worker_class_id, proto);
3864+
JS_SetClassProto(ctx, ts->worker_class_id, proto);
38513865

38523866
/* set 'Worker.parent' if necessary */
38533867
if (ts->recv_pipe && ts->send_pipe) {
@@ -3961,7 +3975,7 @@ void js_std_init_handlers(JSRuntime *rt)
39613975
{
39623976
JSThreadState *ts;
39633977

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

39783992
JS_SetRuntimeOpaque(rt, ts);
3993+
JS_AddRuntimeFinalizer(rt, js_free_rt, ts);
39793994

39803995
#ifdef USE_WORKER
39813996
/* set the SharedArrayBuffer memory handlers */
@@ -4015,9 +4030,6 @@ void js_std_free_handlers(JSRuntime *rt)
40154030
js_free_message_pipe(ts->recv_pipe);
40164031
js_free_message_pipe(ts->send_pipe);
40174032
#endif
4018-
4019-
free(ts);
4020-
JS_SetRuntimeOpaque(rt, NULL); /* fail safe */
40214033
}
40224034

40234035
static void js_dump_obj(JSContext *ctx, FILE *f, JSValue val)

‎quickjs.c

+27
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,12 @@ typedef struct JSMallocState {
221221
void *opaque; /* user opaque */
222222
} JSMallocState;
223223

224+
typedef struct JSRuntimeFinalizerState {
225+
struct JSRuntimeFinalizerState *next;
226+
JSRuntimeFinalizer *finalizer;
227+
void *arg;
228+
} JSRuntimeFinalizerState;
229+
224230
struct JSRuntime {
225231
JSMallocFunctions mf;
226232
JSMallocState malloc_state;
@@ -292,6 +298,7 @@ struct JSRuntime {
292298
JSShape **shape_hash;
293299
bf_context_t bf_ctx;
294300
void *user_opaque;
301+
JSRuntimeFinalizerState *finalizers;
295302
};
296303

297304
struct JSClass {
@@ -1816,6 +1823,19 @@ void JS_SetRuntimeOpaque(JSRuntime *rt, void *opaque)
18161823
rt->user_opaque = opaque;
18171824
}
18181825

1826+
int JS_AddRuntimeFinalizer(JSRuntime *rt, JSRuntimeFinalizer *finalizer,
1827+
void *arg)
1828+
{
1829+
JSRuntimeFinalizerState *fs = js_malloc_rt(rt, sizeof(*fs));
1830+
if (!fs)
1831+
return -1;
1832+
fs->next = rt->finalizers;
1833+
fs->finalizer = finalizer;
1834+
fs->arg = arg;
1835+
rt->finalizers = fs;
1836+
return 0;
1837+
}
1838+
18191839
static void *js_def_calloc(void *opaque, size_t count, size_t size)
18201840
{
18211841
return calloc(count, size);
@@ -2196,6 +2216,13 @@ void JS_FreeRuntime(JSRuntime *rt)
21962216
}
21972217
#endif
21982218

2219+
while (rt->finalizers) {
2220+
JSRuntimeFinalizerState *fs = rt->finalizers;
2221+
rt->finalizers = fs->next;
2222+
fs->finalizer(rt, fs->arg);
2223+
js_free_rt(rt, fs);
2224+
}
2225+
21992226
{
22002227
JSMallocState *ms = &rt->malloc_state;
22012228
rt->mf.js_free(ms->opaque, rt);

‎quickjs.h

+7
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,11 @@ typedef struct JSMallocFunctions {
287287
size_t (*js_malloc_usable_size)(const void *ptr);
288288
} JSMallocFunctions;
289289

290+
// Finalizers run in LIFO order at the very end of JS_FreeRuntime.
291+
// Intended for cleanup of associated resources; the runtime itself
292+
// is no longer usable.
293+
typedef void JSRuntimeFinalizer(JSRuntime *rt, void *arg);
294+
290295
typedef struct JSGCObjectHeader JSGCObjectHeader;
291296

292297
JS_EXTERN JSRuntime *JS_NewRuntime(void);
@@ -306,6 +311,8 @@ JS_EXTERN JSRuntime *JS_NewRuntime2(const JSMallocFunctions *mf, void *opaque);
306311
JS_EXTERN void JS_FreeRuntime(JSRuntime *rt);
307312
JS_EXTERN void *JS_GetRuntimeOpaque(JSRuntime *rt);
308313
JS_EXTERN void JS_SetRuntimeOpaque(JSRuntime *rt, void *opaque);
314+
JS_EXTERN int JS_AddRuntimeFinalizer(JSRuntime *rt,
315+
JSRuntimeFinalizer *finalizer, void *arg);
309316
typedef void JS_MarkFunc(JSRuntime *rt, JSGCObjectHeader *gp);
310317
JS_EXTERN void JS_MarkValue(JSRuntime *rt, JSValue val, JS_MarkFunc *mark_func);
311318
JS_EXTERN void JS_RunGC(JSRuntime *rt);

0 commit comments

Comments
 (0)