Re: NULL pointer arithmetic issues

[Kamil Rytarowski <n54%gmx.com@localhost>]

On 22.02.2020 17:25, Kamil Rytarowski wrote:
> When running the ATF tests under MKLIBCSANITIZER [1], there are many
> NULL pointer arithmetic issues .
> 
> http://netbsd.org/~kamil/mksanitizer-reports/ubsan-2020-02-22-null-pointer.txt
> 
> These issues are in macros like:
>  - IN_ADDRHASH_READER_FOREACH()
>  - IN_ADDRLIST_WRITER_INSERT_TAIL()
>  - IFADDR_READER_FOREACH()
>  - etc
> 
> These macros wrap internally pserialize-safe linked lists.
> 
> What's the proper approach to address this issue?
> 
> These reports are responsible for around half of all kinds of the
> remaining Undefined Behavior unique issues when executing ATF tests.
> 
> 
> [1] ./build.sh -N0 -U -V MAKECONF=/dev/null -V HAVE_LLVM=yes -V MKGCC=no
> -V MKLLVM=yes -V MKLIBCSANITIZER=yes -j8 -u -O /public/netbsd-llvm
> distribution
> 
> 
> 

NULL + 0 and NULL + x are both Undefined Behavior (as confirmed by a C
committee member, it was discussed and confirmed to be intentional).

NULL+x is now miscompiled by Clang/LLVM after this commit:

https://reviews.llvm.org/rL369789

This broke various programs like:

"Performing base + offset pointer arithmetic is only allowed when base
itself is not nullptr. In other words, the compiler is assumed to allow
that base + offset is always non-null, which an upcoming compiler
release will do in this case. The result is that CommandStream.cpp,
which calls this in a loop until the result is nullptr, will never
terminate (until it runs junk data and crashes)."

https://github.com/google/filament/pull/1566

LLVM middle-end uses those guarantees for transformations.


This pushed the LLVM devs to implement this NULL+x and NULL+0 check in
UBSan:

https://reviews.llvm.org/D67122

NULL+0 was added to UBSan proactively as it is as of today not
miscompiled, but it is planned to so in not so distant time. It is a
chicken-egg problem.

There is however a fallback. If we want to use NULL+0, we must use
-fno-delete-null-pointer-checks that avoids miscompilation and raising
UBSan reports. If we want to allow NULL+x we must enable -fwrapv.

This means that we can avoid pserialize reports by enabling these CFLAGS
for clang as well.

     22 # We are compiling the kernel code with
no-delete-null-pointer-checks,
     23 # and compiling without it, causes issues at least on sh3 by adding
     24 # aborts after kern_assert on NULL pointer checks.
     25 CFLAGS+=    ${${ACTIVE_CC} == "gcc":?
-fno-delete-null-pointer-checks :}
     26

https://nxr.netbsd.org/xref/src/sys/rump/Makefile.rump#25

I'm going to test it and switch -fno-delete-null-pointer-check on
Clang/LLVM.


For the historical note, a reproducer miscompiling NULL + x:

http://netbsd.org/~kamil/null_plus_x-ub.c

136 kamil@chieftec /tmp $ /usr/local/bin/clang -O0 null_plus_x-ub.c
137 kamil@chieftec /tmp $ ./a.out
1
138 kamil@chieftec /tmp $ /usr/local/bin/clang -O2 null_plus_x-ub.c
139 kamil@chieftec /tmp $ ./a.out
0
151 kamil@chieftec /tmp $ /usr/local/bin/clang -O3 -fwrapv null_plus_x-ub.c
152 kamil@chieftec /tmp $ ./a.out

1

NULL+0 is planned to follow up.

Attachment: signature.asc
Description: OpenPGP digital signature