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

[Maxim Dounin]

Hello!

On Mon, Nov 07, 2022 at 03:07:36PM +0100, Alejandro Colomar wrote:

> 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
>  
>  

Thanks, but no.

It's unfortunate to see that new developers in NGINX Unit actually 
do allow such trivial bugs to happen, but on the other hand that's 
somewhat expected effect of dumping the whole development team.

Either way, suggested change looks bad to me.  It obfuscates 
what's going on in the macro, making it harder to understand it 
and to use it correctly.  While it might save some newbie 
developers from making silly mistakes, it will also make the code 
harder to understand for a lot more developers.

Note well that using ternary operators in macro arguments is a bad 
coding practice regardless of whether it works with the particular 
macro or not, and should be avoided.

Hope this helps.

-- 
Maxim Dounin
http://mdounin.ru/