Skip to content

Commit dd6d1a1

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. js_std_free_handlers is now a no-op but is left in (for now) for compatibility with bellard/quickjs. Fixes: #577
1 parent ddabcf5 commit dd6d1a1

File tree

5 files changed

+98
-49
lines changed

5 files changed

+98
-49
lines changed

‎qjs.c

-2
Original file line numberDiff line numberDiff line change
@@ -562,7 +562,6 @@ int main(int argc, char **argv)
562562
JS_ComputeMemoryUsage(rt, &stats);
563563
JS_DumpMemoryUsage(stdout, &stats, rt);
564564
}
565-
js_std_free_handlers(rt);
566565
JS_FreeContext(ctx);
567566
JS_FreeRuntime(rt);
568567

@@ -592,7 +591,6 @@ int main(int argc, char **argv)
592591
}
593592
return 0;
594593
fail:
595-
js_std_free_handlers(rt);
596594
JS_FreeContext(ctx);
597595
JS_FreeRuntime(rt);
598596
return 1;

‎qjsc.c

-1
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,6 @@ static const char main_c_template1[] =
324324
static const char main_c_template2[] =
325325
" js_std_loop(ctx);\n"
326326
" JS_FreeContext(ctx);\n"
327-
" js_std_free_handlers(rt);\n"
328327
" JS_FreeRuntime(rt);\n"
329328
" return 0;\n"
330329
"}\n";

‎quickjs-libc.c

+64-46
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 close_in_finalizer;
@@ -884,7 +884,8 @@ typedef struct {
884884

885885
static void js_std_file_finalizer(JSRuntime *rt, JSValue val)
886886
{
887-
JSSTDFile *s = JS_GetOpaque(val, js_std_file_class_id);
887+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
888+
JSSTDFile *s = JS_GetOpaque(val, ts->std_file_class_id);
888889
if (s) {
889890
if (s->f && s->close_in_finalizer) {
890891
#if !defined(__wasi__)
@@ -918,9 +919,11 @@ static JSValue js_new_std_file(JSContext *ctx, FILE *f,
918919
BOOL close_in_finalizer,
919920
BOOL is_popen)
920921
{
922+
JSRuntime *rt = JS_GetRuntime(ctx);
923+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
921924
JSSTDFile *s;
922925
JSValue obj;
923-
obj = JS_NewObjectClass(ctx, js_std_file_class_id);
926+
obj = JS_NewObjectClass(ctx, ts->std_file_class_id);
924927
if (JS_IsException(obj))
925928
return obj;
926929
s = js_mallocz(ctx, sizeof(*s));
@@ -1077,7 +1080,9 @@ static JSValue js_std_printf(JSContext *ctx, JSValue this_val,
10771080

10781081
static FILE *js_std_file_get(JSContext *ctx, JSValue obj)
10791082
{
1080-
JSSTDFile *s = JS_GetOpaque2(ctx, obj, js_std_file_class_id);
1083+
JSRuntime *rt = JS_GetRuntime(ctx);
1084+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
1085+
JSSTDFile *s = JS_GetOpaque2(ctx, obj, ts->std_file_class_id);
10811086
if (!s)
10821087
return NULL;
10831088
if (!s->f) {
@@ -1116,7 +1121,9 @@ static JSValue js_std_file_puts(JSContext *ctx, JSValue this_val,
11161121
static JSValue js_std_file_close(JSContext *ctx, JSValue this_val,
11171122
int argc, JSValue *argv)
11181123
{
1119-
JSSTDFile *s = JS_GetOpaque2(ctx, this_val, js_std_file_class_id);
1124+
JSRuntime *rt = JS_GetRuntime(ctx);
1125+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
1126+
JSSTDFile *s = JS_GetOpaque2(ctx, this_val, ts->std_file_class_id);
11201127
int err;
11211128
if (!s)
11221129
return JS_EXCEPTION;
@@ -1630,16 +1637,17 @@ static int js_std_init(JSContext *ctx, JSModuleDef *m)
16301637
{
16311638
JSValue proto;
16321639
JSRuntime *rt = JS_GetRuntime(ctx);
1640+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
16331641

16341642
/* FILE class */
16351643
/* the class ID is created once */
1636-
JS_NewClassID(rt, &js_std_file_class_id);
1644+
JS_NewClassID(rt, &ts->std_file_class_id);
16371645
/* the class is created once per runtime */
1638-
JS_NewClass(rt, js_std_file_class_id, &js_std_file_class);
1646+
JS_NewClass(rt, ts->std_file_class_id, &js_std_file_class);
16391647
proto = JS_NewObject(ctx);
16401648
JS_SetPropertyFunctionList(ctx, proto, js_std_file_proto_funcs,
16411649
countof(js_std_file_proto_funcs));
1642-
JS_SetClassProto(ctx, js_std_file_class_id, proto);
1650+
JS_SetClassProto(ctx, ts->std_file_class_id, proto);
16431651

16441652
JS_SetModuleExportList(ctx, m, js_std_funcs,
16451653
countof(js_std_funcs));
@@ -3275,7 +3283,6 @@ typedef struct {
32753283
uint64_t buf[];
32763284
} JSSABHeader;
32773285

3278-
static JSClassID js_worker_class_id;
32793286
static JSContext *(*js_worker_new_context_func)(JSRuntime *rt);
32803287

32813288
static int atomic_add_int(int *ptr, int v)
@@ -3388,7 +3395,8 @@ static void js_free_port(JSRuntime *rt, JSWorkerMessageHandler *port)
33883395

33893396
static void js_worker_finalizer(JSRuntime *rt, JSValue val)
33903397
{
3391-
JSWorkerData *worker = JS_GetOpaque(val, js_worker_class_id);
3398+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
3399+
JSWorkerData *worker = JS_GetOpaque(val, ts->worker_class_id);
33923400
if (worker) {
33933401
js_free_message_pipe(worker->recv_pipe);
33943402
js_free_message_pipe(worker->send_pipe);
@@ -3447,7 +3455,6 @@ static void *worker_func(void *opaque)
34473455
js_std_loop(ctx);
34483456

34493457
JS_FreeContext(ctx);
3450-
js_std_free_handlers(rt);
34513458
JS_FreeRuntime(rt);
34523459
return NULL;
34533460
}
@@ -3456,18 +3463,20 @@ static JSValue js_worker_ctor_internal(JSContext *ctx, JSValue new_target,
34563463
JSWorkerMessagePipe *recv_pipe,
34573464
JSWorkerMessagePipe *send_pipe)
34583465
{
3466+
JSRuntime *rt = JS_GetRuntime(ctx);
3467+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
34593468
JSValue obj = JS_UNDEFINED, proto;
34603469
JSWorkerData *s;
34613470

34623471
/* create the object */
34633472
if (JS_IsUndefined(new_target)) {
3464-
proto = JS_GetClassProto(ctx, js_worker_class_id);
3473+
proto = JS_GetClassProto(ctx, ts->worker_class_id);
34653474
} else {
34663475
proto = JS_GetPropertyStr(ctx, new_target, "prototype");
34673476
if (JS_IsException(proto))
34683477
goto fail;
34693478
}
3470-
obj = JS_NewObjectProtoClass(ctx, proto, js_worker_class_id);
3479+
obj = JS_NewObjectProtoClass(ctx, proto, ts->worker_class_id);
34713480
JS_FreeValue(ctx, proto);
34723481
if (JS_IsException(obj))
34733482
goto fail;
@@ -3571,7 +3580,9 @@ static JSValue js_worker_ctor(JSContext *ctx, JSValue new_target,
35713580
static JSValue js_worker_postMessage(JSContext *ctx, JSValue this_val,
35723581
int argc, JSValue *argv)
35733582
{
3574-
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, js_worker_class_id);
3583+
JSRuntime *rt = JS_GetRuntime(ctx);
3584+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
3585+
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, ts->worker_class_id);
35753586
JSWorkerMessagePipe *ps;
35763587
size_t data_len, i;
35773588
uint8_t *data;
@@ -3650,7 +3661,7 @@ static JSValue js_worker_set_onmessage(JSContext *ctx, JSValue this_val,
36503661
{
36513662
JSRuntime *rt = JS_GetRuntime(ctx);
36523663
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
3653-
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, js_worker_class_id);
3664+
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, ts->worker_class_id);
36543665
JSWorkerMessageHandler *port;
36553666

36563667
if (!worker)
@@ -3682,7 +3693,9 @@ static JSValue js_worker_set_onmessage(JSContext *ctx, JSValue this_val,
36823693

36833694
static JSValue js_worker_get_onmessage(JSContext *ctx, JSValue this_val)
36843695
{
3685-
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, js_worker_class_id);
3696+
JSRuntime *rt = JS_GetRuntime(ctx);
3697+
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
3698+
JSWorkerData *worker = JS_GetOpaque2(ctx, this_val, ts->worker_class_id);
36863699
JSWorkerMessageHandler *port;
36873700
if (!worker)
36883701
return JS_EXCEPTION;
@@ -3835,16 +3848,16 @@ static int js_os_init(JSContext *ctx, JSModuleDef *m)
38353848
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
38363849
JSValue proto, obj;
38373850
/* Worker class */
3838-
JS_NewClassID(rt, &js_worker_class_id);
3839-
JS_NewClass(rt, js_worker_class_id, &js_worker_class);
3851+
JS_NewClassID(rt, &ts->worker_class_id);
3852+
JS_NewClass(rt, ts->worker_class_id, &js_worker_class);
38403853
proto = JS_NewObject(ctx);
38413854
JS_SetPropertyFunctionList(ctx, proto, js_worker_proto_funcs, countof(js_worker_proto_funcs));
38423855

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

3847-
JS_SetClassProto(ctx, js_worker_class_id, proto);
3860+
JS_SetClassProto(ctx, ts->worker_class_id, proto);
38483861

38493862
/* set 'Worker.parent' if necessary */
38503863
if (ts->recv_pipe && ts->send_pipe) {
@@ -3954,6 +3967,36 @@ void js_std_add_helpers(JSContext *ctx, int argc, char **argv)
39543967
JS_FreeValue(ctx, global_obj);
39553968
}
39563969

3970+
static void js_std_rt_finalizer(JSRuntime *rt, void *arg)
3971+
{
3972+
JSThreadState *ts = arg;
3973+
struct list_head *el, *el1;
3974+
3975+
list_for_each_safe(el, el1, &ts->os_rw_handlers) {
3976+
JSOSRWHandler *rh = list_entry(el, JSOSRWHandler, link);
3977+
free_rw_handler(rt, rh);
3978+
}
3979+
3980+
list_for_each_safe(el, el1, &ts->os_signal_handlers) {
3981+
JSOSSignalHandler *sh = list_entry(el, JSOSSignalHandler, link);
3982+
free_sh(rt, sh);
3983+
}
3984+
3985+
list_for_each_safe(el, el1, &ts->os_timers) {
3986+
JSOSTimer *th = list_entry(el, JSOSTimer, link);
3987+
free_timer(rt, th);
3988+
}
3989+
3990+
#ifdef USE_WORKER
3991+
/* XXX: free port_list ? */
3992+
js_free_message_pipe(ts->recv_pipe);
3993+
js_free_message_pipe(ts->send_pipe);
3994+
#endif
3995+
3996+
free(ts);
3997+
JS_SetRuntimeOpaque(rt, NULL); /* fail safe */
3998+
}
3999+
39574000
void js_std_init_handlers(JSRuntime *rt)
39584001
{
39594002
JSThreadState *ts;
@@ -3973,6 +4016,7 @@ void js_std_init_handlers(JSRuntime *rt)
39734016
ts->exc = JS_UNDEFINED;
39744017

39754018
JS_SetRuntimeOpaque(rt, ts);
4019+
JS_AddRuntimeFinalizer(rt, js_std_rt_finalizer, ts);
39764020

39774021
#ifdef USE_WORKER
39784022
/* set the SharedArrayBuffer memory handlers */
@@ -3989,32 +4033,6 @@ void js_std_init_handlers(JSRuntime *rt)
39894033

39904034
void js_std_free_handlers(JSRuntime *rt)
39914035
{
3992-
JSThreadState *ts = JS_GetRuntimeOpaque(rt);
3993-
struct list_head *el, *el1;
3994-
3995-
list_for_each_safe(el, el1, &ts->os_rw_handlers) {
3996-
JSOSRWHandler *rh = list_entry(el, JSOSRWHandler, link);
3997-
free_rw_handler(rt, rh);
3998-
}
3999-
4000-
list_for_each_safe(el, el1, &ts->os_signal_handlers) {
4001-
JSOSSignalHandler *sh = list_entry(el, JSOSSignalHandler, link);
4002-
free_sh(rt, sh);
4003-
}
4004-
4005-
list_for_each_safe(el, el1, &ts->os_timers) {
4006-
JSOSTimer *th = list_entry(el, JSOSTimer, link);
4007-
free_timer(rt, th);
4008-
}
4009-
4010-
#ifdef USE_WORKER
4011-
/* XXX: free port_list ? */
4012-
js_free_message_pipe(ts->recv_pipe);
4013-
js_free_message_pipe(ts->send_pipe);
4014-
#endif
4015-
4016-
free(ts);
4017-
JS_SetRuntimeOpaque(rt, NULL); /* fail safe */
40184036
}
40194037

40204038
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)