[Tarantool-patches] [PATCH luajit] FFI: Workaround for platform dlerror() returning NULL.

[Tarantool-patches] [PATCH luajit] FFI: Workaround for platform dlerror() returning NULL.

[Sergey Kaplun via Tarantool-patches]

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.

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

Re: [Tarantool-patches] [PATCH luajit] FFI: Workaround for platform dlerror() returning NULL.

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 6020 bytes --]

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)

[-- Attachment #2: Type: text/html, Size: 6928 bytes --]

Re: [Tarantool-patches] [PATCH luajit] FFI: Workaround for platform dlerror() returning NULL.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On 13.09.24, Sergey Bronnikov wrote:
> 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

Updated the commit message to the following and force-pushed the branch:

| FFI: Workaround for platform dlerror() returning NULL.
|
| 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 POSIX platforms [1] where it is possible.
|
| This patch adds the corresponding check and the default "dlopen failed"
| error message.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| [1]: https://pubs.opengroup.org/onlinepubs/009695399/functions/dlerror.html
|
| Part of tarantool/tarantool#10199


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

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] FFI: Workaround for platform dlerror() returning NULL.

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]

Hi, Sergey,

On 13.09.2024 18:32, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
>
> On 13.09.24, Sergey Bronnikov wrote:
>> 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
> Updated the commit message to the following and force-pushed the branch:
Thanks! LGTM
>
> | FFI: Workaround for platform dlerror() returning NULL.
> |
> | 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 POSIX platforms [1] where it is possible.
> |
> | This patch adds the corresponding check and the default "dlopen failed"
> | error message.
> |
> | Sergey Kaplun:
> | * added the description and the test for the problem
> |
> | [1]:https://pubs.opengroup.org/onlinepubs/009695399/functions/dlerror.html
> |
> | Part of tarantool/tarantool#10199
>
>
>>> 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
>>> ---

[-- Attachment #2: Type: text/html, Size: 3139 bytes --]

Re: [Tarantool-patches] [PATCH luajit] FFI: Workaround for platform dlerror() returning NULL.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM!

Re: [Tarantool-patches] [PATCH luajit] FFI: Workaround for platform dlerror() returning NULL.

[Sergey Kaplun via Tarantool-patches]

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.2 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/10712
[2]: https://github.com/tarantool/tarantool/pull/10713
[3]: https://github.com/tarantool/tarantool/pull/10714

-- 
Best regards,
Sergey Kaplun