summaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorAlejandro Colomar <alx.manpages@gmail.com>2022-11-05 01:10:44 +0100
committerAndrew Clayton <a.clayton@nginx.com>2024-10-29 01:33:37 +0000
commit85f21b7cf30bc5bf0cc39aa9a7ebb2a25cd27a78 (patch)
tree27091b54184be007a0f840a6fd91f6c2e805e524
parentebd02c6026f976ab8ca03b6008ff866a86b6ad57 (diff)
downloadunit-85f21b7cf30bc5bf0cc39aa9a7ebb2a25cd27a78.tar.gz
unit-85f21b7cf30bc5bf0cc39aa9a7ebb2a25cd27a78.tar.bz2
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>
-rw-r--r--src/nxt_clang.h2
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_ */