Skip to content

Commit

Permalink
Use nxt_nitems() instead of sizeof() for strings (arrays)
Browse files Browse the repository at this point in the history
sizeof() should never be used to get the size of an array.  It is
very unsafe, since arrays easily decay to pointers, and sizeof()
applied to a pointer gives false results that compile and produce
silent bugs.

It's better to use nxt_items(), which implements sizeof()
division, which recent compilers warn when used with pointers.

This change would have avoided a bug that we almost introduced
recently by using:

    nxt_str_set(&port, (r->tls ? "https://" : "http://"));

which in the macro expansion runs:

    (&port)->length = nxt_length((r->tls ? : "https://" : "http://"));

which evaluates to:

    port.length = sizeof(r->tls ? "https://" : "http://") - 1;

which evaluates to:

    port.length = 8 - 1;

Of course, we didn't want a compile-time-constant 8 there, but
rather the length of the string.

The above bug is not obvious to the untrained eye, so let's show some
example programs that may give some more hints about the problem.

$ cat sizeof.c
 #include <stdio.h>

int
main(void)
{
    printf("%zu\n", sizeof("01"));
    printf("%zu\n", sizeof("012"));
    printf("%zu\n", sizeof(char *));
}
$ cc -Wall -Wextra sizeof.c
$ ./a.out
3
4
8

sizeof() returns the size in bytes of the array passed to it, which in
case of char strings, it is equivalent to the length of the string + 1
(for the terminating '\0').

However, arrays decay very easily in C, and they decay to a pointer to
the first element in the array.  In case of strings, that is a 'char *'.

When sizeof() is given a pointer, it returns the size of the pointer,
which in most platforms is 8.

The ternary operator (?) performs default promotions (and other
nefarious stuff) that may surprise even the most experienced
programmers.  It contrasts the __builtin_choose_expr() GCC builtin [1],
which performs almost equivalently, but without the unwanted effects of
the ternary operator.

[1]: <https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fchoose_005fexpr>

$ cat ?.c
 #include <stdio.h>

int
main(void)
{
    printf("%zu\n", sizeof("01"));
    printf("%zu\n", sizeof(__builtin_choose_expr(1, "01", "01")));
    printf("%zu\n", sizeof(1 ? "01" : "01"));
    printf("%zu\n", sizeof(char *));
}
$ cc -Wall -Wextra ?.c
$ ./a.out
3
3
8
8

In the above program, we can see how the ternary operator (?) decays
the array into a pointer, and makes it so that sizeof() will return a
constant 8.

As we can see, everything in the use of the macro would make it look
like it should work, but the combination of some seemingly-safe side
effects of various C features produces a completely unexpected bug.

The bug dissected here was originally found in our Review Board:
<https://rb.nginx.com/r/1113/#gcomment2489>
even though it was not fully understood what was causing it.

Link: <https://stackoverflow.com/a/57537491>
Cc: Andrew Clayton <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Reviewed-by: Andrew Clayton <[email protected]>
Tested-by: Andrew Clayton <[email protected]>
Signed-off-by: Andrew Clayton <[email protected]>
  • Loading branch information
alejandro-colomar authored and ac000 committed Oct 23, 2024
1 parent f6036bb commit bbb620b
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/nxt_clang.h
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ nxt_popcount(unsigned int x)


#define nxt_length(s) \
(sizeof(s) - 1)
(nxt_nitems(s) - 1)


#endif /* _NXT_CLANG_H_INCLUDED_ */

0 comments on commit bbb620b

Please sign in to comment.