On Wed, Apr 16, 2025 at 08:40:17PM +0200, Thomas Weißschuh wrote:

Both constructors and main() may be executed with different function signatures than they are actually using. This is intentional but trips up UBSAN.

Disable the function sanitizer of UBSAN in _start_c().

Signed-off-by: Thomas Weißschuh linux@weissschuh.net

tools/include/nolibc/crt.h | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/tools/include/nolibc/crt.h b/tools/include/nolibc/crt.h index c4b10103bbec50f1a3a0a4562e34fdbd1b43ce6f..961cfe777c3564e705dfdd581de828b374d05b0b 100644 --- a/tools/include/nolibc/crt.h +++ b/tools/include/nolibc/crt.h @@ -7,6 +7,8 @@ #ifndef _NOLIBC_CRT_H #define _NOLIBC_CRT_H +#include "compiler.h"

char **environ __attribute__((weak)); const unsigned long *_auxv __attribute__((weak)); @@ -25,6 +27,9 @@ extern void (*const __fini_array_end[])(void) __attribute__((weak)); void _start_c(long *sp); __attribute__((weak,used)) +#if __nolibc_has_feature(undefined_behavior_sanitizer)

• __attribute__((no_sanitize("function")))

+#endif

I'm wondering if it wouldn't be more reliable with:

#if __nolibc_has_attribute(no_sanitize) __attribute__((no_sanitize("function"))) #endif

Because in the end, what you want is to always place that attribute whenever it's supported, no ?

void _start_c(long *sp) { long argc;

Willy

On Sat, Apr 19, 2025 at 12:10:24PM +0200, Thomas Weißschuh wrote:

On 2025-04-19 11:06:31+0200, Willy Tarreau wrote:

...

On Wed, Apr 16, 2025 at 08:40:17PM +0200, Thomas Weißschuh wrote:

...

Both constructors and main() may be executed with different function signatures than they are actually using. This is intentional but trips up UBSAN.

Disable the function sanitizer of UBSAN in _start_c().

Signed-off-by: Thomas Weißschuh linux@weissschuh.net

tools/include/nolibc/crt.h | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/tools/include/nolibc/crt.h b/tools/include/nolibc/crt.h index c4b10103bbec50f1a3a0a4562e34fdbd1b43ce6f..961cfe777c3564e705dfdd581de828b374d05b0b 100644 --- a/tools/include/nolibc/crt.h +++ b/tools/include/nolibc/crt.h @@ -7,6 +7,8 @@ #ifndef _NOLIBC_CRT_H #define _NOLIBC_CRT_H +#include "compiler.h"

char **environ __attribute__((weak)); const unsigned long *_auxv __attribute__((weak)); @@ -25,6 +27,9 @@ extern void (*const __fini_array_end[])(void) __attribute__((weak)); void _start_c(long *sp); __attribute__((weak,used)) +#if __nolibc_has_feature(undefined_behavior_sanitizer)

• __attribute__((no_sanitize("function")))

+#endif

I'm wondering if it wouldn't be more reliable with:

#if __nolibc_has_attribute(no_sanitize) __attribute__((no_sanitize("function"))) #endif

Because in the end, what you want is to always place that attribute whenever it's supported, no ?

That doesn't work because GCC does knows no_sanitize but not no_sanitize("function"). Also no_sanitize is not specific to UBSAN but works for all sanitizers.

OK, thanks for explaining!

Willy

On Wed, Apr 16, 2025 at 08:40:18PM +0200, Thomas Weißschuh wrote:

As byte buffer is overlaid with a 'struct dirent64'. it has to satisfy the structs alignment requirements.

Fixes: 665fa8dea90d ("tools/nolibc: add support for directory access") Signed-off-by: Thomas Weißschuh linux@weissschuh.net

Oh good catch! I already faced crashes in other programs due to AVX memcpy when doing similar casts without thinking about alignment.

@@ -58,6 +58,7 @@ int closedir(DIR *dirp) static __attribute__((unused)) int readdir_r(DIR *dirp, struct dirent *entry, struct dirent **result) {

• __attribute__((aligned(__alignof__(struct linux_dirent64)))) char buf[sizeof(struct linux_dirent64) + NAME_MAX + 1];

I tend to find it more readable when the attribute is on the same line as the type on variables declaration, often at the end of the line, to keep declarations alignes, even if that makes longer lines. If alignment concerns come back often, we could maybe have __nolibc_align(<align>) and maybe even __nolibc_align_as(<type>) to slightly shorten the lines. Just an idea.

In any case: Acked-by: Willy Tarreau w@1wt.eu

Willy

On 2025-04-19 11:40:08+0200, Willy Tarreau wrote:

On Wed, Apr 16, 2025 at 08:40:19PM +0200, Thomas Weißschuh wrote:

...

In twos complement the most negative number can not be negated.

well, if we're picky, that's not really an int overflow since it's only used as an unsigned in the end, so -2^(N-1) == 2^(N-1) in twos complement.

It is used as unsigned, but at the point of the cast it's still signed. From ccpreference:

The unary minus invokes undefined behavior due to signed integer overflow when applied to INT_MIN, LONG_MIN, or LLONG_MIN, on typical (2's complement) platforms.

https://en.cppreference.com/w/c/language/operator_arithmetic

...

@@ -271,16 +271,12 @@ int utoa_r(unsigned long in, char *buffer) static __attribute__((unused)) int itoa_r(long in, char *buffer) {

• char *ptr = buffer;
• int len = 0;
• if (in < 0) {

• in = -in;
  

• *(ptr++) = '-';
  

• len++;
  

• *(buffer++) = '-';
  

• return 1 + utoa_r(-(unsigned long)in, buffer);
  

  }

• len += utoa_r(in, ptr);
• return len;

• return utoa_r(in, buffer);

}

At -Os it's OK but at -O2 it inflates it a little bit and at -O3 it significantly inflates the function (175 -> 320 bytes) due to the two calls that get inlined. Have you tried to check if ubsan is happy with just this?

• in = -in;
  

• in = -(unsigned long)in;
  

That seems to work. Let's use it.

Thanks!

Otherwise this variant doesn't inflate it for me and keeps the spirit of the original one (i.e. single call):

int itoa_r3(long in, char *buffer) { unsigned long uin = in; int len = 0;

    if ((long)uin < 0) {
            uin = -uin;
            *(buffer++) = '-';
            len++;
    }
    len += utoa_r(uin, buffer);
    return len;

}

I'm also seeing a way to make it even smaller than the original by changing utoa_r() so that it takes the original buffer in a 3rd argument and emits the difference at the end as the length. This allows to always perform a tail call, though I'm not sure we really need it.

Thomas