[PATCH 2/2] Core: replaced unsafe uses of sizeof() by ngx_nitems().

[Alejandro Colomar]

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 especially important within macros that will be used in unplanned
cases, where a programmer would just expect it to work, rather than
inline in code where it can be more obvious that some combination is
not a good idea.

An important case where arrays can decay without the programmer
noticing is in the ternary operator (? :).  The ternary operator
applies default promotions and other undesired effects to the arguments,
which causes arrays to decay to pointers.

The following expression seems reasonable:

    ngx_string(tls ? "https://" : "http://")

And it is not.  The code above would be expanded (prior to this patch)
to:

    { sizeof(tls ? "https://" : "http://") - 1,
      (u_char *) tls ? "https://" : "http://" }

which evaluates to:

    { sizeof(char *) - 1, (u_char *) tls ? "https://" : "http://" }

which evaluates to:

    { 8 - 1, (u_char *) tls ? "https://" : "http://" }

Of course, a programmer would not want that, but rather:

    { (tls ? 9 : 8) - 1, (u_char *) tls ? "https://" : "http://" }

The worst part in this example is that since one of the strings has
exactly the same size as a pointer in most platforms, testing would
not report an issue in one of the paths of code (coincidentally, the
easier one to test), so it would be very difficult to detect this bug,
either in tests, or in code review.

This example is not a hypothetical one, but rather one that was found
by chance in Nginx Unit.  Now, imagine that both strings in the ternary
operator would have 8 bytes: tests would not possibly catch the bug,
and future changes to the code where one of the strings might change
would result in a completely unexpected bug that would be very hard to
track.

This patch makes such code trigger a compile-time warning that prevents
this class of bugs by using this macro.

This is also a recommendation that new code measuring length of arrays
uses the same macro instead of sizeof() directly.  A stackoverflow post
linked below details some more recommendations about sizeof() and
arrays.

Link: <https://stackoverflow.com/a/57537491>
Cc: Andrew Clayton <a.clayton at nginx.com>
Cc: Zhidao Hong <z.hong at f5.com>
Signed-off-by: Alejandro Colomar <alx at nginx.com>
---
 src/core/ngx_string.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h
index 0fb9be72..ad7b51ec 100644
--- a/src/core/ngx_string.h
+++ b/src/core/ngx_string.h
@@ -37,10 +37,10 @@ typedef struct {
 } ngx_variable_value_t;
 
 
-#define ngx_string(str)     { sizeof(str) - 1, (u_char *) str }
+#define ngx_string(str)     { ngx_nitems(str) - 1, (u_char *) str }
 #define ngx_null_string     { 0, NULL }
 #define ngx_str_set(str, text)                                               \
-    (str)->len = sizeof(text) - 1; (str)->data = (u_char *) text
+    (str)->len = ngx_nitems(text) - 1; (str)->data = (u_char *) text
 #define ngx_str_null(str)   (str)->len = 0; (str)->data = NULL
 
 
-- 
2.37.2