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

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

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