Welcome! Log In Create A New Profile

Advanced

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

Maxim Dounin
November 09, 2022 10:04AM
Hello!

On Tue, Nov 08, 2022 at 11:55:40AM +0100, Alejandro Colomar wrote:

> From: Alejandro Colomar <alx@kernel.org>
>
> 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 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.
>
> Apart from the small risk, there's a bigger concern about readability.
> Casts tend to disable compiler warnings, and so are unsafe by nature.
> A lot of care needs to be taken when casts are used, and a programmer
> reading the cast might wonder what was that specific cast trying to
> achieve. The answer is clear: nothing. So it's better to just prevent
> that moment of "wtf is this doing here".
>
> Signed-off-by: Alejandro Colomar <alx@nginx.com>
> ---
> src/core/ngx_string.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h
> index 0fb9be72..6c218e22 100644
> --- a/src/core/ngx_string.h
> +++ b/src/core/ngx_string.h
> @@ -145,7 +145,7 @@ ngx_copy(u_char *dst, u_char *src, size_t len)
>
>
> /* msvc and icc7 compile memcmp() to the inline loop */
> -#define ngx_memcmp(s1, s2, n) memcmp((const char *) s1, (const char *) s2, n)
> +#define ngx_memcmp(s1, s2, n) memcmp(s1, s2, n)
>
>
> u_char *ngx_cpystrn(u_char *dst, u_char *src, size_t n);

The suggested commit log seems to be referring to non-nginx code,
and in general seems to be overcomplicated on the one hand, and
lacks details supporting the claim that these casts are not needed
in the nginx code as discussed in this thread on the other hand.

The suggested change, while being mostly style, does not take into
account expected alignment of adjacent macro definitions: since
the ngx_memcmp() macro definition is now short enough, it can be
aligned similarly to ngx_memzero()/ngx_memcpy()/etc.

While here, fixed alignment of ngx_memmove() and ngx_movemem()
macro definitions, these were off by 1 character.

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1668004692 -10800
# Wed Nov 09 17:38:12 2022 +0300
# Node ID fc79ea0724a92c1f463625a11ed4cb785cd342b7
# Parent 42bc158a47ecb3c2bd0396c723c307c757f2770e
Fixed alignment of ngx_memmove()/ngx_movemem() macro definitions.

diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h
--- a/src/core/ngx_string.h
+++ b/src/core/ngx_string.h
@@ -140,8 +140,8 @@ ngx_copy(u_char *dst, u_char *src, size_
#endif


-#define ngx_memmove(dst, src, n) (void) memmove(dst, src, n)
-#define ngx_movemem(dst, src, n) (((u_char *) memmove(dst, src, n)) + (n))
+#define ngx_memmove(dst, src, n) (void) memmove(dst, src, n)
+#define ngx_movemem(dst, src, n) (((u_char *) memmove(dst, src, n)) + (n))


/* msvc and icc7 compile memcmp() to the inline loop */
# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1668005196 -10800
# Wed Nov 09 17:46:36 2022 +0300
# Node ID 5269880f00df1e5ae08299165ec43435b759c5a3
# Parent fc79ea0724a92c1f463625a11ed4cb785cd342b7
Removed casts from ngx_memcmp() macro.

Casts are believed to be not needed, since memcmp() has "const void *"
arguments since introduction of the "void" type in C89. And on pre-C89
platforms nginx is unlikely to compile without warnings anyway, as there
are no casts in memcpy() and memmove() calls.

These casts were added in 1648:89a47f19b9ec without any details on why they
were added, and Igor does not remember details either. The most plausible
explanation is that they were copied from ngx_strcmp() and were not really
needed even at that time.

Prodded by Alejandro Colomar.

diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h
--- a/src/core/ngx_string.h
+++ b/src/core/ngx_string.h
@@ -145,7 +145,7 @@ ngx_copy(u_char *dst, u_char *src, size_


/* msvc and icc7 compile memcmp() to the inline loop */
-#define ngx_memcmp(s1, s2, n) memcmp((const char *) s1, (const char *) s2, n)
+#define ngx_memcmp(s1, s2, n) memcmp(s1, s2, n)


u_char *ngx_cpystrn(u_char *dst, u_char *src, size_t n);

--
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 508 November 04, 2022 11:26AM

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

Alejandro Colomar 112 November 04, 2022 11:30AM

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

Maxim Dounin 115 November 04, 2022 10:40PM

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

Alejandro Colomar 101 November 06, 2022 05:52PM

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

Maxim Dounin 97 November 08, 2022 04:52AM

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

Alejandro Colomar 135 November 08, 2022 05:50AM

[PATCH v2] Removed the casts within ngx_memcmp()

Alejandro Colomar 92 November 08, 2022 05:58AM

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

Maxim Dounin 90 November 09, 2022 10:04AM

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

Maxim Dounin 108 November 29, 2022 11:24PM

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

Alex Colomar 79 November 30, 2022 05:50AM

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

Maxim Dounin 125 November 30, 2022 08:22PM

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

Sergey Kandaurov 108 November 30, 2022 07:18AM

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

Maxim Dounin 82 November 30, 2022 08:08PM

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

Alejandro Colomar 94 November 08, 2022 06:18AM



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

Online Users

Guests: 109
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