Skip to content

Commit 82416e4

Browse files
bakpakinbakpakin
committed
Address #1629 - janet_deinit called before threaded channel message sent
to thread. If we take a reference to another thread inside channel code, make sure that we increase the refcount to avoid a use after free.
1 parent ae51434 commit 82416e4

File tree

5 files changed

+117
-9
lines changed

5 files changed

+117
-9
lines changed

‎meson.build‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,7 @@ test_files = [
281281
'test/suite-corelib.janet',
282282
'test/suite-debug.janet',
283283
'test/suite-ev.janet',
284+
'test/suite-ev2.janet',
284285
'test/suite-ffi.janet',
285286
'test/suite-filewatch.janet',
286287
'test/suite-inttypes.janet',

‎src/core/ev.c‎

Lines changed: 50 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -839,6 +839,34 @@ static int janet_chanat_gc(void *p, size_t s) {
839839
return 0;
840840
}
841841

842+
static void janet_chanat_remove_vmref(JanetQueue *fq) {
843+
JanetChannelPending *pending = fq->data;
844+
if (fq->head <= fq->tail) {
845+
for (int32_t i = fq->head; i < fq->tail; i++) {
846+
if (pending[i].thread == &janet_vm) pending[i].thread = NULL;
847+
}
848+
} else {
849+
for (int32_t i = fq->head; i < fq->capacity; i++) {
850+
if (pending[i].thread == &janet_vm) pending[i].thread = NULL;
851+
}
852+
for (int32_t i = 0; i < fq->tail; i++) {
853+
if (pending[i].thread == &janet_vm) pending[i].thread = NULL;
854+
}
855+
}
856+
}
857+
858+
static int janet_chanat_gcperthread(void *p, size_t s) {
859+
(void) s;
860+
JanetChannel *chan = p;
861+
janet_chan_lock(chan);
862+
/* Make sure that the internals of the threaded channel no longer reference _this_ thread. Replace
863+
* those references with NULL. */
864+
janet_chanat_remove_vmref(&chan->read_pending);
865+
janet_chanat_remove_vmref(&chan->write_pending);
866+
janet_chan_unlock(chan);
867+
return 0;
868+
}
869+
842870
static void janet_chanat_mark_fq(JanetQueue *fq) {
843871
JanetChannelPending *pending = fq->data;
844872
if (fq->head <= fq->tail) {
@@ -921,27 +949,31 @@ static void janet_thread_chan_cb(JanetEVGenericMessage msg) {
921949
int is_read = (mode == JANET_CP_MODE_CHOICE_READ) || (mode == JANET_CP_MODE_READ);
922950
if (is_read) {
923951
JanetChannelPending reader;
924-
if (!janet_q_pop(&channel->read_pending, &reader, sizeof(reader))) {
952+
while (!janet_q_pop(&channel->read_pending, &reader, sizeof(reader))) {
925953
JanetVM *vm = reader.thread;
954+
if (!vm) continue;
926955
JanetEVGenericMessage msg;
927956
msg.tag = reader.mode;
928957
msg.fiber = reader.fiber;
929958
msg.argi = (int32_t) reader.sched_id;
930959
msg.argp = channel;
931960
msg.argj = x;
932961
janet_ev_post_event(vm, janet_thread_chan_cb, msg);
962+
break;
933963
}
934964
} else {
935965
JanetChannelPending writer;
936-
if (!janet_q_pop(&channel->write_pending, &writer, sizeof(writer))) {
966+
while (!janet_q_pop(&channel->write_pending, &writer, sizeof(writer))) {
937967
JanetVM *vm = writer.thread;
968+
if (!vm) continue;
938969
JanetEVGenericMessage msg;
939970
msg.tag = writer.mode;
940971
msg.fiber = writer.fiber;
941972
msg.argi = (int32_t) writer.sched_id;
942973
msg.argp = channel;
943974
msg.argj = janet_wrap_nil();
944975
janet_ev_post_event(vm, janet_thread_chan_cb, msg);
976+
break;
945977
}
946978
}
947979
}
@@ -1005,7 +1037,9 @@ static int janet_channel_push_with_lock(JanetChannel *channel, Janet x, int mode
10051037
msg.argi = (int32_t) reader.sched_id;
10061038
msg.argp = channel;
10071039
msg.argj = x;
1008-
janet_ev_post_event(vm, janet_thread_chan_cb, msg);
1040+
if (vm) {
1041+
janet_ev_post_event(vm, janet_thread_chan_cb, msg);
1042+
}
10091043
} else {
10101044
if (reader.mode == JANET_CP_MODE_CHOICE_READ) {
10111045
janet_schedule(reader.fiber, make_read_result(channel, x));
@@ -1060,7 +1094,9 @@ static int janet_channel_pop_with_lock(JanetChannel *channel, Janet *item, int i
10601094
msg.argi = (int32_t) writer.sched_id;
10611095
msg.argp = channel;
10621096
msg.argj = janet_wrap_nil();
1063-
janet_ev_post_event(vm, janet_thread_chan_cb, msg);
1097+
if (vm) {
1098+
janet_ev_post_event(vm, janet_thread_chan_cb, msg);
1099+
}
10641100
} else {
10651101
if (writer.mode == JANET_CP_MODE_CHOICE_WRITE) {
10661102
janet_schedule(writer.fiber, make_write_result(channel));
@@ -1324,7 +1360,9 @@ JANET_CORE_FN(cfun_channel_close,
13241360
msg.tag = JANET_CP_MODE_CLOSE;
13251361
msg.argi = (int32_t) writer.sched_id;
13261362
msg.argj = janet_wrap_nil();
1327-
janet_ev_post_event(vm, janet_thread_chan_cb, msg);
1363+
if (vm) {
1364+
janet_ev_post_event(vm, janet_thread_chan_cb, msg);
1365+
}
13281366
} else {
13291367
if (janet_fiber_can_resume(writer.fiber)) {
13301368
if (writer.mode == JANET_CP_MODE_CHOICE_WRITE) {
@@ -1345,7 +1383,9 @@ JANET_CORE_FN(cfun_channel_close,
13451383
msg.tag = JANET_CP_MODE_CLOSE;
13461384
msg.argi = (int32_t) reader.sched_id;
13471385
msg.argj = janet_wrap_nil();
1348-
janet_ev_post_event(vm, janet_thread_chan_cb, msg);
1386+
if (vm) {
1387+
janet_ev_post_event(vm, janet_thread_chan_cb, msg);
1388+
}
13491389
} else {
13501390
if (janet_fiber_can_resume(reader.fiber)) {
13511391
if (reader.mode == JANET_CP_MODE_CHOICE_READ) {
@@ -1438,7 +1478,10 @@ const JanetAbstractType janet_channel_type = {
14381478
NULL, /* compare */
14391479
NULL, /* hash */
14401480
janet_chanat_next,
1441-
JANET_ATEND_NEXT
1481+
NULL, /* call */
1482+
NULL, /* length */
1483+
NULL, /* bytes */
1484+
janet_chanat_gcperthread
14421485
};
14431486

14441487
/* Main event loop */

‎src/core/gc.c‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -497,9 +497,13 @@ void janet_sweep() {
497497
/* If not visited... */
498498
if (!janet_truthy(items[i].value)) {
499499
void *abst = janet_unwrap_abstract(items[i].key);
500+
JanetAbstractHead *head = janet_abstract_head(abst);
501+
/* Optional per-thread finalizer */
502+
if (head->type->gcperthread) {
503+
janet_assert(!head->type->gcperthread(head->data, head->size), "finalizer failed");
504+
}
500505
if (0 == janet_abstract_decref(abst)) {
501506
/* Run finalizer */
502-
JanetAbstractHead *head = janet_abstract_head(abst);
503507
if (head->type->gc) {
504508
janet_assert(!head->type->gc(head->data, head->size), "finalizer failed");
505509
}

‎src/include/janet.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1188,6 +1188,7 @@ struct JanetAbstractType {
11881188
Janet(*call)(void *p, int32_t argc, Janet *argv);
11891189
size_t (*length)(void *p, size_t len);
11901190
JanetByteView(*bytes)(void *p, size_t len);
1191+
int (*gcperthread)(void *data, size_t len);
11911192
};
11921193

11931194
/* Some macros to let us add extra types to JanetAbstract types without
@@ -1207,7 +1208,8 @@ struct JanetAbstractType {
12071208
#define JANET_ATEND_NEXT NULL,JANET_ATEND_CALL
12081209
#define JANET_ATEND_CALL NULL,JANET_ATEND_LENGTH
12091210
#define JANET_ATEND_LENGTH NULL,JANET_ATEND_BYTES
1210-
#define JANET_ATEND_BYTES
1211+
#define JANET_ATEND_BYTES NULL,JANET_ATEND_GCPERTHREAD
1212+
#define JANET_ATEND_GCPERTHREAD
12111213

12121214
struct JanetReg {
12131215
const char *name;

‎test/suite-ev2.janet‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
# Copyright (c) 2025 Calvin Rose & contributors
2+
#
3+
# Permission is hereby granted, free of charge, to any person obtaining a copy
4+
# of this software and associated documentation files (the "Software"), to
5+
# deal in the Software without restriction, including without limitation the
6+
# rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
7+
# sell copies of the Software, and to permit persons to whom the Software is
8+
# furnished to do so, subject to the following conditions:
9+
#
10+
# The above copyright notice and this permission notice shall be included in
11+
# all copies or substantial portions of the Software.
12+
#
13+
# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
14+
# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
15+
# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
16+
# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
17+
# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
18+
# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
19+
# IN THE SOFTWARE.
20+
21+
(import ./helper :prefix "" :exit true)
22+
(start-suite)
23+
24+
# Issue #1629
25+
(def thread-channel (ev/thread-chan 100))
26+
(def super (ev/thread-chan 10))
27+
(defn worker []
28+
(while true
29+
(def item (ev/take thread-channel))
30+
(when (= item :deadline)
31+
(ev/deadline 0.1 nil (fiber/current) true))))
32+
(ev/thread worker nil :n super)
33+
(ev/give thread-channel :item)
34+
(ev/sleep 0.05)
35+
(ev/give thread-channel :item)
36+
(ev/sleep 0.05)
37+
(ev/give thread-channel :deadline)
38+
(ev/sleep 0.05)
39+
(ev/give thread-channel :item)
40+
(ev/sleep 0.05)
41+
(ev/give thread-channel :item)
42+
(ev/sleep 0.15)
43+
(assert (deep= '(:error "deadline expired" nil) (ev/take super)) "deadline expirataion")
44+
45+
# Another variant
46+
(def thread-channel (ev/thread-chan 100))
47+
(def super (ev/thread-chan 10))
48+
(defn worker []
49+
(while true
50+
(def item (ev/take thread-channel))
51+
(when (= item :deadline)
52+
(ev/deadline 0.1 nil (fiber/current) true))))
53+
(ev/thread worker nil :n super)
54+
(ev/give thread-channel :deadline)
55+
(ev/sleep 0.2)
56+
(assert (deep= '(:error "deadline expired" nil) (ev/take super)) "deadline expirataion")
57+
58+
(end-suite)

0 commit comments

Comments
 (0)