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 6CCACD65441; Fri, 13 Sep 2024 17:45:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6CCACD65441 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1726238728; bh=YUJTO+ZYbW7z6Hd4gbNL+/xOY2Ki+fuJcus8FZlIiKo=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=vjYK36IFQmhe9kVuLBhbercJSy2ipM7xmbAre6TAkJqzFk3w6xm5dzpbi0TUGaAb3 UDniutf4R7duP6PDo0IwjriZ+y5KfoQmrlTnRhxQvQ6tKtDgkZVmwc8KAXV02u+29j ndYf0G4qO9zYrOiWGDsFREe1ODeXESpFTEE5Th1g= Received: from smtp35.i.mail.ru (smtp35.i.mail.ru [95.163.41.76]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 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 636B3D65441 for ; Fri, 13 Sep 2024 17:45:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 636B3D65441 Received: by smtp35.i.mail.ru with esmtpa (envelope-from ) id 1sp7Y5-00000000Q2f-1Tyh; Fri, 13 Sep 2024 17:45:25 +0300 Content-Type: multipart/alternative; boundary="------------k7qe0J5aqSGA6MNVk7C2aLpn" Message-ID: Date: Fri, 13 Sep 2024 17:45:25 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun , Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org References: <20240913080550.22599-1-skaplun@tarantool.org> In-Reply-To: <20240913080550.22599-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9149934E261B3C850BC57BA9F42E0002904EF93859B3DA84E182A05F53808504048A828C55C34A2543DE06ABAFEAF67054EBBDB7714DD9B2AD275C5A0189E2CE32EB2DA79D556EF8B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72E4E5201E1C2E308EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063754D7F012132A27898638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8A806A56731BD212F29E0B76BD3C28C5AB80AD91EF48BCCE4CC7F00164DA146DAFE8445B8C89999728AA50765F790063783E00425F71A4181389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC821E93C0F2A571C7BF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C632EDEA9CD5989A39735652A29929C6C4AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C31DA0D137B259B101BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE702706FBA10211704731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A545A7F18C3B89DE065002B1117B3ED6965B07925793B563868B25839F35DFE037823CB91A9FED034534781492E4B8EEAD4ECBDE8281F904F9BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF69CBC7EBC6CB8E6FE1F10CEE1B11C47EC5471B1BC9421CEDD779D8D0E7E9A6E7F61CD9246542D0C4BEF149BEF2827C682ED480BE642AE96D335E3FE52646C3383F9D8FA6B467EF745F4332CA8FE04980913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojmn7hiCWKTuh+biDrJ8bUqA== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D61402C1DB9BB62D2B30A4BD03525DDD6C8964CB78DA61F45638C0152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] FFI: Workaround for platform dlerror() returning NULL. 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" This is a multi-part message in MIME format. --------------k7qe0J5aqSGA6MNVk7C2aLpn Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey, thanks for the patch! LGTM with a minor comment below. On 13.09.2024 11:05, Sergey Kaplun wrote: > From: Mike Pall > > Contributed by mcclure. > > (cherry picked from commit 478bcfe52a653bf338f17690147fa9f5793f5b42) > > The `ffi.load()` implementation assumes the string returned from > `dlerror()` is non-NULL and immediately dereferences it. This may lead > to a crash on some platforms like Android (Oculus Quest) where it is > possible. According to a POSIX standard, it is not Android-specific behaviour [1]: > If no dynamic linking errors have occurred since the last invocation of /dlerror/(), /dlerror/() shall return NULL. 1. https://pubs.opengroup.org/onlinepubs/009695399/functions/dlerror.html > > This patch adds the corresponding check and the default "dlopen failed" > error message. > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#10199 > --- > > Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-522-fix-dlerror-return-null > Related Issues: > *https://github.com/LuaJIT/LuaJIT/pull/522 > *https://github.com/tarantool/tarantool/issues/10199 > > src/lj_clib.c | 3 ++- > test/tarantool-tests/CMakeLists.txt | 1 + > .../lj-522-fix-dlerror-return-null.test.lua | 27 +++++++++++++++++++ > .../CMakeLists.txt | 1 + > .../mydlerror.c | 6 +++++ > .../lj-522-fix-dlerror-return-null/script.lua | 10 +++++++ > 6 files changed, 47 insertions(+), 1 deletion(-) > create mode 100644 test/tarantool-tests/lj-522-fix-dlerror-return-null.test.lua > create mode 100644 test/tarantool-tests/lj-522-fix-dlerror-return-null/CMakeLists.txt > create mode 100644 test/tarantool-tests/lj-522-fix-dlerror-return-null/mydlerror.c > create mode 100644 test/tarantool-tests/lj-522-fix-dlerror-return-null/script.lua > > diff --git a/src/lj_clib.c b/src/lj_clib.c > index 2f11b2e9..9716684c 100644 > --- a/src/lj_clib.c > +++ b/src/lj_clib.c > @@ -119,12 +119,13 @@ static void *clib_loadlib(lua_State *L, const char *name, int global) > RTLD_LAZY | (global?RTLD_GLOBAL:RTLD_LOCAL)); > if (!h) { > const char *e, *err = dlerror(); > - if (*err == '/' && (e = strchr(err, ':')) && > + if (err && *err == '/' && (e = strchr(err, ':')) && > (name = clib_resolve_lds(L, strdata(lj_str_new(L, err, e-err))))) { > h = dlopen(name, RTLD_LAZY | (global?RTLD_GLOBAL:RTLD_LOCAL)); > if (h) return h; > err = dlerror(); > } > + if (!err) err = "dlopen failed"; > lj_err_callermsg(L, err); > } > return h; > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > index e3750bf3..0bd3e6fc 100644 > --- a/test/tarantool-tests/CMakeLists.txt > +++ b/test/tarantool-tests/CMakeLists.txt > @@ -28,6 +28,7 @@ add_subdirectory(gh-5813-resolving-of-c-symbols/stripped) > add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64) > add_subdirectory(gh-6189-cur_L) > add_subdirectory(lj-416-xor-before-jcc) > +add_subdirectory(lj-522-fix-dlerror-return-null) > add_subdirectory(lj-549-bytecode-loader) > add_subdirectory(lj-551-bytecode-c-broken-macro) > add_subdirectory(lj-601-fix-gc-finderrfunc) > diff --git a/test/tarantool-tests/lj-522-fix-dlerror-return-null.test.lua b/test/tarantool-tests/lj-522-fix-dlerror-return-null.test.lua > new file mode 100644 > index 00000000..f9f4af6a > --- /dev/null > +++ b/test/tarantool-tests/lj-522-fix-dlerror-return-null.test.lua > @@ -0,0 +1,27 @@ > +local tap = require('tap') > +local test = tap.test('lj-522-fix-dlerror-return-null'):skipcond({ > + -- XXX: Unfortunately, it's too hard to overload (or even > + -- impossible, who knows, since Cupertino fellows do not > + -- provide any information about their system) something from > + -- dyld sharing cache (includes the library > + -- providing `dlerror()`). > + -- All in all, this test checks the part that is common for all > + -- platforms, so it's not vital to run this test on macOS since > + -- everything can be checked on Linux in a much easier way. > + [' cannot be overridden on macOS'] = jit.os == 'OSX', > +}) > + > +test:plan(1) > + > +-- `makecmd()` runs <%testname%/script.lua> by > +-- `LUAJIT_TEST_BINARY` with the given environment and launch > +-- options. > +local script = require('utils').exec.makecmd(arg, { > + env = { LD_PRELOAD = 'mydlerror.so' }, > + redirect = '2>&1', > +}) > + > +local output = script() > +test:like(output, 'dlopen failed', 'correct error message') > + > +test:done(true) > diff --git a/test/tarantool-tests/lj-522-fix-dlerror-return-null/CMakeLists.txt b/test/tarantool-tests/lj-522-fix-dlerror-return-null/CMakeLists.txt > new file mode 100644 > index 00000000..903da4d3 > --- /dev/null > +++ b/test/tarantool-tests/lj-522-fix-dlerror-return-null/CMakeLists.txt > @@ -0,0 +1 @@ > +BuildTestCLib(mydlerror mydlerror.c) > diff --git a/test/tarantool-tests/lj-522-fix-dlerror-return-null/mydlerror.c b/test/tarantool-tests/lj-522-fix-dlerror-return-null/mydlerror.c > new file mode 100644 > index 00000000..89e881e4 > --- /dev/null > +++ b/test/tarantool-tests/lj-522-fix-dlerror-return-null/mydlerror.c > @@ -0,0 +1,6 @@ > +#include > + > +char *dlerror(void) > +{ > + return NULL; > +} > diff --git a/test/tarantool-tests/lj-522-fix-dlerror-return-null/script.lua b/test/tarantool-tests/lj-522-fix-dlerror-return-null/script.lua > new file mode 100644 > index 00000000..b2f673c9 > --- /dev/null > +++ b/test/tarantool-tests/lj-522-fix-dlerror-return-null/script.lua > @@ -0,0 +1,10 @@ > +local ffi = require('ffi') > + > +-- Overloaded `dlerror()` returns NULL after trying to load an > +-- unexisting file. > +local res, errmsg = pcall(ffi.load, 'lj-522-fix-dlerror-return-null-unexisted') > + > +assert(not res, 'pcall should fail') > + > +-- Return the error message to be checked by the TAP. > +io.write(errmsg) --------------k7qe0J5aqSGA6MNVk7C2aLpn Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey,

thanks for the patch! LGTM with a minor comment below.

On 13.09.2024 11:05, Sergey Kaplun wrote:
From: Mike Pall <mike>

Contributed by mcclure.

(cherry picked from commit 478bcfe52a653bf338f17690147fa9f5793f5b42)

The `ffi.load()` implementation assumes the string returned from
`dlerror()` is non-NULL and immediately dereferences it. This may lead
to a crash on some platforms like Android (Oculus Quest) where it is
possible.

According to a POSIX standard, it is not Android-specific behaviour [1]:

> If no dynamic linking errors have occurred since the last invocation of dlerror(), dlerror() shall return NULL.

1. https://pubs.opengroup.org/onlinepubs/009695399/functions/dlerror.html


This patch adds the corresponding check and the default "dlopen failed"
error message.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#10199
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-522-fix-dlerror-return-null
Related Issues:
* https://github.com/LuaJIT/LuaJIT/pull/522
* https://github.com/tarantool/tarantool/issues/10199

 src/lj_clib.c                                 |  3 ++-
 test/tarantool-tests/CMakeLists.txt           |  1 +
 .../lj-522-fix-dlerror-return-null.test.lua   | 27 +++++++++++++++++++
 .../CMakeLists.txt                            |  1 +
 .../mydlerror.c                               |  6 +++++
 .../lj-522-fix-dlerror-return-null/script.lua | 10 +++++++
 6 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-522-fix-dlerror-return-null.test.lua
 create mode 100644 test/tarantool-tests/lj-522-fix-dlerror-return-null/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-522-fix-dlerror-return-null/mydlerror.c
 create mode 100644 test/tarantool-tests/lj-522-fix-dlerror-return-null/script.lua

diff --git a/src/lj_clib.c b/src/lj_clib.c
index 2f11b2e9..9716684c 100644
--- a/src/lj_clib.c
+++ b/src/lj_clib.c
@@ -119,12 +119,13 @@ static void *clib_loadlib(lua_State *L, const char *name, int global)
 		   RTLD_LAZY | (global?RTLD_GLOBAL:RTLD_LOCAL));
   if (!h) {
     const char *e, *err = dlerror();
-    if (*err == '/' && (e = strchr(err, ':')) &&
+    if (err && *err == '/' && (e = strchr(err, ':')) &&
 	(name = clib_resolve_lds(L, strdata(lj_str_new(L, err, e-err))))) {
       h = dlopen(name, RTLD_LAZY | (global?RTLD_GLOBAL:RTLD_LOCAL));
       if (h) return h;
       err = dlerror();
     }
+    if (!err) err = "dlopen failed";
     lj_err_callermsg(L, err);
   }
   return h;
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index e3750bf3..0bd3e6fc 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -28,6 +28,7 @@ add_subdirectory(gh-5813-resolving-of-c-symbols/stripped)
 add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64)
 add_subdirectory(gh-6189-cur_L)
 add_subdirectory(lj-416-xor-before-jcc)
+add_subdirectory(lj-522-fix-dlerror-return-null)
 add_subdirectory(lj-549-bytecode-loader)
 add_subdirectory(lj-551-bytecode-c-broken-macro)
 add_subdirectory(lj-601-fix-gc-finderrfunc)
diff --git a/test/tarantool-tests/lj-522-fix-dlerror-return-null.test.lua b/test/tarantool-tests/lj-522-fix-dlerror-return-null.test.lua
new file mode 100644
index 00000000..f9f4af6a
--- /dev/null
+++ b/test/tarantool-tests/lj-522-fix-dlerror-return-null.test.lua
@@ -0,0 +1,27 @@
+local tap = require('tap')
+local test = tap.test('lj-522-fix-dlerror-return-null'):skipcond({
+  -- XXX: Unfortunately, it's too hard to overload (or even
+  -- impossible, who knows, since Cupertino fellows do not
+  -- provide any information about their system) something from
+  -- dyld sharing cache (includes the <libdyld.dylib> library
+  -- providing `dlerror()`).
+  -- All in all, this test checks the part that is common for all
+  -- platforms, so it's not vital to run this test on macOS since
+  -- everything can be checked on Linux in a much easier way.
+  ['<dlerror> cannot be overridden on macOS'] = jit.os == 'OSX',
+})
+
+test:plan(1)
+
+-- `makecmd()` runs <%testname%/script.lua> by
+-- `LUAJIT_TEST_BINARY` with the given environment and launch
+-- options.
+local script = require('utils').exec.makecmd(arg, {
+  env = { LD_PRELOAD = 'mydlerror.so' },
+  redirect = '2>&1',
+})
+
+local output = script()
+test:like(output, 'dlopen failed', 'correct error message')
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-522-fix-dlerror-return-null/CMakeLists.txt b/test/tarantool-tests/lj-522-fix-dlerror-return-null/CMakeLists.txt
new file mode 100644
index 00000000..903da4d3
--- /dev/null
+++ b/test/tarantool-tests/lj-522-fix-dlerror-return-null/CMakeLists.txt
@@ -0,0 +1 @@
+BuildTestCLib(mydlerror mydlerror.c)
diff --git a/test/tarantool-tests/lj-522-fix-dlerror-return-null/mydlerror.c b/test/tarantool-tests/lj-522-fix-dlerror-return-null/mydlerror.c
new file mode 100644
index 00000000..89e881e4
--- /dev/null
+++ b/test/tarantool-tests/lj-522-fix-dlerror-return-null/mydlerror.c
@@ -0,0 +1,6 @@
+#include <stddef.h>
+
+char *dlerror(void)
+{
+	return NULL;
+}
diff --git a/test/tarantool-tests/lj-522-fix-dlerror-return-null/script.lua b/test/tarantool-tests/lj-522-fix-dlerror-return-null/script.lua
new file mode 100644
index 00000000..b2f673c9
--- /dev/null
+++ b/test/tarantool-tests/lj-522-fix-dlerror-return-null/script.lua
@@ -0,0 +1,10 @@
+local ffi = require('ffi')
+
+-- Overloaded `dlerror()` returns NULL after trying to load an
+-- unexisting file.
+local res, errmsg = pcall(ffi.load, 'lj-522-fix-dlerror-return-null-unexisted')
+
+assert(not res, 'pcall should fail')
+
+-- Return the error message to be checked by the TAP.
+io.write(errmsg)
--------------k7qe0J5aqSGA6MNVk7C2aLpn--