Skip to content

Fix thread-safety issue in quickjs-libc #578

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

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Fix thread-safety issue in quickjs-libc #578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
58 changes: 35 additions & 23 deletions quickjs-libc.c
View file
Open in desktop
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you avoid this by using the system allocator like before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could but this seems neater. Is there a reason to prefer plain malloc?

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion, but we are somewhat contradicting the documentation you just added: the runtime itself is no longer usable. I would've understood calling any RT function (including the memory ones) is a no-no.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do as I say, not as I do, is what I always say :-)

quickjs and quickjs-libc are strongly integrated so I don't think there's a dichotomy here. The doc comment is for downstream users and it's best to keep it simple ("do X" instead of "do X unless Y")


#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
Open in desktop
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
Open in desktop
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
Loading