From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id F26ADE3F09E; Mon, 8 Jul 2024 15:28:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F26ADE3F09E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1720441702; bh=ph9c0ArQlnEGw0oCO0r7zBBo2wkWc1dUNGIfXEidNzA=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=DEsjhwNf/6dNtfjAuXFd/WcZzMeh7B4rgztKxZeuWP5UZHkqJydxHQQ3kwh+PoFts P+SIIKOJt+PX8tR8Ar7uoNFFqRofraI71Q+pqIM97A9waknVxflTGK6BobZ7TXGObj 6WU3CE0xddeEJW+Sn68B4/wAbDRNPMy2X/9KB1pE= Received: from mail-lf1-f54.google.com (mail-lf1-f54.google.com [209.85.167.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D934CE3F0A0 for ; Mon, 8 Jul 2024 15:27:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D934CE3F0A0 Received: by mail-lf1-f54.google.com with SMTP id 2adb3069b0e04-52e9c55febcso4910202e87.2 for ; Mon, 08 Jul 2024 05:27:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720441663; x=1721046463; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=jbIKLSTBFk1sN7TTOAQQlNN8una7D3WnlePWa4a0M8I=; b=iwXg1NINAWmWTMMdYsnEZHxU+mkhi6bKAe8j6TCt74qghwRAqi7nCvn8crbBiR0Zf5 iSamFaVMb5YxTVEfEk7kVqFFy0sXPT8M00dQp17hALr+FyanusEXsfLBaKTK7qojFwNm Ao9/UZ0PURas9ksfLrkLOziQtbDunfJ7O379kK3OTUpR8nxAsFodJaGAEcw5Gem5CUHK E4nhS0vG/ncnHNBeJk2QCxgY5CnTyshy5sahpwdnRxo8i2alWoW1Gc4Se++5jgcM0VAV 5ZKUiOlnZrKUCKVOdpxkccfkadcGQ5pNIyN4FsJNcbgfGgo8ccy+Egxa7VQbIv9N41jV 13ag== X-Gm-Message-State: AOJu0Yx/6EvmIYR15lb3YfmTxwTBEmAudUYAamZd9E0EDMNCgVV2w30I uG5BvLU35BxWAk0eEZgamCrB+5yBH0ADUHe1ygO1XTvOjc2kIafaZ1uRkUki X-Google-Smtp-Source: AGHT+IFzHX3cmSDmQw9lRlcM52JCOddAx1JmYmjJLrPzB/r9BK3UQm+eKHXFDD05EJewaLTrO1Uksg== X-Received: by 2002:a19:f805:0:b0:52c:da18:618c with SMTP id 2adb3069b0e04-52ea06e4ce5mr7283040e87.45.1720441662554; Mon, 08 Jul 2024 05:27:42 -0700 (PDT) Received: from pony.mail.msk ([5.181.62.98]) by smtp.gmail.com with ESMTPSA id 2adb3069b0e04-52eab036662sm603876e87.4.2024.07.08.05.27.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Jul 2024 05:27:42 -0700 (PDT) To: tarantool-patches@dev.tarantool.org, Sergey Kaplun , Maxim Kokryashkin Date: Mon, 8 Jul 2024 15:26:14 +0300 Message-Id: X-Mailer: git-send-email 2.34.1 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH luajit 2/2] FFI: Turn FFI finalizer table into a proper GC root. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" From: Mike Pall Reported by Sergey Bronnikov. (cherry picked from commit f5affaa6c4e7524e661484f22f24255f9a83eb47) Previous patch fixes the problem partially because the introduced GC root may not exist at the start phase of the GC cycle. In that case, the cdata finalizer table will be collected at the end of the cycle. Access to the cdata finalizer table exhibits heap use after free. The patch is turned the finalizer table into a proper GC root. Sergey Bronnikov: * added the description and the tests for the problem Part of tarantool/tarantool#10199 --- src/lib_ffi.c | 20 +-------- src/lj_cdata.c | 2 +- src/lj_ctype.c | 12 ++++++ src/lj_ctype.h | 2 +- src/lj_gc.c | 41 ++++++++---------- src/lj_obj.h | 3 ++ src/lj_state.c | 3 ++ ...free-on-access-to-CTState-finalizer.test.c | 42 +++++++++++++++++++ 8 files changed, 81 insertions(+), 44 deletions(-) diff --git a/src/lib_ffi.c b/src/lib_ffi.c index 7ed6fc78..3c8dd77f 100644 --- a/src/lib_ffi.c +++ b/src/lib_ffi.c @@ -513,7 +513,7 @@ LJLIB_CF(ffi_new) LJLIB_REC(.) /* Handle ctype __gc metamethod. Use the fast lookup here. */ cTValue *tv = lj_tab_getinth(cts->miscmap, -(int32_t)id); if (tv && tvistab(tv) && (tv = lj_meta_fast(L, tabV(tv), MM_gc))) { - GCtab *t = cts->finalizer; + GCtab *t = tabref(G(L)->gcroot[GCROOT_FFI_FIN]); if (gcref(t->metatable)) { /* Add to finalizer table, if still enabled. */ copyTV(L, lj_tab_set(L, t, o-1), tv); @@ -762,7 +762,7 @@ LJLIB_CF(ffi_abi) LJLIB_REC(.) return 1; } -LJLIB_PUSH(top-8) LJLIB_SET(!) /* Store reference to miscmap table. */ +LJLIB_PUSH(top-7) LJLIB_SET(!) /* Store reference to miscmap table. */ LJLIB_CF(ffi_metatype) { @@ -788,8 +788,6 @@ LJLIB_CF(ffi_metatype) return 1; } -LJLIB_PUSH(top-7) LJLIB_SET(!) /* Store reference to finalizer table. */ - LJLIB_CF(ffi_gc) LJLIB_REC(.) { GCcdata *cd = ffi_checkcdata(L, 1); @@ -822,19 +820,6 @@ LJLIB_PUSH(top-2) LJLIB_SET(arch) /* ------------------------------------------------------------------------ */ -/* Create special weak-keyed finalizer table. */ -static GCtab *ffi_finalizer(lua_State *L) -{ - /* NOBARRIER: The table is new (marked white). */ - GCtab *t = lj_tab_new(L, 0, 1); - settabV(L, L->top++, t); - setgcref(t->metatable, obj2gco(t)); - setstrV(L, lj_tab_setstr(L, t, lj_str_newlit(L, "__mode")), - lj_str_newlit(L, "k")); - t->nomm = (uint8_t)(~(1u<top++, (cts->miscmap = lj_tab_new(L, 0, 1))); - cts->finalizer = ffi_finalizer(L); LJ_LIB_REG(L, NULL, ffi_meta); /* NOBARRIER: basemt is a GC root. */ setgcref(basemt_it(G(L), LJ_TCDATA), obj2gco(tabV(L->top-1))); diff --git a/src/lj_cdata.c b/src/lj_cdata.c index 35d0e76a..3d6ff1cc 100644 --- a/src/lj_cdata.c +++ b/src/lj_cdata.c @@ -89,7 +89,7 @@ void LJ_FASTCALL lj_cdata_free(global_State *g, GCcdata *cd) void lj_cdata_setfin(lua_State *L, GCcdata *cd, GCobj *obj, uint32_t it) { - GCtab *t = ctype_ctsG(G(L))->finalizer; + GCtab *t = tabref(G(L)->gcroot[GCROOT_FFI_FIN]); if (gcref(t->metatable)) { /* Add cdata to finalizer table, if still enabled. */ TValue *tv, tmp; diff --git a/src/lj_ctype.c b/src/lj_ctype.c index 53b83031..c0213629 100644 --- a/src/lj_ctype.c +++ b/src/lj_ctype.c @@ -643,6 +643,18 @@ CTState *lj_ctype_init(lua_State *L) return cts; } +/* Create special weak-keyed finalizer table. */ +void lj_ctype_initfin(lua_State *L) +{ + /* NOBARRIER: The table is new (marked white). */ + GCtab *t = lj_tab_new(L, 0, 1); + setgcref(t->metatable, obj2gco(t)); + setstrV(L, lj_tab_setstr(L, t, lj_str_newlit(L, "__mode")), + lj_str_newlit(L, "k")); + t->nomm = (uint8_t)(~(1u<gcroot[GCROOT_FFI_FIN], obj2gco(t)); +} + /* Free C type table and state. */ void lj_ctype_freestate(global_State *g) { diff --git a/src/lj_ctype.h b/src/lj_ctype.h index 8edbd561..2d393eb9 100644 --- a/src/lj_ctype.h +++ b/src/lj_ctype.h @@ -177,7 +177,6 @@ typedef struct CTState { MSize sizetab; /* Size of C type table. */ lua_State *L; /* Lua state (needed for errors and allocations). */ global_State *g; /* Global state. */ - GCtab *finalizer; /* Map of cdata to finalizer. */ GCtab *miscmap; /* Map of -CTypeID to metatable and cb slot to func. */ CCallback cb; /* Temporary callback state. */ CTypeID1 hash[CTHASH_SIZE]; /* Hash anchors for C type table. */ @@ -473,6 +472,7 @@ LJ_FUNC GCstr *lj_ctype_repr(lua_State *L, CTypeID id, GCstr *name); LJ_FUNC GCstr *lj_ctype_repr_int64(lua_State *L, uint64_t n, int isunsigned); LJ_FUNC GCstr *lj_ctype_repr_complex(lua_State *L, void *sp, CTSize size); LJ_FUNC CTState *lj_ctype_init(lua_State *L); +LJ_FUNC void lj_ctype_initfin(lua_State *L); LJ_FUNC void lj_ctype_freestate(global_State *g); #endif diff --git a/src/lj_gc.c b/src/lj_gc.c index 42348a34..4c222f21 100644 --- a/src/lj_gc.c +++ b/src/lj_gc.c @@ -99,9 +99,6 @@ static void gc_mark_start(global_State *g) gc_markobj(g, tabref(mainthread(g)->env)); gc_marktv(g, &g->registrytv); gc_mark_gcroot(g); -#if LJ_HASFFI - if (ctype_ctsG(g)) gc_markobj(g, ctype_ctsG(g)->finalizer); -#endif g->gc.state = GCSpropagate; } @@ -181,8 +178,7 @@ static int gc_traverse_tab(global_State *g, GCtab *t) } if (weak) { /* Weak tables are cleared in the atomic phase. */ #if LJ_HASFFI - CTState *cts = ctype_ctsG(g); - if (cts && cts->finalizer == t) { + if (gcref(g->gcroot[GCROOT_FFI_FIN]) == obj2gco(t)) { weak = (int)(~0u & ~LJ_GC_WEAKVAL); } else #endif @@ -550,7 +546,7 @@ static void gc_finalize(lua_State *L) o->gch.marked &= (uint8_t)~LJ_GC_CDATA_FIN; /* Resolve finalizer. */ setcdataV(L, &tmp, gco2cd(o)); - tv = lj_tab_set(L, ctype_ctsG(g)->finalizer, &tmp); + tv = lj_tab_set(L, tabref(g->gcroot[GCROOT_FFI_FIN]), &tmp); if (!tvisnil(tv)) { g->gc.nocdatafin = 0; copyTV(L, &tmp, tv); @@ -582,23 +578,20 @@ void lj_gc_finalize_udata(lua_State *L) void lj_gc_finalize_cdata(lua_State *L) { global_State *g = G(L); - CTState *cts = ctype_ctsG(g); - if (cts) { - GCtab *t = cts->finalizer; - Node *node = noderef(t->node); - ptrdiff_t i; - setgcrefnull(t->metatable); /* Mark finalizer table as disabled. */ - for (i = (ptrdiff_t)t->hmask; i >= 0; i--) - if (!tvisnil(&node[i].val) && tviscdata(&node[i].key)) { - GCobj *o = gcV(&node[i].key); - TValue tmp; - makewhite(g, o); - o->gch.marked &= (uint8_t)~LJ_GC_CDATA_FIN; - copyTV(L, &tmp, &node[i].val); - setnilV(&node[i].val); - gc_call_finalizer(g, L, &tmp, o); - } - } + GCtab *t = tabref(g->gcroot[GCROOT_FFI_FIN]); + Node *node = noderef(t->node); + ptrdiff_t i; + setgcrefnull(t->metatable); /* Mark finalizer table as disabled. */ + for (i = (ptrdiff_t)t->hmask; i >= 0; i--) + if (!tvisnil(&node[i].val) && tviscdata(&node[i].key)) { + GCobj *o = gcV(&node[i].key); + TValue tmp; + makewhite(g, o); + o->gch.marked &= (uint8_t)~LJ_GC_CDATA_FIN; + copyTV(L, &tmp, &node[i].val); + setnilV(&node[i].val); + gc_call_finalizer(g, L, &tmp, o); + } } #endif @@ -721,7 +714,7 @@ static size_t gc_onestep(lua_State *L) return GCFINALIZECOST; } #if LJ_HASFFI - if (!g->gc.nocdatafin) lj_tab_rehash(L, ctype_ctsG(g)->finalizer); + if (!g->gc.nocdatafin) lj_tab_rehash(L, tabref(g->gcroot[GCROOT_FFI_FIN])); #endif g->gc.state = GCSpause; /* End of GC cycle. */ g->gc.debt = 0; diff --git a/src/lj_obj.h b/src/lj_obj.h index 69e94ff2..06ea0cd0 100644 --- a/src/lj_obj.h +++ b/src/lj_obj.h @@ -580,6 +580,9 @@ typedef enum { GCROOT_BASEMT_NUM = GCROOT_BASEMT + ~LJ_TNUMX, GCROOT_IO_INPUT, /* Userdata for default I/O input file. */ GCROOT_IO_OUTPUT, /* Userdata for default I/O output file. */ +#if LJ_HASFFI + GCROOT_FFI_FIN, /* FFI finalizer table. */ +#endif GCROOT_MAX } GCRootID; diff --git a/src/lj_state.c b/src/lj_state.c index 01d4901a..5a920102 100644 --- a/src/lj_state.c +++ b/src/lj_state.c @@ -180,6 +180,9 @@ static TValue *cpluaopen(lua_State *L, lua_CFunction dummy, void *ud) lj_lex_init(L); fixstring(lj_err_str(L, LJ_ERR_ERRMEM)); /* Preallocate memory error msg. */ g->gc.threshold = 4*g->gc.total; +#if LJ_HASFFI + lj_ctype_initfin(L); +#endif lj_trace_initstate(g); lj_err_verify(); return NULL; diff --git a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c index ad2d8e62..50359a98 100644 --- a/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c +++ b/test/tarantool-c-tests/lj-1168-heap-use-after-free-on-access-to-CTState-finalizer.test.c @@ -94,10 +94,52 @@ static int lua_cdata_finalizers_testcase_part_1(void *test_state) return TEST_EXIT_SUCCESS; } +static int +lua_cdata_finalizers_testcase_part_2(void *test_state) +{ + const char buff[] = "return 1LL"; + + /* Shared Lua state is not needed. */ + (void)test_state; + + /* Setup. */ + lua_State *L = luaL_newstate(); + + /* Set GC at the start. */ + lua_gc(L, LUA_GCCOLLECT, 0); + + /* + * Default step is too big -- one step ends after the + * atomic phase. + */ + lua_gc(L, LUA_GCSETSTEPMUL, 1); + + /* Skip marking roots. */ + lua_gc(L, LUA_GCSTEP, 1); + + /* Not trigger GC during `lua_openffi()`. */ + lua_gc(L, LUA_GCSTOP, 0); + + int res = luaL_loadbufferx(L, buff, sizeof(buff) - 1, "chunk", "t"); + assert_true(res == LUA_OK); + + /* Finish GC cycle. */ + while (!lua_gc(L, LUA_GCSTEP, -1)); + + assert_true(lua_gettop(L) == 1); + + /* Teardown. */ + lua_settop(L, 0); + lua_close(L); + + return TEST_EXIT_SUCCESS; +} + int main(void) { const struct test_unit tgroup[] = { test_unit_def(lua_cdata_finalizers_testcase_part_1), + test_unit_def(lua_cdata_finalizers_testcase_part_2), }; const int test_result = test_run_group(tgroup, NULL); -- 2.34.1