Use nxt_nitems() instead of sizeof() for strings (arrays)

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 caught a couple of bugs that were *almost*
introduced

First up is the _infamous_ ternary macro bug (yes, using the ternary
operator in a macro is of itself a bad idea)

    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.

Second up is a more straight forward case of simply calling nxt_length()
on a char * pointer.

Like the above this will generally result in a length of 7.

When you sit and think about it, you know very well sizeof(char *) is
probably 8 these days (but may be some other value like 4).

But when you're in the depths of code it's very easy to overlook this
when all you're thinking about is to get the length of some string.

Let's look at this patch in action

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

  #define nxt_nitems(x)		(sizeof(x) / sizeof((x)[0]))
  #define nxt_length(s)		(nxt_nitems(s) - 1)

  #define nxt_unsafe_length(s)	(sizeof(s) - 1)

  #define STR_LITERAL		"1234567890"

  static const char *str_lit  = "1234567890";

  int main(void)
  {
  	printf("[STR_LITERAL] nxt_unsafe_length(\"1234567890\") [%lu]\n",
  	       nxt_unsafe_length(STR_LITERAL));
  	printf("[STR_LITERAL] nxt_length(\"1234567890\")        [%lu]\n",
  	       nxt_length(STR_LITERAL));

  	printf("[char *     ] nxt_unsafe_length(\"1234567890\") [%lu]\n",
  	       nxt_unsafe_length(str_lit));
  	printf("[char *     ] nxt_length(\"1234567890\")        [%lu]\n",
  	       nxt_length(str_lit));

  	return 0;
  }

First lets compile it without any flags

  $ make sdiv
  $ ./sdiv
  [STR_LITERAL] nxt_unsafe_length("1234567890") [10]
  [STR_LITERAL] nxt_length("1234567890")        [10]
  [char *     ] nxt_unsafe_length("1234567890") [7]
  [char *     ] nxt_length("1234567890")        [7]

It compiled without error and runs, although with incorrect results for
the two char *'s.

Now lets build it with -Wsizeof-pointer-div (also enabled with -Wall)

  $ CFLAGS="-Wsizeof-pointer-div" make sdiv
  cc -Wsizeof-pointer-div    nxt_nitems.c   -o nxt_nitems
  sdiv.c: In function ‘main’:
  sdiv.c:3:44: warning: division ‘sizeof (const char *) / sizeof (char)’ does not compute the number of array elements [-Wsizeof-pointer-div]
      3 | #define nxt_nitems(x)           (sizeof(x) / sizeof((x)[0]))
        |                                            ^
  nxt_nitems.c:4:34: note: in expansion of macro ‘nxt_nitems’
      4 | #define nxt_length(s)           (nxt_nitems(s) - 1)
        |                                  ^~~~~~~~~~
  nxt_nitems.c:22:16: note: in expansion of macro ‘nxt_length’
     22 |                nxt_length(str_lit));
        |                ^~~~~~~~~~
  nxt_nitems.c:10:20: note: first ‘sizeof’ operand was declared here
     10 | static const char *str_lit    = "1234567890";
        |                    ^~~~~~~

So we now get a very loud compiler warning (coming from nxt_length(char
*), nxt_unsafe_length() of course didn't trigger any warnings), telling
us we're being daft.

The good news is this didn't find any existing bugs! Let's keep it that
way...

Link: <https://stackoverflow.com/a/57537491>
Cc: Andrew Clayton <a.clayton@nginx.com>
Signed-off-by: Alejandro Colomar <alx@nginx.com>
Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
Tested-by: Andrew Clayton <a.clayton@nginx.com>
[ Tweaked and expanded the commit message - Andrew ]
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

1 files changed, 1 insertions, 1 deletions

diff --git a/src/nxt_clang.h b/src/nxt_clang.h
index 94638346..6803ffc8 100644
--- a/src/nxt_clang.h
+++ b/src/nxt_clang.h
@@ -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_ */