summaryrefslogtreecommitdiffhomepage
diff options
context:
space:
mode:
authorAlejandro Colomar <alx@nginx.com>2022-11-07 11:27:47 +0100
committerAlejandro Colomar <alx@nginx.com>2022-11-15 13:05:24 +0100
commit6ab70192fa26e801ac5891aad85bc6b3ff819782 (patch)
treec7f6f4099fa602e2424bdfe20494a605e7af5b86
parent269bc8e4e86b6be5cd7acd5ca41b1a86a9032c8c (diff)
downloadunit-6ab70192fa26e801ac5891aad85bc6b3ff819782.tar.gz
unit-6ab70192fa26e801ac5891aad85bc6b3ff819782.tar.bz2
Using nxt_sizeof_array() 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_sizeof_array(), 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/#review4063> even though it was not fully understood what was causing it. Link: <https://stackoverflow.com/questions/37538/how-do-i-determine-the-size-of-my-array-in-c/57537491#57537491> Cc: Andrew Clayton <a.clayton@nginx.com> Signed-off-by: Alejandro Colomar <alx@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 919d9168..f465d562 100644
--- a/src/nxt_clang.h
+++ b/src/nxt_clang.h
@@ -256,7 +256,7 @@ nxt_popcount(unsigned int x)
#define nxt_length(s) \
- (sizeof(s) - 1)
+ (nxt_sizeof_array(s) - 1)
#endif /* _NXT_CLANG_H_INCLUDED_ */