[Tarantool-patches] [PATCH luajit 0/2] Add UBSan support

[Tarantool-patches] [PATCH luajit 0/2] Add UBSan support

[Sergey Kaplun via Tarantool-patches]

This patchset enables UBSan support for LuaJIT.
The first patch adds the corresponding option for the CMake build, and
the second enables UBSan in the workflow for sanitizers.

Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-8473-ubsan
Issue: https://github.com/tarantool/tarantool/issues/8473

Sergey Kaplun (2):
  build: introduce LUAJIT_USE_UBSAN option
  ci: enable UBSan for sanitizers testing workflow

 .github/workflows/sanitizers-testing.yml |  8 +++--
 CMakeLists.txt                           | 45 ++++++++++++++++++++++++
 cmake/SetDynASMFlags.cmake               | 11 ++++++
 src/lj_carith.c                          |  5 +++
 src/lj_opt_fold.c                        |  5 +++
 src/lj_parse.c                           |  5 +++
 src/lj_snap.c                            |  7 ++++
 src/lj_strfmt.c                          |  5 +++
 8 files changed, 89 insertions(+), 2 deletions(-)

-- 
2.45.0

[Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Sergey Kaplun via Tarantool-patches]

This patch adds Undefined Behaviour Sanitizer [1] support. It enables
all checks except several that are not useful for LuaJIT. Also, it
instruments all known issues to be fixed in future patches (except
`kfold_intop()` since cdata arithmetic relies on integer overflow).

[1]: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html

Resolves tarantool/tarantool#8473
---
 CMakeLists.txt             | 45 ++++++++++++++++++++++++++++++++++++++
 cmake/SetDynASMFlags.cmake | 11 ++++++++++
 src/lj_carith.c            |  5 +++++
 src/lj_opt_fold.c          |  5 +++++
 src/lj_parse.c             |  5 +++++
 src/lj_snap.c              |  7 ++++++
 src/lj_strfmt.c            |  5 +++++
 7 files changed, 83 insertions(+)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2355ce17..edf2012f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -300,6 +300,51 @@ if(LUAJIT_USE_ASAN)
   )
 endif()
 
+option(LUAJIT_USE_UBSAN "Build LuaJIT with UndefinedBehaviorSanitizer" OFF)
+if(LUAJIT_USE_UBSAN)
+  # Use all recommendations from the UndefinedBehaviorSanitizer
+  # documentation:
+  # https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html.
+  string(JOIN "," UBSAN_IGNORE_OPTIONS
+    # Misaligned pseudo-pointers are used to determine internal
+    # variable names inside the `for` cycle.
+    alignment
+    # Not interested in float cast overflow errors.
+    float-cast-overflow
+    # NULL checking is disabled because this is not a UB and
+    # raises lots of false-positive fails.
+    null
+    # Not interested in checking arithmetic with NULL.
+    pointer-overflow
+    # Shifts of negative numbers are widely used in parsing ULEB,
+    # cdata arithmetic, vmevent hash calculation, etc.
+    shift-base
+  )
+  if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
+    string(JOIN "," UBSAN_IGNORE_OPTIONS
+      ${UBSAN_IGNORE_OPTIONS}
+      # Not interested in function type mismatch errors.
+      function
+    )
+  endif()
+  AppendFlags(CMAKE_C_FLAGS
+    # Enable hints for UndefinedBehaviorSanitizer.
+    -DLUAJIT_USE_UBSAN
+    # XXX: To get nicer stack traces in error messages.
+    -fno-omit-frame-pointer
+    # Enable UndefinedBehaviorSanitizer support.
+    # This flag enables all supported options (the documentation
+    # on cite is not correct about that moment, unfortunately)
+    # except float-divide-by-zero. Floating point division by zero
+    # behaviour is defined without -ffast-math and uses the
+    # IEEE 754 standard on which all NaN tagging is based.
+    -fsanitize=undefined
+    -fno-sanitize=${UBSAN_IGNORE_OPTIONS}
+    # Print a verbose error report and exit the program.
+    -fno-sanitize-recover=undefined
+  )
+endif()
+
 # Enable code coverage support.
 option(LUAJIT_ENABLE_COVERAGE "Enable code coverage support (gcovr)" OFF)
 if(LUAJIT_ENABLE_COVERAGE)
diff --git a/cmake/SetDynASMFlags.cmake b/cmake/SetDynASMFlags.cmake
index 7eead6e9..ae3c75b1 100644
--- a/cmake/SetDynASMFlags.cmake
+++ b/cmake/SetDynASMFlags.cmake
@@ -136,5 +136,16 @@ if(NOT CMAKE_SYSTEM_NAME STREQUAL ${CMAKE_HOST_SYSTEM_NAME})
   endif()
 endif()
 
+if(LUAJIT_USE_UBSAN)
+  # XXX: Skip checks for now to avoid build failures due to
+  # sanitizer errors.
+  # Need to backprot commits that fix the following issues first:
+  # https://github.com/LuaJIT/LuaJIT/pull/969,
+  # https://github.com/LuaJIT/LuaJIT/pull/970,
+  # https://github.com/LuaJIT/LuaJIT/issues/1041,
+  # https://github.com/LuaJIT/LuaJIT/pull/1044.
+  AppendFlags(HOST_C_FLAGS -fno-sanitize=undefined)
+endif()
+
 unset(LUAJIT_ARCH)
 unset(TESTARCH)
diff --git a/src/lj_carith.c b/src/lj_carith.c
index 4e1d450a..1d9d6fe1 100644
--- a/src/lj_carith.c
+++ b/src/lj_carith.c
@@ -159,6 +159,11 @@ static int carith_ptr(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
 }
 
 /* 64 bit integer arithmetic. */
+#if LUAJIT_USE_UBSAN
+/* See https://github.com/LuaJIT/LuaJIT/issues/928. */
+static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
+  __attribute__((no_sanitize("signed-integer-overflow")));
+#endif
 static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
 {
   if (ctype_isnum(ca->ct[0]->info) && ca->ct[0]->size <= 8 &&
diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
index 96780fa2..b9326c65 100644
--- a/src/lj_opt_fold.c
+++ b/src/lj_opt_fold.c
@@ -260,6 +260,11 @@ LJFOLDF(kfold_numcomp)
 
 /* -- Constant folding for 32 bit integers -------------------------------- */
 
+#if LUAJIT_USE_UBSAN
+/* Cdata arithmetic depends on the interger overflow. */
+static int32_t kfold_intop(int32_t k1, int32_t k2, IROp op)
+  __attribute__((no_sanitize("signed-integer-overflow")));
+#endif
 static int32_t kfold_intop(int32_t k1, int32_t k2, IROp op)
 {
   switch (op) {
diff --git a/src/lj_parse.c b/src/lj_parse.c
index 5a4ab7c8..acceed17 100644
--- a/src/lj_parse.c
+++ b/src/lj_parse.c
@@ -939,6 +939,11 @@ static void bcemit_binop(FuncState *fs, BinOpr op, ExpDesc *e1, ExpDesc *e2)
 }
 
 /* Emit unary operator. */
+#if LUAJIT_USE_UBSAN
+/* See https://github.com/LuaJIT/LuaJIT/issues/928. */
+static void bcemit_unop(FuncState *fs, BCOp op, ExpDesc *e)
+  __attribute__((no_sanitize("signed-integer-overflow")));
+#endif
 static void bcemit_unop(FuncState *fs, BCOp op, ExpDesc *e)
 {
   if (op == BC_NOT) {
diff --git a/src/lj_snap.c b/src/lj_snap.c
index 5a00b5cd..7dc4fe35 100644
--- a/src/lj_snap.c
+++ b/src/lj_snap.c
@@ -756,6 +756,13 @@ static void snap_restoreval(jit_State *J, GCtrace *T, ExitState *ex,
 }
 
 #if LJ_HASFFI
+# if LUAJIT_USE_UBSAN
+/* See https://github.com/LuaJIT/LuaJIT/issues/1193. */
+static void snap_restoredata(jit_State *J, GCtrace *T, ExitState *ex,
+			     SnapNo snapno, BloomFilter rfilt,
+			     IRRef ref, void *dst, CTSize sz)
+  __attribute__((no_sanitize("bounds")));
+# endif
 /* Restore raw data from the trace exit state. */
 static void snap_restoredata(jit_State *J, GCtrace *T, ExitState *ex,
 			     SnapNo snapno, BloomFilter rfilt,
diff --git a/src/lj_strfmt.c b/src/lj_strfmt.c
index ff5568c3..9592eff1 100644
--- a/src/lj_strfmt.c
+++ b/src/lj_strfmt.c
@@ -93,6 +93,11 @@ retlit:
   { uint32_t d = (x*(((1<<sh)+sc-1)/sc))>>sh; x -= d*sc; *p++ = (char)('0'+d); }
 
 /* Write integer to buffer. */
+#if LUAJIT_USE_UBSAN
+/* See https://github.com/LuaJIT/LuaJIT/issues/928. */
+char * LJ_FASTCALL lj_strfmt_wint(char *p, int32_t k)
+  __attribute__((no_sanitize("signed-integer-overflow")));
+#endif
 char * LJ_FASTCALL lj_strfmt_wint(char *p, int32_t k)
 {
   uint32_t u = (uint32_t)k;
-- 
2.45.0

[Tarantool-patches] [PATCH luajit 2/2] ci: enable UBSan for sanitizers testing workflow

[Sergey Kaplun via Tarantool-patches]

Relates to tarantool/tarantool#8473
---
 .github/workflows/sanitizers-testing.yml | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/sanitizers-testing.yml b/.github/workflows/sanitizers-testing.yml
index 154ebe40..4bf7d023 100644
--- a/.github/workflows/sanitizers-testing.yml
+++ b/.github/workflows/sanitizers-testing.yml
@@ -41,7 +41,7 @@ jobs:
             CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
     runs-on: [self-hosted, regular, Linux, x86_64]
     name: >
-      LuaJIT with ASan (Linux/x86_64)
+      LuaJIT with ASan and UBSan (Linux/x86_64)
       ${{ matrix.BUILDTYPE }}
       CC:${{ matrix.CC }}
       GC64:ON SYSMALLOC:ON
@@ -70,9 +70,10 @@ jobs:
           cmake -S . -B ${{ env.BUILDDIR }}
           -G Ninja
           ${{ matrix.CMAKEFLAGS }}
+          -DLUAJIT_ENABLE_GC64=ON
           -DLUAJIT_USE_ASAN=ON
           -DLUAJIT_USE_SYSMALLOC=ON
-          -DLUAJIT_ENABLE_GC64=ON
+          -DLUAJIT_USE_UBSAN=ON
       - name: build
         run: cmake --build . --parallel
         working-directory: ${{ env.BUILDDIR }}
@@ -91,5 +92,8 @@ jobs:
             symbolize=1:                     \
             unmap_shadow_on_exit=1:          \
           "
+          UBSAN_OPTIONS: "
+            print_stacktrace=1 \
+          "
         run: cmake --build . --parallel --target LuaJIT-test
         working-directory: ${{ env.BUILDDIR }}
-- 
2.45.0

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Sergey Kaplun via Tarantool-patches]

Hi, folks!
Some more thoughts below.

On 15.05.24, Sergey Kaplun wrote:

<snipped>

> +  string(JOIN "," UBSAN_IGNORE_OPTIONS
> +    # Misaligned pseudo-pointers are used to determine internal
> +    # variable names inside the `for` cycle.
> +    alignment
> +    # Not interested in float cast overflow errors.
> +    float-cast-overflow
> +    # NULL checking is disabled because this is not a UB and
> +    # raises lots of false-positive fails.
> +    null

Maybe it is worth to add also "nonnull-attribute" to the ignore options:

```
LSAN_OPTIONS="abort_on_error=1" src/luajit -e 'error("bad usage", 3)'
/home/burii/builds_workspace/luajit/gh-8473-ubsan/src/lj_buf.h:75:25: runtime error: null pointer passed as argument 1, which is declared to never be null
/usr/include/string.h:44:28: note: nonnull attribute specified here
```

Here, `memcpy()` gets the NULL pointer as the first argument and the
`len` == 0. So there are no problems here. Also, the nullability
violation is not a UB, as mentioned in the documentation.

Thoughts?

> +    # Not interested in checking arithmetic with NULL.
> +    pointer-overflow
> +    # Shifts of negative numbers are widely used in parsing ULEB,
> +    # cdata arithmetic, vmevent hash calculation, etc.
> +    shift-base
> +  )

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2] ci: enable UBSan for sanitizers testing workflow

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, except for the single nit below.

On Wed, May 15, 2024 at 03:32:01PM UTC, Sergey Kaplun wrote:
> Relates to tarantool/tarantool#8473
> ---
>  .github/workflows/sanitizers-testing.yml | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/.github/workflows/sanitizers-testing.yml b/.github/workflows/sanitizers-testing.yml
> index 154ebe40..4bf7d023 100644
> --- a/.github/workflows/sanitizers-testing.yml
> +++ b/.github/workflows/sanitizers-testing.yml
> @@ -41,7 +41,7 @@ jobs:
>              CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
>      runs-on: [self-hosted, regular, Linux, x86_64]
>      name: >
> -      LuaJIT with ASan (Linux/x86_64)
> +      LuaJIT with ASan and UBSan (Linux/x86_64)
>        ${{ matrix.BUILDTYPE }}
>        CC:${{ matrix.CC }}
>        GC64:ON SYSMALLOC:ON
> @@ -70,9 +70,10 @@ jobs:
>            cmake -S . -B ${{ env.BUILDDIR }}
>            -G Ninja
>            ${{ matrix.CMAKEFLAGS }}
> +          -DLUAJIT_ENABLE_GC64=ON
I see why this change was made, but it is irrelevant to the patch.
The decision to leave it as it is or remove is up to you.
>            -DLUAJIT_USE_ASAN=ON
>            -DLUAJIT_USE_SYSMALLOC=ON
> -          -DLUAJIT_ENABLE_GC64=ON
> +          -DLUAJIT_USE_UBSAN=ON
>        - name: build
>          run: cmake --build . --parallel
>          working-directory: ${{ env.BUILDDIR }}
> @@ -91,5 +92,8 @@ jobs:
>              symbolize=1:                     \
>              unmap_shadow_on_exit=1:          \
>            "
> +          UBSAN_OPTIONS: "
> +            print_stacktrace=1 \
> +          "
>          run: cmake --build . --parallel --target LuaJIT-test
>          working-directory: ${{ env.BUILDDIR }}
> --
> 2.45.0
>

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
See my thoughts below.

On Thu, May 16, 2024 at 01:14:14PM UTC, Sergey Kaplun wrote:
> Hi, folks!
> Some more thoughts below.
>
> On 15.05.24, Sergey Kaplun wrote:
>
> <snipped>
>
> > +  string(JOIN "," UBSAN_IGNORE_OPTIONS
> > +    # Misaligned pseudo-pointers are used to determine internal
> > +    # variable names inside the `for` cycle.
> > +    alignment
> > +    # Not interested in float cast overflow errors.
> > +    float-cast-overflow
> > +    # NULL checking is disabled because this is not a UB and
> > +    # raises lots of false-positive fails.
> > +    null
>
> Maybe it is worth to add also "nonnull-attribute" to the ignore options:
>
> ```
> LSAN_OPTIONS="abort_on_error=1" src/luajit -e 'error("bad usage", 3)'
> /home/burii/builds_workspace/luajit/gh-8473-ubsan/src/lj_buf.h:75:25: runtime error: null pointer passed as argument 1, which is declared to never be null
> /usr/include/string.h:44:28: note: nonnull attribute specified here
> ```
>
> Here, `memcpy()` gets the NULL pointer as the first argument and the
> `len` == 0. So there are no problems here. Also, the nullability
> violation is not a UB, as mentioned in the documentation.

It is UB, though: https://en.cppreference.com/w/cpp/string/byte/memcpy
Even with the zero len it may still cause issues, so I don't think we
should disable this check.

> Thoughts?
>
> > +    # Not interested in checking arithmetic with NULL.
> > +    pointer-overflow
> > +    # Shifts of negative numbers are widely used in parsing ULEB,
> > +    # cdata arithmetic, vmevent hash calculation, etc.
> > +    shift-base
> > +  )
>
> --
> Best regards,
> Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!

On 26.05.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> See my thoughts below.
> 
> On Thu, May 16, 2024 at 01:14:14PM UTC, Sergey Kaplun wrote:
> > Hi, folks!
> > Some more thoughts below.
> >
> > On 15.05.24, Sergey Kaplun wrote:
> >
> > <snipped>
> >
> > > +  string(JOIN "," UBSAN_IGNORE_OPTIONS
> > > +    # Misaligned pseudo-pointers are used to determine internal
> > > +    # variable names inside the `for` cycle.
> > > +    alignment
> > > +    # Not interested in float cast overflow errors.
> > > +    float-cast-overflow
> > > +    # NULL checking is disabled because this is not a UB and
> > > +    # raises lots of false-positive fails.
> > > +    null
> >
> > Maybe it is worth to add also "nonnull-attribute" to the ignore options:
> >
> > ```
> > LSAN_OPTIONS="abort_on_error=1" src/luajit -e 'error("bad usage", 3)'
> > /home/burii/builds_workspace/luajit/gh-8473-ubsan/src/lj_buf.h:75:25: runtime error: null pointer passed as argument 1, which is declared to never be null
> > /usr/include/string.h:44:28: note: nonnull attribute specified here
> > ```
> >
> > Here, `memcpy()` gets the NULL pointer as the first argument and the
> > `len` == 0. So there are no problems here. Also, the nullability
> > violation is not a UB, as mentioned in the documentation.
> 
> It is UB, though: https://en.cppreference.com/w/cpp/string/byte/memcpy
> Even with the zero len it may still cause issues, so I don't think we
> should disable this check.

But there are no such words in the `memcpy` man page. The only one
mentioned UB is about overlapping memory chunks. Also, I suppose that
the first point applies only to the case, when the bytes are actually
copied (i.e., when size is not zero).

> 
> > Thoughts?
> >
> > > +    # Not interested in checking arithmetic with NULL.
> > > +    pointer-overflow
> > > +    # Shifts of negative numbers are widely used in parsing ULEB,
> > > +    # cdata arithmetic, vmevent hash calculation, etc.
> > > +    shift-base
> > > +  )
> >
> > --
> > Best regards,
> > Sergey Kaplun

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
See my answer below.

On Mon, May 27, 2024 at 10:22:08AM UTC, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the review!
>
> On 26.05.24, Maxim Kokryashkin wrote:
> > Hi, Sergey!
> > See my thoughts below.
> >
> > On Thu, May 16, 2024 at 01:14:14PM UTC, Sergey Kaplun wrote:
> > > Hi, folks!
> > > Some more thoughts below.
> > >
> > > On 15.05.24, Sergey Kaplun wrote:
> > >
> > > <snipped>
> > >
> > > > +  string(JOIN "," UBSAN_IGNORE_OPTIONS
> > > > +    # Misaligned pseudo-pointers are used to determine internal
> > > > +    # variable names inside the `for` cycle.
> > > > +    alignment
> > > > +    # Not interested in float cast overflow errors.
> > > > +    float-cast-overflow
> > > > +    # NULL checking is disabled because this is not a UB and
> > > > +    # raises lots of false-positive fails.
> > > > +    null
> > >
> > > Maybe it is worth to add also "nonnull-attribute" to the ignore options:
> > >
> > > ```
> > > LSAN_OPTIONS="abort_on_error=1" src/luajit -e 'error("bad usage", 3)'
> > > /home/burii/builds_workspace/luajit/gh-8473-ubsan/src/lj_buf.h:75:25: runtime error: null pointer passed as argument 1, which is declared to never be null
> > > /usr/include/string.h:44:28: note: nonnull attribute specified here
> > > ```
> > >
> > > Here, `memcpy()` gets the NULL pointer as the first argument and the
> > > `len` == 0. So there are no problems here. Also, the nullability
> > > violation is not a UB, as mentioned in the documentation.
> >
> > It is UB, though: https://en.cppreference.com/w/cpp/string/byte/memcpy
> > Even with the zero len it may still cause issues, so I don't think we
> > should disable this check.
>
> But there are no such words in the `memcpy` man page. The only one
> mentioned UB is about overlapping memory chunks. Also, I suppose that
> the first point applies only to the case, when the bytes are actually
> copied (i.e., when size is not zero).

Here is the standard: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf
And it states clearly:

| 7.24.1 String function conventions
...
| 2. Where an argument declared as size_t n specifies the length of the array for a function, n can have
| the value zero on a call to that function. Unless explicitly stated otherwise in the description of a
| particular function in this subclause, pointer arguments on such a call shall still have valid values, as
| described in 7.1.4. On such a call, a function that locates a character finds no occurrence, a function
| that compares two character sequences returns zero, and a function that copies characters copies
| zero characters.

The 7.1.4.1 then states:
| Each of the following statements applies unless explicitly stated otherwise in the detailed descriptions that follow:
| — If an argument to a function has an invalid value (such as a value outside the domain of the
| function, or a pointer outside the address space of the program, or a null pointer, or a pointer
| to non-modifiable storage when the corresponding parameter is not const-qualified) or a type
| (after default argument promotion) not expected by a function with a variable number of
| arguments, the behavior is undefined.

So it is UB after all.

Side note: if a function is able to accept a NULL-pointer, then the man
page usually has its signature written like this:
| int accept(int sockfd, struct sockaddr *_Nullable restrict addr,
|                  socklen_t *_Nullable restrict addrlen);

`_Nullable` before a parameter name here means that it can be a
NULL-pointer safely.

>
> >
> > > Thoughts?
> > >
> > > > +    # Not interested in checking arithmetic with NULL.
> > > > +    pointer-overflow
> > > > +    # Shifts of negative numbers are widely used in parsing ULEB,
> > > > +    # cdata arithmetic, vmevent hash calculation, etc.
> > > > +    shift-base
> > > > +  )
> > >
> > > --
> > > Best regards,
> > > Sergey Kaplun
>
> --
> Best regards,
> Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On Wed, May 15, 2024 at 03:32:00PM UTC, Sergey Kaplun wrote:
> This patch adds Undefined Behaviour Sanitizer [1] support. It enables
> all checks except several that are not useful for LuaJIT. Also, it
> instruments all known issues to be fixed in future patches (except
> `kfold_intop()` since cdata arithmetic relies on integer overflow).
>
> [1]: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
>
> Resolves tarantool/tarantool#8473
> ---
>  CMakeLists.txt             | 45 ++++++++++++++++++++++++++++++++++++++
>  cmake/SetDynASMFlags.cmake | 11 ++++++++++
>  src/lj_carith.c            |  5 +++++
>  src/lj_opt_fold.c          |  5 +++++
>  src/lj_parse.c             |  5 +++++
>  src/lj_snap.c              |  7 ++++++
>  src/lj_strfmt.c            |  5 +++++
>  7 files changed, 83 insertions(+)
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 2355ce17..edf2012f 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -300,6 +300,51 @@ if(LUAJIT_USE_ASAN)
>    )
>  endif()
>
> +option(LUAJIT_USE_UBSAN "Build LuaJIT with UndefinedBehaviorSanitizer" OFF)
> +if(LUAJIT_USE_UBSAN)
> +  # Use all recommendations from the UndefinedBehaviorSanitizer
> +  # documentation:
> +  # https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html.
> +  string(JOIN "," UBSAN_IGNORE_OPTIONS
> +    # Misaligned pseudo-pointers are used to determine internal
> +    # variable names inside the `for` cycle.
> +    alignment
> +    # Not interested in float cast overflow errors.
> +    float-cast-overflow
Why we are not interested in them though?
> +    # NULL checking is disabled because this is not a UB and
> +    # raises lots of false-positive fails.
> +    null
> +    # Not interested in checking arithmetic with NULL.
> +    pointer-overflow
> +    # Shifts of negative numbers are widely used in parsing ULEB,
> +    # cdata arithmetic, vmevent hash calculation, etc.
> +    shift-base
> +  )
> +  if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
> +    string(JOIN "," UBSAN_IGNORE_OPTIONS
> +      ${UBSAN_IGNORE_OPTIONS}
> +      # Not interested in function type mismatch errors.
> +      function
> +    )
> +  endif()
Please drop a comment explaining why this additional configuration
is needed.
> +  AppendFlags(CMAKE_C_FLAGS
> +    # Enable hints for UndefinedBehaviorSanitizer.
> +    -DLUAJIT_USE_UBSAN
> +    # XXX: To get nicer stack traces in error messages.
> +    -fno-omit-frame-pointer
> +    # Enable UndefinedBehaviorSanitizer support.
> +    # This flag enables all supported options (the documentation
> +    # on cite is not correct about that moment, unfortunately)
> +    # except float-divide-by-zero. Floating point division by zero
> +    # behaviour is defined without -ffast-math and uses the
> +    # IEEE 754 standard on which all NaN tagging is based.
> +    -fsanitize=undefined
> +    -fno-sanitize=${UBSAN_IGNORE_OPTIONS}
> +    # Print a verbose error report and exit the program.
> +    -fno-sanitize-recover=undefined
> +  )
> +endif()

The whole chunk above is a bit too large to include into the root
CMakeLists.txt, so I propose to move it into a separate CMake module.
> +
<snipped>

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Please consider my answers below.

On 27.05.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> 
> On Wed, May 15, 2024 at 03:32:00PM UTC, Sergey Kaplun wrote:
> > This patch adds Undefined Behaviour Sanitizer [1] support. It enables
> > all checks except several that are not useful for LuaJIT. Also, it
> > instruments all known issues to be fixed in future patches (except
> > `kfold_intop()` since cdata arithmetic relies on integer overflow).
> >
> > [1]: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
> >
> > Resolves tarantool/tarantool#8473
> > ---
> >  CMakeLists.txt             | 45 ++++++++++++++++++++++++++++++++++++++
> >  cmake/SetDynASMFlags.cmake | 11 ++++++++++
> >  src/lj_carith.c            |  5 +++++
> >  src/lj_opt_fold.c          |  5 +++++
> >  src/lj_parse.c             |  5 +++++
> >  src/lj_snap.c              |  7 ++++++
> >  src/lj_strfmt.c            |  5 +++++
> >  7 files changed, 83 insertions(+)
> >
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index 2355ce17..edf2012f 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -300,6 +300,51 @@ if(LUAJIT_USE_ASAN)
> >    )
> >  endif()
> >
> > +option(LUAJIT_USE_UBSAN "Build LuaJIT with UndefinedBehaviorSanitizer" OFF)
> > +if(LUAJIT_USE_UBSAN)
> > +  # Use all recommendations from the UndefinedBehaviorSanitizer
> > +  # documentation:
> > +  # https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html.
> > +  string(JOIN "," UBSAN_IGNORE_OPTIONS
> > +    # Misaligned pseudo-pointers are used to determine internal
> > +    # variable names inside the `for` cycle.
> > +    alignment
> > +    # Not interested in float cast overflow errors.
> > +    float-cast-overflow
> Why we are not interested in them though?

Most warnings are referenced by `lj_num2int()`, which just converts the `double` to `int`. All such checks are covered by the corresponding check that the resulted value of the `int` cast back to `double` remains the same number.

I've added the following comment:
===================================================================
diff --git a/CMakeLists.txt b/CMakeLists.txt
index edf2012f..5eeea783 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -309,7 +309,9 @@ if(LUAJIT_USE_UBSAN)
     # Misaligned pseudo-pointers are used to determine internal
     # variable names inside the `for` cycle.
     alignment
-    # Not interested in float cast overflow errors.
+    # Not interested in float cast overflow errors. These
+    # overflows are handled by special checks after
+    # `lj_num2int()`, etc.
     float-cast-overflow
     # NULL checking is disabled because this is not a UB and
     # raises lots of false-positive fails.
===================================================================

> > +    # NULL checking is disabled because this is not a UB and
> > +    # raises lots of false-positive fails.
> > +    null
> > +    # Not interested in checking arithmetic with NULL.
> > +    pointer-overflow
> > +    # Shifts of negative numbers are widely used in parsing ULEB,
> > +    # cdata arithmetic, vmevent hash calculation, etc.
> > +    shift-base
> > +  )
> > +  if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
> > +    string(JOIN "," UBSAN_IGNORE_OPTIONS
> > +      ${UBSAN_IGNORE_OPTIONS}
> > +      # Not interested in function type mismatch errors.
> > +      function
> > +    )
> > +  endif()
> Please drop a comment explaining why this additional configuration
> is needed.

Added.
===================================================================
diff --git a/CMakeLists.txt b/CMakeLists.txt
index edf2012f..5eeea783 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -320,6 +322,7 @@ if(LUAJIT_USE_UBSAN)
     # cdata arithmetic, vmevent hash calculation, etc.
     shift-base
   )
+  # GCC has no "function" UB check.
   if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
     string(JOIN "," UBSAN_IGNORE_OPTIONS
       ${UBSAN_IGNORE_OPTIONS}
===================================================================

> > +  AppendFlags(CMAKE_C_FLAGS
> > +    # Enable hints for UndefinedBehaviorSanitizer.
> > +    -DLUAJIT_USE_UBSAN
> > +    # XXX: To get nicer stack traces in error messages.
> > +    -fno-omit-frame-pointer
> > +    # Enable UndefinedBehaviorSanitizer support.
> > +    # This flag enables all supported options (the documentation
> > +    # on cite is not correct about that moment, unfortunately)
> > +    # except float-divide-by-zero. Floating point division by zero
> > +    # behaviour is defined without -ffast-math and uses the
> > +    # IEEE 754 standard on which all NaN tagging is based.
> > +    -fsanitize=undefined
> > +    -fno-sanitize=${UBSAN_IGNORE_OPTIONS}
> > +    # Print a verbose error report and exit the program.
> > +    -fno-sanitize-recover=undefined
> > +  )
> > +endif()
> 
> The whole chunk above is a bit too large to include into the root
> CMakeLists.txt, so I propose to move it into a separate CMake module.

It has a size similar to the ASAN blob (44 lines, most of which are
comments, against 25 lines), so ignoring.

> > +
> <snipped>

Also, added the additional instrumentation for `lj_buf_wmem()` as a
result of this discussion [1]:

===================================================================
diff --git a/src/lj_buf.h b/src/lj_buf.h
index a4051694..de7bee06 100644
--- a/src/lj_buf.h
+++ b/src/lj_buf.h
@@ -70,6 +70,13 @@ LJ_FUNC SBuf *lj_buf_putmem(SBuf *sb, const void *q, MSize len);
 LJ_FUNC SBuf * LJ_FASTCALL lj_buf_putchar(SBuf *sb, int c);
 LJ_FUNC SBuf * LJ_FASTCALL lj_buf_putstr(SBuf *sb, GCstr *s);
 
+#if LUAJIT_USE_UBSAN
+/* The `NULL` argument with the zero length, like in the case:
+** | luajit -e 'error("x", 3)'
+*/
+static LJ_AINLINE char *lj_buf_wmem(char *p, const void *q, MSize len)
+  __attribute__((no_sanitize("nonnull-attribute")));
+#endif
 static LJ_AINLINE char *lj_buf_wmem(char *p, const void *q, MSize len)
 {
   return (char *)memcpy(p, q, len) + len;
===================================================================

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029192.html

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2] ci: enable UBSan for sanitizers testing workflow

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
See my answers below!

On 26.05.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for the single nit below.
> 
> On Wed, May 15, 2024 at 03:32:01PM UTC, Sergey Kaplun wrote:
> > Relates to tarantool/tarantool#8473
> > ---

<snipped>

> > @@ -70,9 +70,10 @@ jobs:
> >            cmake -S . -B ${{ env.BUILDDIR }}
> >            -G Ninja
> >            ${{ matrix.CMAKEFLAGS }}
> > +          -DLUAJIT_ENABLE_GC64=ON
> I see why this change was made, but it is irrelevant to the patch.
> The decision to leave it as it is or remove is up to you.

Added the following paragraph to the commit message to avoid confusion:

| Also, this patch sorts the corresponding flags in the CI workflow
| alphabetically for better readability.

> >            -DLUAJIT_USE_ASAN=ON

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Sergey Bronnikov via Tarantool-patches]

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

Sergey,

thanks for the patch! Please see my comments below.

On 15.05.2024 15:32, Sergey Kaplun wrote:
> This patch adds Undefined Behaviour Sanitizer [1] support. It enables
> all checks except several that are not useful for LuaJIT. Also, it
> instruments all known issues to be fixed in future patches (except
> `kfold_intop()` since cdata arithmetic relies on integer overflow).
>
> [1]:https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
>
> Resolves tarantool/tarantool#8473
> ---
>   CMakeLists.txt             | 45 ++++++++++++++++++++++++++++++++++++++
>   cmake/SetDynASMFlags.cmake | 11 ++++++++++
>   src/lj_carith.c            |  5 +++++
>   src/lj_opt_fold.c          |  5 +++++
>   src/lj_parse.c             |  5 +++++
>   src/lj_snap.c              |  7 ++++++
>   src/lj_strfmt.c            |  5 +++++
>   7 files changed, 83 insertions(+)

patch in mail is outdated, so I'll copypaste missed part:


diff --git a/src/lj_buf.h b/src/lj_buf.h
index a4051694..aaecc9f8 100644
--- a/src/lj_buf.h
+++ b/src/lj_buf.h
@@ -70,6 +70,13 @@ LJ_FUNC SBuf *lj_buf_putmem(SBuf *sb, const void *q, 
MSize len);
  LJ_FUNC SBuf * LJ_FASTCALL lj_buf_putchar(SBuf *sb, int c);
  LJ_FUNC SBuf * LJ_FASTCALL lj_buf_putstr(SBuf *sb, GCstr *s);

+#if LUAJIT_USE_UBSAN
+/* The `NULL` argument with the zero length, like in the case:
+** | luajit -e 'error("x", 3)'
+*/
+static LJ_AINLINE char *lj_buf_wmem(char *p, const void *q, MSize len)
+  __attribute__((no_sanitize("nonnull-attribute")));
+#endif
  static LJ_AINLINE char *lj_buf_wmem(char *p, const void *q, MSize len)
  {
    return (char *)memcpy(p, q, len) + len;


With this reverted patch tests passed. Do we really need this patch?


>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 2355ce17..edf2012f 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -300,6 +300,51 @@ if(LUAJIT_USE_ASAN)
>     )
>   endif()
>   
> +option(LUAJIT_USE_UBSAN "Build LuaJIT with UndefinedBehaviorSanitizer" OFF)
> +if(LUAJIT_USE_UBSAN)
> +  # Use all recommendations from the UndefinedBehaviorSanitizer

probably you mean "checks" [1] and not "recommendations"


1. https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks

> +  # documentation:
> +  #https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html.
> +  string(JOIN "," UBSAN_IGNORE_OPTIONS
> +    # Misaligned pseudo-pointers are used to determine internal
> +    # variable names inside the `for` cycle.
> +    alignment
> +    # Not interested in float cast overflow errors.
> +    float-cast-overflow
> +    # NULL checking is disabled because this is not a UB and
> +    # raises lots of false-positive fails.
> +    null
> +    # Not interested in checking arithmetic with NULL.
> +    pointer-overflow
> +    # Shifts of negative numbers are widely used in parsing ULEB,
> +    # cdata arithmetic, vmevent hash calculation, etc.
> +    shift-base

Will we report issues produced by these checks to upstream?

Decision "not interested" confuses.

> +  )
> +  if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")

please add a link to GCC documentation

https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003dundefined

> +    string(JOIN "," UBSAN_IGNORE_OPTIONS
> +      ${UBSAN_IGNORE_OPTIONS}
> +      # Not interested in function type mismatch errors.
> +      function
> +    )
> +  endif()
> +  AppendFlags(CMAKE_C_FLAGS
> +    # Enable hints for UndefinedBehaviorSanitizer.
> +    -DLUAJIT_USE_UBSAN
> +    # XXX: To get nicer stack traces in error messages.
> +    -fno-omit-frame-pointer
> +    # Enable UndefinedBehaviorSanitizer support.
> +    # This flag enables all supported options (the documentation
> +    # on cite is not correct about that moment, unfortunately)
typo: cite -> site
> +    # except float-divide-by-zero. Floating point division by zero
> +    # behaviour is defined without -ffast-math and uses the
> +    # IEEE 754 standard on which all NaN tagging is based.
> +    -fsanitize=undefined
> +    -fno-sanitize=${UBSAN_IGNORE_OPTIONS}
> +    # Print a verbose error report and exit the program.
> +    -fno-sanitize-recover=undefined
> +  )
> +endif()
> +
>   # Enable code coverage support.
>   option(LUAJIT_ENABLE_COVERAGE "Enable code coverage support (gcovr)" OFF)
>   if(LUAJIT_ENABLE_COVERAGE)
> diff --git a/cmake/SetDynASMFlags.cmake b/cmake/SetDynASMFlags.cmake
> index 7eead6e9..ae3c75b1 100644
> --- a/cmake/SetDynASMFlags.cmake
> +++ b/cmake/SetDynASMFlags.cmake
> @@ -136,5 +136,16 @@ if(NOT CMAKE_SYSTEM_NAME STREQUAL ${CMAKE_HOST_SYSTEM_NAME})
>     endif()
>   endif()
>   
> +if(LUAJIT_USE_UBSAN)
> +  # XXX: Skip checks for now to avoid build failures due to
> +  # sanitizer errors.
> +  # Need to backprot commits that fix the following issues first:
typo: backprot -> backport
> +  #https://github.com/LuaJIT/LuaJIT/pull/969,
> +  #https://github.com/LuaJIT/LuaJIT/pull/970,
> +  #https://github.com/LuaJIT/LuaJIT/issues/1041,
> +  #https://github.com/LuaJIT/LuaJIT/pull/1044.
> +  AppendFlags(HOST_C_FLAGS -fno-sanitize=undefined)
> +endif()
> +
>   unset(LUAJIT_ARCH)
>   unset(TESTARCH)
> diff --git a/src/lj_carith.c b/src/lj_carith.c
With this reverted patch tests passed. Do we really need this patch?
> index 4e1d450a..1d9d6fe1 100644
> --- a/src/lj_carith.c
> +++ b/src/lj_carith.c
> @@ -159,6 +159,11 @@ static int carith_ptr(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
>   }
>   
>   /* 64 bit integer arithmetic. */
> +#if LUAJIT_USE_UBSAN
> +/* Seehttps://github.com/LuaJIT/LuaJIT/issues/928. */
> +static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
> +  __attribute__((no_sanitize("signed-integer-overflow")));
> +#endif
>   static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
>   {
>     if (ctype_isnum(ca->ct[0]->info) && ca->ct[0]->size <= 8 &&
> diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> index 96780fa2..b9326c65 100644
> --- a/src/lj_opt_fold.c
> +++ b/src/lj_opt_fold.c
> @@ -260,6 +260,11 @@ LJFOLDF(kfold_numcomp)
>   
>   /* -- Constant folding for 32 bit integers -------------------------------- */
>   
> +#if LUAJIT_USE_UBSAN
> +/* Cdata arithmetic depends on the interger overflow. */
> +static int32_t kfold_intop(int32_t k1, int32_t k2, IROp op)
> +  __attribute__((no_sanitize("signed-integer-overflow")));
> +#endif
>   static int32_t kfold_intop(int32_t k1, int32_t k2, IROp op)
>   {
>     switch (op) {
> diff --git a/src/lj_parse.c b/src/lj_parse.c
> index 5a4ab7c8..acceed17 100644
> --- a/src/lj_parse.c
> +++ b/src/lj_parse.c
> @@ -939,6 +939,11 @@ static void bcemit_binop(FuncState *fs, BinOpr op, ExpDesc *e1, ExpDesc *e2)
>   }
>   
>   /* Emit unary operator. */
> +#if LUAJIT_USE_UBSAN
> +/* Seehttps://github.com/LuaJIT/LuaJIT/issues/928. */
> +static void bcemit_unop(FuncState *fs, BCOp op, ExpDesc *e)
> +  __attribute__((no_sanitize("signed-integer-overflow")));
> +#endif
>   static void bcemit_unop(FuncState *fs, BCOp op, ExpDesc *e)
>   {
>     if (op == BC_NOT) {
> diff --git a/src/lj_snap.c b/src/lj_snap.c
> index 5a00b5cd..7dc4fe35 100644
> --- a/src/lj_snap.c
> +++ b/src/lj_snap.c
> @@ -756,6 +756,13 @@ static void snap_restoreval(jit_State *J, GCtrace *T, ExitState *ex,
>   }
>   
>   #if LJ_HASFFI
> +# if LUAJIT_USE_UBSAN
> +/* Seehttps://github.com/LuaJIT/LuaJIT/issues/1193. */
> +static void snap_restoredata(jit_State *J, GCtrace *T, ExitState *ex,
> +			     SnapNo snapno, BloomFilter rfilt,
> +			     IRRef ref, void *dst, CTSize sz)
> +  __attribute__((no_sanitize("bounds")));
> +# endif
>   /* Restore raw data from the trace exit state. */
>   static void snap_restoredata(jit_State *J, GCtrace *T, ExitState *ex,
>   			     SnapNo snapno, BloomFilter rfilt,
> diff --git a/src/lj_strfmt.c b/src/lj_strfmt.c
> index ff5568c3..9592eff1 100644
> --- a/src/lj_strfmt.c
> +++ b/src/lj_strfmt.c
> @@ -93,6 +93,11 @@ retlit:
>     { uint32_t d = (x*(((1<<sh)+sc-1)/sc))>>sh; x -= d*sc; *p++ = (char)('0'+d); }
>   
>   /* Write integer to buffer. */
> +#if LUAJIT_USE_UBSAN
> +/* Seehttps://github.com/LuaJIT/LuaJIT/issues/928. */
> +char * LJ_FASTCALL lj_strfmt_wint(char *p, int32_t k)
> +  __attribute__((no_sanitize("signed-integer-overflow")));
> +#endif
>   char * LJ_FASTCALL lj_strfmt_wint(char *p, int32_t k)
>   {
>     uint32_t u = (uint32_t)k;

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

Re: [Tarantool-patches] [PATCH luajit 2/2] ci: enable UBSan for sanitizers testing workflow

[Sergey Bronnikov via Tarantool-patches]

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

Sergey,


thanks for the patch! See my comments below.

Sergey

On 15.05.2024 15:32, Sergey Kaplun wrote:
> Relates to tarantool/tarantool#8473
> ---
>   .github/workflows/sanitizers-testing.yml | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/.github/workflows/sanitizers-testing.yml b/.github/workflows/sanitizers-testing.yml
> index 154ebe40..4bf7d023 100644
> --- a/.github/workflows/sanitizers-testing.yml
> +++ b/.github/workflows/sanitizers-testing.yml
> @@ -41,7 +41,7 @@ jobs:
>               CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
>       runs-on: [self-hosted, regular, Linux, x86_64]
>       name: >
> -      LuaJIT with ASan (Linux/x86_64)
> +      LuaJIT with ASan and UBSan (Linux/x86_64)
>         ${{ matrix.BUILDTYPE }}
>         CC:${{ matrix.CC }}
>         GC64:ON SYSMALLOC:ON
> @@ -70,9 +70,10 @@ jobs:
>             cmake -S . -B ${{ env.BUILDDIR }}
>             -G Ninja
>             ${{ matrix.CMAKEFLAGS }}
> +          -DLUAJIT_ENABLE_GC64=ON
>             -DLUAJIT_USE_ASAN=ON
>             -DLUAJIT_USE_SYSMALLOC=ON
> -          -DLUAJIT_ENABLE_GC64=ON
> +          -DLUAJIT_USE_UBSAN=ON
>         - name: build
>           run: cmake --build . --parallel
>           working-directory: ${{ env.BUILDDIR }}
> @@ -91,5 +92,8 @@ jobs:
>               symbolize=1:                     \
>               unmap_shadow_on_exit=1:          \
>             "
> +          UBSAN_OPTIONS: "
> +            print_stacktrace=1 \
I propose to move these env variables to CMake.
> +          "
>           run: cmake --build . --parallel --target LuaJIT-test
>           working-directory: ${{ env.BUILDDIR }}

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

Re: [Tarantool-patches] [PATCH luajit 2/2] ci: enable UBSan for sanitizers testing workflow

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Please consider my answers below.

On 07.06.24, Sergey Bronnikov wrote:
> Sergey,
> 
> 
> thanks for the patch! See my comments below.
> 
> Sergey
> 
> On 15.05.2024 15:32, Sergey Kaplun wrote:
> > Relates to tarantool/tarantool#8473
> > ---
> >   .github/workflows/sanitizers-testing.yml | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/.github/workflows/sanitizers-testing.yml b/.github/workflows/sanitizers-testing.yml
> > index 154ebe40..4bf7d023 100644
> > --- a/.github/workflows/sanitizers-testing.yml
> > +++ b/.github/workflows/sanitizers-testing.yml

<snipped>

> > @@ -91,5 +92,8 @@ jobs:
> >               symbolize=1:                     \
> >               unmap_shadow_on_exit=1:          \
> >             "
> > +          UBSAN_OPTIONS: "
> > +            print_stacktrace=1 \
> I propose to move these env variables to CMake.

I suppose this is the same reason as for ASAN options: when run the asan
+ lsan CI in the tarantool repository, it configures its own ASAN +
UBSAN options. To avoid their overwriting in our CMake file, we don't
include this part there.

> > +          "
> >           run: cmake --build . --parallel --target LuaJIT-test
> >           working-directory: ${{ env.BUILDDIR }}

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Please considered my answers below.

On 07.06.24, Sergey Bronnikov wrote:
> Sergey,
> 
> thanks for the patch! Please see my comments below.

Fixed your comments, see the iterative patch below.
The branch is force-pushed.

===================================================================
diff --git a/CMakeLists.txt b/CMakeLists.txt
index e00b1536..76e0c067 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -302,7 +302,7 @@ endif()
 
 option(LUAJIT_USE_UBSAN "Build LuaJIT with UndefinedBehaviorSanitizer" OFF)
 if(LUAJIT_USE_UBSAN)
-  # Use all recommendations from the UndefinedBehaviorSanitizer
+  # Use all needed checks from the UndefinedBehaviorSanitizer
   # documentation:
   # https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html.
   string(JOIN "," UBSAN_IGNORE_OPTIONS
@@ -322,7 +322,8 @@ if(LUAJIT_USE_UBSAN)
     # cdata arithmetic, vmevent hash calculation, etc.
     shift-base
   )
-  # GCC has no "function" UB check.
+  # GCC has no "function" UB check. See details here:
+  # https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003dundefined
   if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
     string(JOIN "," UBSAN_IGNORE_OPTIONS
       ${UBSAN_IGNORE_OPTIONS}
@@ -337,7 +338,7 @@ if(LUAJIT_USE_UBSAN)
     -fno-omit-frame-pointer
     # Enable UndefinedBehaviorSanitizer support.
     # This flag enables all supported options (the documentation
-    # on cite is not correct about that moment, unfortunately)
+    # on site is not correct about that moment, unfortunately)
     # except float-divide-by-zero. Floating point division by zero
     # behaviour is defined without -ffast-math and uses the
     # IEEE 754 standard on which all NaN tagging is based.
diff --git a/cmake/SetDynASMFlags.cmake b/cmake/SetDynASMFlags.cmake
index ae3c75b1..b400cf57 100644
--- a/cmake/SetDynASMFlags.cmake
+++ b/cmake/SetDynASMFlags.cmake
@@ -139,7 +139,7 @@ endif()
 if(LUAJIT_USE_UBSAN)
   # XXX: Skip checks for now to avoid build failures due to
   # sanitizer errors.
-  # Need to backprot commits that fix the following issues first:
+  # Need to backport commits that fix the following issues first:
   # https://github.com/LuaJIT/LuaJIT/pull/969,
   # https://github.com/LuaJIT/LuaJIT/pull/970,
   # https://github.com/LuaJIT/LuaJIT/issues/1041,
===================================================================

> 
> On 15.05.2024 15:32, Sergey Kaplun wrote:
> > This patch adds Undefined Behaviour Sanitizer [1] support. It enables
> > all checks except several that are not useful for LuaJIT. Also, it
> > instruments all known issues to be fixed in future patches (except
> > `kfold_intop()` since cdata arithmetic relies on integer overflow).
> >
> > [1]:https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
> >
> > Resolves tarantool/tarantool#8473
> > ---
> >   CMakeLists.txt             | 45 ++++++++++++++++++++++++++++++++++++++
> >   cmake/SetDynASMFlags.cmake | 11 ++++++++++
> >   src/lj_carith.c            |  5 +++++
> >   src/lj_opt_fold.c          |  5 +++++
> >   src/lj_parse.c             |  5 +++++
> >   src/lj_snap.c              |  7 ++++++
> >   src/lj_strfmt.c            |  5 +++++
> >   7 files changed, 83 insertions(+)
> 
> patch in mail is outdated, so I'll copypaste missed part:

Yes, it is mentioned in this subthread [1].

> 
> 
> diff --git a/src/lj_buf.h b/src/lj_buf.h
> index a4051694..aaecc9f8 100644
> --- a/src/lj_buf.h
> +++ b/src/lj_buf.h
> @@ -70,6 +70,13 @@ LJ_FUNC SBuf *lj_buf_putmem(SBuf *sb, const void *q, 
> MSize len);
>   LJ_FUNC SBuf * LJ_FASTCALL lj_buf_putchar(SBuf *sb, int c);
>   LJ_FUNC SBuf * LJ_FASTCALL lj_buf_putstr(SBuf *sb, GCstr *s);
> 
> +#if LUAJIT_USE_UBSAN
> +/* The `NULL` argument with the zero length, like in the case:
> +** | luajit -e 'error("x", 3)'
> +*/
> +static LJ_AINLINE char *lj_buf_wmem(char *p, const void *q, MSize len)
> +  __attribute__((no_sanitize("nonnull-attribute")));
> +#endif
>   static LJ_AINLINE char *lj_buf_wmem(char *p, const void *q, MSize len)
>   {
>     return (char *)memcpy(p, q, len) + len;
> 
> 
> With this reverted patch tests passed. Do we really need this patch?

Should I add the corresponding test mentioned in [1]?

> 
> 
> >
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index 2355ce17..edf2012f 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -300,6 +300,51 @@ if(LUAJIT_USE_ASAN)
> >     )
> >   endif()
> >   
> > +option(LUAJIT_USE_UBSAN "Build LuaJIT with UndefinedBehaviorSanitizer" OFF)
> > +if(LUAJIT_USE_UBSAN)
> > +  # Use all recommendations from the UndefinedBehaviorSanitizer
> 
> probably you mean "checks" [1] and not "recommendations"

Fixed, thanks.

> 
> 
> 1. https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks
> 
> > +  # documentation:
> > +  #https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html.
> > +  string(JOIN "," UBSAN_IGNORE_OPTIONS
> > +    # Misaligned pseudo-pointers are used to determine internal
> > +    # variable names inside the `for` cycle.
> > +    alignment
> > +    # Not interested in float cast overflow errors.
> > +    float-cast-overflow
> > +    # NULL checking is disabled because this is not a UB and
> > +    # raises lots of false-positive fails.
> > +    null
> > +    # Not interested in checking arithmetic with NULL.
> > +    pointer-overflow
> > +    # Shifts of negative numbers are widely used in parsing ULEB,
> > +    # cdata arithmetic, vmevent hash calculation, etc.
> > +    shift-base
> 
> Will we report issues produced by these checks to upstream?

These particular checks -- no, since they are not so interesting for us,
and most probably may be considered by Mike as "white noise".

For others -- yes.
I've already reported the related problem with the patch [3].

> 
> Decision "not interested" confuses.

I've given the rationale for float-cast-overflow [2].
Pointer overflow is not interesting for us since it is widely used in
LuaJIT, in particular in <lj_alloc.c>. So, we may avoid warnings in
`NULL - ptr` arithmetics.

> 
> > +  )
> > +  if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
> 
> please add a link to GCC documentation
> 
> https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003dundefined

Added, thanks.

> 
> > +    string(JOIN "," UBSAN_IGNORE_OPTIONS
> > +      ${UBSAN_IGNORE_OPTIONS}
> > +      # Not interested in function type mismatch errors.
> > +      function
> > +    )
> > +  endif()
> > +  AppendFlags(CMAKE_C_FLAGS
> > +    # Enable hints for UndefinedBehaviorSanitizer.
> > +    -DLUAJIT_USE_UBSAN
> > +    # XXX: To get nicer stack traces in error messages.
> > +    -fno-omit-frame-pointer
> > +    # Enable UndefinedBehaviorSanitizer support.
> > +    # This flag enables all supported options (the documentation
> > +    # on cite is not correct about that moment, unfortunately)
> typo: cite -> site

Fixed, thanks.

> > +    # except float-divide-by-zero. Floating point division by zero
> > +    # behaviour is defined without -ffast-math and uses the
> > +    # IEEE 754 standard on which all NaN tagging is based.
> > +    -fsanitize=undefined
> > +    -fno-sanitize=${UBSAN_IGNORE_OPTIONS}
> > +    # Print a verbose error report and exit the program.
> > +    -fno-sanitize-recover=undefined
> > +  )
> > +endif()
> > +
> >   # Enable code coverage support.
> >   option(LUAJIT_ENABLE_COVERAGE "Enable code coverage support (gcovr)" OFF)
> >   if(LUAJIT_ENABLE_COVERAGE)
> > diff --git a/cmake/SetDynASMFlags.cmake b/cmake/SetDynASMFlags.cmake
> > index 7eead6e9..ae3c75b1 100644
> > --- a/cmake/SetDynASMFlags.cmake
> > +++ b/cmake/SetDynASMFlags.cmake
> > @@ -136,5 +136,16 @@ if(NOT CMAKE_SYSTEM_NAME STREQUAL ${CMAKE_HOST_SYSTEM_NAME})
> >     endif()
> >   endif()
> >   
> > +if(LUAJIT_USE_UBSAN)
> > +  # XXX: Skip checks for now to avoid build failures due to
> > +  # sanitizer errors.
> > +  # Need to backprot commits that fix the following issues first:
> typo: backprot -> backport

Fixed, thanks!

> > +  #https://github.com/LuaJIT/LuaJIT/pull/969,
> > +  #https://github.com/LuaJIT/LuaJIT/pull/970,
> > +  #https://github.com/LuaJIT/LuaJIT/issues/1041,
> > +  #https://github.com/LuaJIT/LuaJIT/pull/1044.
> > +  AppendFlags(HOST_C_FLAGS -fno-sanitize=undefined)
> > +endif()
> > +
> >   unset(LUAJIT_ARCH)
> >   unset(TESTARCH)
> > diff --git a/src/lj_carith.c b/src/lj_carith.c
> With this reverted patch tests passed. Do we really need this patch?

Yes, since cdata arithmetics depends on overflows of integers. So we
should ignore all warnings here.

> > index 4e1d450a..1d9d6fe1 100644
> > --- a/src/lj_carith.c
> > +++ b/src/lj_carith.c
> > @@ -159,6 +159,11 @@ static int carith_ptr(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
> >   }
> >   
> >   /* 64 bit integer arithmetic. */
> > +#if LUAJIT_USE_UBSAN
> > +/* Seehttps://github.com/LuaJIT/LuaJIT/issues/928. */
> > +static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
> > +  __attribute__((no_sanitize("signed-integer-overflow")));
> > +#endif
> >   static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
> >   {
> >     if (ctype_isnum(ca->ct[0]->info) && ca->ct[0]->size <= 8 &&

<snipped>

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029185.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029195.html
[3]: https://github.com/LuaJIT/LuaJIT/issues/1193

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2] ci: enable UBSan for sanitizers testing workflow

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey


LGTM

On 13.06.2024 13:35, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Please consider my answers below.
>
> On 07.06.24, Sergey Bronnikov wrote:
>> Sergey,
>>
>>
>> thanks for the patch! See my comments below.
>>
>> Sergey
>>
>> On 15.05.2024 15:32, Sergey Kaplun wrote:
>>> Relates to tarantool/tarantool#8473
>>> ---
>>>    .github/workflows/sanitizers-testing.yml | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/.github/workflows/sanitizers-testing.yml b/.github/workflows/sanitizers-testing.yml
>>> index 154ebe40..4bf7d023 100644
>>> --- a/.github/workflows/sanitizers-testing.yml
>>> +++ b/.github/workflows/sanitizers-testing.yml
> <snipped>
>
>>> @@ -91,5 +92,8 @@ jobs:
>>>                symbolize=1:                     \
>>>                unmap_shadow_on_exit=1:          \
>>>              "
>>> +          UBSAN_OPTIONS: "
>>> +            print_stacktrace=1 \
>> I propose to move these env variables to CMake.
> I suppose this is the same reason as for ASAN options: when run the asan
> + lsan CI in the tarantool repository, it configures its own ASAN +
> UBSAN options. To avoid their overwriting in our CMake file, we don't
> include this part there.
Got it, thanks!
>>> +          "
>>>            run: cmake --build . --parallel --target LuaJIT-test
>>>            working-directory: ${{ env.BUILDDIR }}

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

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey

thanks for the fixes and answers!

LGTM, anyway, please take a look at my answers below.

On 13.06.2024 13:56, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Please considered my answers below.
>
> On 07.06.24, Sergey Bronnikov wrote:
>> Sergey,
>>
>> thanks for the patch! Please see my comments below.
> Fixed your comments, see the iterative patch below.
> The branch is force-pushed.
Thanks!
<snipped>
>> On 15.05.2024 15:32, Sergey Kaplun wrote:
>>> This patch adds Undefined Behaviour Sanitizer [1] support. It enables
>>> all checks except several that are not useful for LuaJIT. Also, it
>>> instruments all known issues to be fixed in future patches (except
>>> `kfold_intop()` since cdata arithmetic relies on integer overflow).
>>>
>>> [1]:https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
>>>
>>> Resolves tarantool/tarantool#8473
>>> ---
>>>    CMakeLists.txt             | 45 ++++++++++++++++++++++++++++++++++++++
>>>    cmake/SetDynASMFlags.cmake | 11 ++++++++++
>>>    src/lj_carith.c            |  5 +++++
>>>    src/lj_opt_fold.c          |  5 +++++
>>>    src/lj_parse.c             |  5 +++++
>>>    src/lj_snap.c              |  7 ++++++
>>>    src/lj_strfmt.c            |  5 +++++
>>>    7 files changed, 83 insertions(+)
>> patch in mail is outdated, so I'll copypaste missed part:
> Yes, it is mentioned in this subthread [1].
>
>>
>> diff --git a/src/lj_buf.h b/src/lj_buf.h
>> index a4051694..aaecc9f8 100644
>> --- a/src/lj_buf.h
>> +++ b/src/lj_buf.h
>> @@ -70,6 +70,13 @@ LJ_FUNC SBuf *lj_buf_putmem(SBuf *sb, const void *q,
>> MSize len);
>>    LJ_FUNC SBuf * LJ_FASTCALL lj_buf_putchar(SBuf *sb, int c);
>>    LJ_FUNC SBuf * LJ_FASTCALL lj_buf_putstr(SBuf *sb, GCstr *s);
>>
>> +#if LUAJIT_USE_UBSAN
>> +/* The `NULL` argument with the zero length, like in the case:
>> +** | luajit -e 'error("x", 3)'
>> +*/
>> +static LJ_AINLINE char *lj_buf_wmem(char *p, const void *q, MSize len)
>> +  __attribute__((no_sanitize("nonnull-attribute")));
>> +#endif
>>    static LJ_AINLINE char *lj_buf_wmem(char *p, const void *q, MSize len)
>>    {
>>      return (char *)memcpy(p, q, len) + len;
>>
>>
>> With this reverted patch tests passed. Do we really need this patch?
> Should I add the corresponding test mentioned in [1]?
>
>>
>>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>>> index 2355ce17..edf2012f 100644
>>> --- a/CMakeLists.txt
>>> +++ b/CMakeLists.txt
>>> @@ -300,6 +300,51 @@ if(LUAJIT_USE_ASAN)
>>>      )
>>>    endif()
>>>    
>>> +option(LUAJIT_USE_UBSAN "Build LuaJIT with UndefinedBehaviorSanitizer" OFF)
>>> +if(LUAJIT_USE_UBSAN)
>>> +  # Use all recommendations from the UndefinedBehaviorSanitizer
>> probably you mean "checks" [1] and not "recommendations"
> Fixed, thanks.

Thanks!


>
>>
>> 1.https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks
>>
>>> +  # documentation:
>>> +  #https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html.
>>> +  string(JOIN "," UBSAN_IGNORE_OPTIONS
>>> +    # Misaligned pseudo-pointers are used to determine internal
>>> +    # variable names inside the `for` cycle.
>>> +    alignment
>>> +    # Not interested in float cast overflow errors.
>>> +    float-cast-overflow
>>> +    # NULL checking is disabled because this is not a UB and
>>> +    # raises lots of false-positive fails.
>>> +    null
>>> +    # Not interested in checking arithmetic with NULL.
>>> +    pointer-overflow
>>> +    # Shifts of negative numbers are widely used in parsing ULEB,
>>> +    # cdata arithmetic, vmevent hash calculation, etc.
>>> +    shift-base
>> Will we report issues produced by these checks to upstream?
> These particular checks -- no, since they are not so interesting for us,
> and most probably may be considered by Mike as "white noise".
>
> For others -- yes.
> I've already reported the related problem with the patch [3].
>
>> Decision "not interested" confuses.
> I've given the rationale for float-cast-overflow [2].
> Pointer overflow is not interesting for us since it is widely used in
> LuaJIT, in particular in <lj_alloc.c>. So, we may avoid warnings in
> `NULL - ptr` arithmetics.

I would replace "not interested" to smthing like "maintainer not 
interested".

Feel free to ignore.

>
>>> +  )
>>> +  if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
>> please add a link to GCC documentation
>>
>> https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003dundefined
> Added, thanks.
>
>>> +    string(JOIN "," UBSAN_IGNORE_OPTIONS
>>> +      ${UBSAN_IGNORE_OPTIONS}
>>> +      # Not interested in function type mismatch errors.
>>> +      function
>>> +    )
>>> +  endif()
>>> +  AppendFlags(CMAKE_C_FLAGS
>>> +    # Enable hints for UndefinedBehaviorSanitizer.
>>> +    -DLUAJIT_USE_UBSAN
>>> +    # XXX: To get nicer stack traces in error messages.
>>> +    -fno-omit-frame-pointer
>>> +    # Enable UndefinedBehaviorSanitizer support.
>>> +    # This flag enables all supported options (the documentation
>>> +    # on cite is not correct about that moment, unfortunately)
>> typo: cite -> site
> Fixed, thanks.
>
>>> +    # except float-divide-by-zero. Floating point division by zero
>>> +    # behaviour is defined without -ffast-math and uses the
>>> +    # IEEE 754 standard on which all NaN tagging is based.
>>> +    -fsanitize=undefined
>>> +    -fno-sanitize=${UBSAN_IGNORE_OPTIONS}
>>> +    # Print a verbose error report and exit the program.
>>> +    -fno-sanitize-recover=undefined
>>> +  )
>>> +endif()
>>> +
>>>    # Enable code coverage support.
>>>    option(LUAJIT_ENABLE_COVERAGE "Enable code coverage support (gcovr)" OFF)
>>>    if(LUAJIT_ENABLE_COVERAGE)
>>> diff --git a/cmake/SetDynASMFlags.cmake b/cmake/SetDynASMFlags.cmake
>>> index 7eead6e9..ae3c75b1 100644
>>> --- a/cmake/SetDynASMFlags.cmake
>>> +++ b/cmake/SetDynASMFlags.cmake
>>> @@ -136,5 +136,16 @@ if(NOT CMAKE_SYSTEM_NAME STREQUAL ${CMAKE_HOST_SYSTEM_NAME})
>>>      endif()
>>>    endif()
>>>    
>>> +if(LUAJIT_USE_UBSAN)
>>> +  # XXX: Skip checks for now to avoid build failures due to
>>> +  # sanitizer errors.
>>> +  # Need to backprot commits that fix the following issues first:
>> typo: backprot -> backport
> Fixed, thanks!
>
>>> +  #https://github.com/LuaJIT/LuaJIT/pull/969,
>>> +  #https://github.com/LuaJIT/LuaJIT/pull/970,
>>> +  #https://github.com/LuaJIT/LuaJIT/issues/1041,
>>> +  #https://github.com/LuaJIT/LuaJIT/pull/1044.
>>> +  AppendFlags(HOST_C_FLAGS -fno-sanitize=undefined)
>>> +endif()
>>> +
>>>    unset(LUAJIT_ARCH)
>>>    unset(TESTARCH)
>>> diff --git a/src/lj_carith.c b/src/lj_carith.c
>> With this reverted patch tests passed. Do we really need this patch?
> Yes, since cdata arithmetics depends on overflows of integers. So we
> should ignore all warnings here.
>
>>> index 4e1d450a..1d9d6fe1 100644
>>> --- a/src/lj_carith.c
>>> +++ b/src/lj_carith.c
>>> @@ -159,6 +159,11 @@ static int carith_ptr(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
>>>    }
>>>    
>>>    /* 64 bit integer arithmetic. */
>>> +#if LUAJIT_USE_UBSAN
>>> +/* Seehttps://github.com/LuaJIT/LuaJIT/issues/928. */
>>> +static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
>>> +  __attribute__((no_sanitize("signed-integer-overflow")));
>>> +#endif
>>>    static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
>>>    {
>>>      if (ctype_isnum(ca->ct[0]->info) && ca->ct[0]->size <= 8 &&
> <snipped>
>
> [1]:https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029185.html
> [2]:https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029195.html
> [3]:https://github.com/LuaJIT/LuaJIT/issues/1193
>

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

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

On 16.05.2024 13:14, Sergey Kaplun wrote:
> Hi, folks!
> Some more thoughts below.
>
> On 15.05.24, Sergey Kaplun wrote:
>
> <snipped>
>
>> +  string(JOIN "," UBSAN_IGNORE_OPTIONS
>> +    # Misaligned pseudo-pointers are used to determine internal
>> +    # variable names inside the `for` cycle.
>> +    alignment
>> +    # Not interested in float cast overflow errors.
>> +    float-cast-overflow
>> +    # NULL checking is disabled because this is not a UB and
>> +    # raises lots of false-positive fails.
>> +    null
> Maybe it is worth to add also "nonnull-attribute" to the ignore options:
>
> ```
> LSAN_OPTIONS="abort_on_error=1" src/luajit -e 'error("bad usage", 3)'
> /home/burii/builds_workspace/luajit/gh-8473-ubsan/src/lj_buf.h:75:25: runtime error: null pointer passed as argument 1, which is declared to never be null
> /usr/include/string.h:44:28: note: nonnull attribute specified here
> ```
>
> Here, `memcpy()` gets the NULL pointer as the first argument and the
> `len` == 0. So there are no problems here. Also, the nullability
> violation is not a UB, as mentioned in the documentation.
>
> Thoughts?

I agree with arguments provided by Maxim - it's UB, the standard states 
it clearly,

and it should be fixed. I propose to do the following:

- suppress the check (add it to UBSAN_IGNORE_OPTIONS) with appropriate 
comment

(something like "we know it is bad, but it is unfixed in upstream, we 
will wait a fix").

Probably we should suppress it per file like you did with other checks.

- send a fix to upstream or submit an issue to the upstream (I'll not 
insist, but it would be desirable)

>
>> +    # Not interested in checking arithmetic with NULL.
>> +    pointer-overflow
>> +    # Shifts of negative numbers are widely used in parsing ULEB,
>> +    # cdata arithmetic, vmevent hash calculation, etc.
>> +    shift-base
>> +  )

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

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the reply!

On 20.06.24, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> On 16.05.2024 13:14, Sergey Kaplun wrote:
> > Hi, folks!
> > Some more thoughts below.
> >
> > On 15.05.24, Sergey Kaplun wrote:
> >
> > <snipped>
> >
> >> +  string(JOIN "," UBSAN_IGNORE_OPTIONS
> >> +    # Misaligned pseudo-pointers are used to determine internal
> >> +    # variable names inside the `for` cycle.
> >> +    alignment
> >> +    # Not interested in float cast overflow errors.
> >> +    float-cast-overflow
> >> +    # NULL checking is disabled because this is not a UB and
> >> +    # raises lots of false-positive fails.
> >> +    null
> > Maybe it is worth to add also "nonnull-attribute" to the ignore options:
> >
> > ```
> > LSAN_OPTIONS="abort_on_error=1" src/luajit -e 'error("bad usage", 3)'
> > /home/burii/builds_workspace/luajit/gh-8473-ubsan/src/lj_buf.h:75:25: runtime error: null pointer passed as argument 1, which is declared to never be null
> > /usr/include/string.h:44:28: note: nonnull attribute specified here
> > ```
> >
> > Here, `memcpy()` gets the NULL pointer as the first argument and the
> > `len` == 0. So there are no problems here. Also, the nullability
> > violation is not a UB, as mentioned in the documentation.
> >
> > Thoughts?
> 
> I agree with arguments provided by Maxim - it's UB, the standard states 
> it clearly,
> 
> and it should be fixed. I propose to do the following:
> 
> - suppress the check (add it to UBSAN_IGNORE_OPTIONS) with appropriate 
> comment
> 
> (something like "we know it is bad, but it is unfixed in upstream, we 
> will wait a fix").
> 
> Probably we should suppress it per file like you did with other checks.

I suppress this particular failure inline, like for other checks.
Should I add the test for it?

> 
> - send a fix to upstream or submit an issue to the upstream (I'll not 
> insist, but it would be desirable)

I suppose this issue will be marked as won't fix or invalid. Feel free
to submit the ticket if you want.

> 
> >

<snipped>

> >> +  )

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 0/2] Add UBSan support

[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.1 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/10200
[2]: https://github.com/tarantool/tarantool/pull/10201
[3]: https://github.com/tarantool/tarantool/pull/10202

-- 
Best regards,
Sergey Kaplun