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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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