Welcome! Log In Create A New Profile

Advanced

Re: [PATCH] Removed the unsafe ngx_memcmp() wrapper for memcmp(3)

Maxim Dounin
November 04, 2022 10:40PM
Hello!

On Fri, Nov 04, 2022 at 04:24:14PM +0100, Alejandro Colomar wrote:

> The casts are unnecessary, since memcmp(3)'s arguments are
> 'const void *', which allows implicit conversion from any pointer type.
> It might have been necessary in the times of K&R C, where 'void *'
> didn't exist yet, up until the early 2000s, because some old systems
> still had limited or no support for ISO C89. Those systems passed away
> a long time ago, and current systems, even the oldest living ones, have
> support for ISO C89.
>
> The changes, apart from the removal of the macro itself, were scripted:
>
> $ find src/ -type f \
> | grep '\.[ch]$' \
> | xargs sed -i 's/ngx_memcmp/memcmp/';
>
> The cast in this case is (almost) innocuous, because it only hides
> warnings for conversions from integer to pointer such as:
>
> nxt_memcmp(n, "foo", 3); // no warnings
> memcmp(n, "foo", 3); // warning: integer to pointer conversion
>
> which is a difficult bug to write, since it's too obvious. Such code
> will probably be caught in a code review, but there's always a small
> risk. Since there's no reason to keep the small risk around, when we
> can just avoid it by removing the cast.
>
> In general, it's better to avoid a cast if possible, since casts will
> disable many compiler warnings regarding type safety.

Thanks for the patch.

The ngx_memcpy() macro was introduced in 107:b5be4b0448d3
(nginx-0.0.1-2003-07-01-19:00:03 import) as an alias to memcmp(),
with the following explanation why it's just an alias:

+/* msvc and icc compile memcmp() to inline loop */
+#define ngx_memcmp memcmp

This was along with the first use of memcmp in nginx, so clearly
the "ngx_" prefix is not an accident.

Indeed, the prefix is used in multiple places to provide optimized
or instrumented functions, even if normally they map to generally
available functions such as memcmp(). For example, ngx_memcpy()
usually maps to memcpy(), but instrumented to detect large copies
if NGX_MEMCPY_LIMIT is defined. Even if the particular function
is not instrumented, the prefix is important to make it possible
to change things without huge modifications of the code, like the
one in your patch. Further, the prefix is also important for code
consistency. And, last but not least, it makes it possible to add
casts if needed on some platforms (or remove them if they are
considered wrong).

In particular, casts to (const char *) for ngx_memcmp() were added
in 2007 by this commit (nginx 0.6.18):

changeset: 1648:89a47f19b9ec
user: Igor Sysoev <igor@sysoev.ru>
date: Fri Nov 23 17:00:11 2007 +0000
summary: update ngx_memcmp()

Unfortunately, this commit contains no details about the
particular system which needed the cast, though it looks like
there were some at least at that time.

Similar casts are used by almost all nginx string macro
definitions, since most C library functions accept "char *", while
nginx uses "u_char *" for strings, and this results in useless
warnings. While it might be seen as something better to avoid,
the only alternative would be to provide wrapper functions, which
might not be a good idea for performance and readability reasons.
On the other hand, macro definitions with casts are easily
readable, have no impact on performance, and provide limited to
no effect on the code quality as long as proper coding, testing
and review process is implemented.

Summing the above, certainly removing the "ngx_" prefix is a bad
idea. Removing casts from the particular macro might be
considered, though in general it looks like a quest to prove these
are not needed (or no longer needed) given the wide range of
platforms nginx supports.

Hope this helps.

[...]

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org
Subject Author Views Posted

[PATCH] Removed the unsafe ngx_memcmp() wrapper for memcmp(3)

Alejandro Colomar 476 November 04, 2022 11:26AM

Re: [PATCH] Removed the unsafe ngx_memcmp() wrapper for memcmp(3)

Alejandro Colomar 100 November 04, 2022 11:30AM

Re: [PATCH] Removed the unsafe ngx_memcmp() wrapper for memcmp(3)

Maxim Dounin 105 November 04, 2022 10:40PM

Re: [PATCH] Removed the unsafe ngx_memcmp() wrapper for memcmp(3)

Alejandro Colomar 86 November 06, 2022 05:52PM

Re: [PATCH] Removed the unsafe ngx_memcmp() wrapper for memcmp(3)

Maxim Dounin 86 November 08, 2022 04:52AM

Re: [PATCH] Removed the unsafe ngx_memcmp() wrapper for memcmp(3)

Alejandro Colomar 113 November 08, 2022 05:50AM

[PATCH v2] Removed the casts within ngx_memcmp()

Alejandro Colomar 83 November 08, 2022 05:58AM

Re: [PATCH v2] Removed the casts within ngx_memcmp()

Maxim Dounin 78 November 09, 2022 10:04AM

Re: [PATCH v2] Removed the casts within ngx_memcmp()

Maxim Dounin 86 November 29, 2022 11:24PM

Re: [PATCH v2] Removed the casts within ngx_memcmp()

Alex Colomar 65 November 30, 2022 05:50AM

Re: [PATCH v2] Removed the casts within ngx_memcmp()

Maxim Dounin 110 November 30, 2022 08:22PM

Re: [PATCH v2] Removed the casts within ngx_memcmp()

Sergey Kandaurov 96 November 30, 2022 07:18AM

Re: [PATCH v2] Removed the casts within ngx_memcmp()

Maxim Dounin 72 November 30, 2022 08:08PM

u_char vs char (was: [PATCH] Removed the unsafe ngx_memcmp() wrapper for memcmp(3))

Alejandro Colomar 86 November 08, 2022 06:18AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 102
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready