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

Alejandro Colomar alx.manpages at gmail.com
Fri Nov 4 15:24:14 UTC 2022


From: Alejandro Colomar <alx at nginx.com>

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.

Cc: Andrei Zeliankou <a.zelenkov at f5.com>
Cc: Andrew Clayton <a.clayton at nginx.com>
Cc: Artem Konev <a.konev at f5.com>
Cc: Liam Crilly <liam at nginx.com>
Cc: Timo Stark <t.stark at nginx.com>
Cc: Zhidao Hong <z.hong at f5.com>
Signed-off-by: Alejandro Colomar <alx at nginx.com>
---
 src/core/ngx_inet.c                           |  4 ++--
 src/core/ngx_resolver.c                       |  4 ++--
 src/core/ngx_string.c                         |  6 ++---
 src/core/ngx_string.h                         |  4 ----
 src/event/ngx_event_openssl.c                 |  2 +-
 src/http/modules/ngx_http_geo_module.c        |  2 +-
 .../modules/ngx_http_secure_link_module.c     |  2 +-
 src/http/ngx_http_core_module.c               |  4 ++--
 src/http/ngx_http_file_cache.c                | 10 ++++-----
 src/http/ngx_http_request.c                   |  2 +-
 src/http/v2/ngx_http_v2.c                     | 22 ++++++-------------
 src/mail/ngx_mail_handler.c                   |  2 +-
 src/stream/ngx_stream_geo_module.c            |  2 +-
 src/stream/ngx_stream_handler.c               |  2 +-
 14 files changed, 28 insertions(+), 40 deletions(-)

diff --git a/src/core/ngx_inet.c b/src/core/ngx_inet.c
index 4228504a..e8ffc288 100644
--- a/src/core/ngx_inet.c
+++ b/src/core/ngx_inet.c
@@ -1349,7 +1349,7 @@ ngx_cmp_sockaddr(struct sockaddr *sa1, socklen_t slen1,
             return NGX_DECLINED;
         }
 
-        if (ngx_memcmp(&sin61->sin6_addr, &sin62->sin6_addr, 16) != 0) {
+        if (memcmp(&sin61->sin6_addr, &sin62->sin6_addr, 16) != 0) {
             return NGX_DECLINED;
         }
 
@@ -1373,7 +1373,7 @@ ngx_cmp_sockaddr(struct sockaddr *sa1, socklen_t slen1,
             len = sizeof(saun1->sun_path);
         }
 
-        if (ngx_memcmp(&saun1->sun_path, &saun2->sun_path, len) != 0) {
+        if (memcmp(&saun1->sun_path, &saun2->sun_path, len) != 0) {
             return NGX_DECLINED;
         }
 
diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
index c76c1785..64a42a49 100644
--- a/src/core/ngx_resolver.c
+++ b/src/core/ngx_resolver.c
@@ -3557,7 +3557,7 @@ ngx_resolver_lookup_addr6(ngx_resolver_t *r, struct in6_addr *addr,
 
         rn = ngx_resolver_node(node);
 
-        rc = ngx_memcmp(addr, &rn->addr6, 16);
+        rc = memcmp(addr, &rn->addr6, 16);
 
         if (rc == 0) {
             return rn;
@@ -3639,7 +3639,7 @@ ngx_resolver_rbtree_insert_addr6_value(ngx_rbtree_node_t *temp,
             rn = ngx_resolver_node(node);
             rn_temp = ngx_resolver_node(temp);
 
-            p = (ngx_memcmp(&rn->addr6, &rn_temp->addr6, 16)
+            p = (memcmp(&rn->addr6, &rn_temp->addr6, 16)
                  < 0) ? &temp->left : &temp->right;
         }
 
diff --git a/src/core/ngx_string.c b/src/core/ngx_string.c
index 98f270ac..93d5a5c6 100644
--- a/src/core/ngx_string.c
+++ b/src/core/ngx_string.c
@@ -882,7 +882,7 @@ ngx_memn2cmp(u_char *s1, u_char *s2, size_t n1, size_t n2)
         z = 1;
     }
 
-    m = ngx_memcmp(s1, s2, n);
+    m = memcmp(s1, s2, n);
 
     if (m || n1 == n2) {
         return m;
@@ -1980,7 +1980,7 @@ ngx_str_rbtree_insert_value(ngx_rbtree_node_t *temp,
             p = (n->str.len < t->str.len) ? &temp->left : &temp->right;
 
         } else {
-            p = (ngx_memcmp(n->str.data, t->str.data, n->str.len) < 0)
+            p = (memcmp(n->str.data, t->str.data, n->str.len) < 0)
                  ? &temp->left : &temp->right;
         }
 
@@ -2023,7 +2023,7 @@ ngx_str_rbtree_lookup(ngx_rbtree_t *rbtree, ngx_str_t *val, uint32_t hash)
             continue;
         }
 
-        rc = ngx_memcmp(val->data, n->str.data, val->len);
+        rc = memcmp(val->data, n->str.data, val->len);
 
         if (rc < 0) {
             node = node->left;
diff --git a/src/core/ngx_string.h b/src/core/ngx_string.h
index 0fb9be72..cfed4ab9 100644
--- a/src/core/ngx_string.h
+++ b/src/core/ngx_string.h
@@ -144,10 +144,6 @@ ngx_copy(u_char *dst, u_char *src, size_t len)
 #define ngx_movemem(dst, src, n)   (((u_char *) memmove(dst, src, n)) + (n))
 
 
-/* msvc and icc7 compile memcmp() to the inline loop */
-#define ngx_memcmp(s1, s2, n)  memcmp((const char *) s1, (const char *) s2, n)
-
-
 u_char *ngx_cpystrn(u_char *dst, u_char *src, size_t n);
 u_char *ngx_pstrdup(ngx_pool_t *pool, ngx_str_t *src);
 u_char * ngx_cdecl ngx_sprintf(u_char *buf, const char *fmt, ...);
diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
index 3be64b6c..d92dae48 100644
--- a/src/event/ngx_event_openssl.c
+++ b/src/event/ngx_event_openssl.c
@@ -4516,7 +4516,7 @@ ngx_ssl_ticket_key_callback(ngx_ssl_conn_t *ssl_conn,
         /* decrypt session ticket */
 
         for (i = 0; i < keys->nelts; i++) {
-            if (ngx_memcmp(name, key[i].name, 16) == 0) {
+            if (memcmp(name, key[i].name, 16) == 0) {
                 goto found;
             }
         }
diff --git a/src/http/modules/ngx_http_geo_module.c b/src/http/modules/ngx_http_geo_module.c
index ef4e9b84..1f243132 100644
--- a/src/http/modules/ngx_http_geo_module.c
+++ b/src/http/modules/ngx_http_geo_module.c
@@ -1497,7 +1497,7 @@ ngx_http_geo_include_binary_base(ngx_conf_t *cf, ngx_http_geo_conf_ctx_t *ctx,
 
     header = (ngx_http_geo_header_t *) base;
 
-    if (size < 16 || ngx_memcmp(&ngx_http_geo_header, header, 12) != 0) {
+    if (size < 16 || memcmp(&ngx_http_geo_header, header, 12) != 0) {
         ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
              "incompatible binary geo range base \"%s\"", name->data);
         goto failed;
diff --git a/src/http/modules/ngx_http_secure_link_module.c b/src/http/modules/ngx_http_secure_link_module.c
index 4d4ce6af..23f3b77c 100644
--- a/src/http/modules/ngx_http_secure_link_module.c
+++ b/src/http/modules/ngx_http_secure_link_module.c
@@ -175,7 +175,7 @@ ngx_http_secure_link_variable(ngx_http_request_t *r,
     ngx_md5_update(&md5, val.data, val.len);
     ngx_md5_final(md5_buf, &md5);
 
-    if (ngx_memcmp(hash_buf, md5_buf, 16) != 0) {
+    if (memcmp(hash_buf, md5_buf, 16) != 0) {
         goto not_found;
     }
 
diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
index 28f7d99b..1a1bc75b 100644
--- a/src/http/ngx_http_core_module.c
+++ b/src/http/ngx_http_core_module.c
@@ -2055,7 +2055,7 @@ ngx_http_gzip_ok(ngx_http_request_t *r)
      *   Opera:   "gzip, deflate"
      */
 
-    if (ngx_memcmp(ae->value.data, "gzip,", 5) != 0
+    if (memcmp(ae->value.data, "gzip,", 5) != 0
         && ngx_http_gzip_accept_encoding(&ae->value) != NGX_OK)
     {
         return NGX_DECLINED;
@@ -2685,7 +2685,7 @@ ngx_http_set_disable_symlinks(ngx_http_request_t *r,
 
     if (from.len == 0
         || from.len > path->len
-        || ngx_memcmp(path->data, from.data, from.len) != 0)
+        || memcmp(path->data, from.data, from.len) != 0)
     {
         return NGX_OK;
     }
diff --git a/src/http/ngx_http_file_cache.c b/src/http/ngx_http_file_cache.c
index 4d2f6c42..0503d9aa 100644
--- a/src/http/ngx_http_file_cache.c
+++ b/src/http/ngx_http_file_cache.c
@@ -567,7 +567,7 @@ ngx_http_file_cache_read(ngx_http_request_t *r, ngx_http_cache_t *c)
 
     key = c->keys.elts;
     for (i = 0; i < c->keys.nelts; i++) {
-        if (ngx_memcmp(p, key[i].data, key[i].len) != 0) {
+        if (memcmp(p, key[i].data, key[i].len) != 0) {
             ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
                           "cache file \"%s\" has md5 collision",
                           c->file.name.data);
@@ -594,7 +594,7 @@ ngx_http_file_cache_read(ngx_http_request_t *r, ngx_http_cache_t *c)
     if (h->vary_len) {
         ngx_http_file_cache_vary(r, h->vary, h->vary_len, c->variant);
 
-        if (ngx_memcmp(c->variant, h->variant, NGX_HTTP_CACHE_KEY_LEN) != 0) {
+        if (memcmp(c->variant, h->variant, NGX_HTTP_CACHE_KEY_LEN) != 0) {
             ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
                            "http file cache vary mismatch");
             return ngx_http_file_cache_reopen(r, c);
@@ -995,7 +995,7 @@ ngx_http_file_cache_lookup(ngx_http_file_cache_t *cache, u_char *key)
 
         fcn = (ngx_http_file_cache_node_t *) node;
 
-        rc = ngx_memcmp(&key[sizeof(ngx_rbtree_key_t)], fcn->key,
+        rc = memcmp(&key[sizeof(ngx_rbtree_key_t)], fcn->key,
                         NGX_HTTP_CACHE_KEY_LEN - sizeof(ngx_rbtree_key_t));
 
         if (rc == 0) {
@@ -1033,7 +1033,7 @@ ngx_http_file_cache_rbtree_insert_value(ngx_rbtree_node_t *temp,
             cn = (ngx_http_file_cache_node_t *) node;
             cnt = (ngx_http_file_cache_node_t *) temp;
 
-            p = (ngx_memcmp(cn->key, cnt->key,
+            p = (memcmp(cn->key, cnt->key,
                             NGX_HTTP_CACHE_KEY_LEN - sizeof(ngx_rbtree_key_t))
                  < 0)
                     ? &temp->left : &temp->right;
@@ -1315,7 +1315,7 @@ ngx_http_file_cache_update_variant(ngx_http_request_t *r, ngx_http_cache_t *c)
     }
 
     if (c->vary.len
-        && ngx_memcmp(c->variant, c->key, NGX_HTTP_CACHE_KEY_LEN) == 0)
+        && memcmp(c->variant, c->key, NGX_HTTP_CACHE_KEY_LEN) == 0)
     {
         return NGX_OK;
     }
diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
index 131a2c83..dce37a26 100644
--- a/src/http/ngx_http_request.c
+++ b/src/http/ngx_http_request.c
@@ -253,7 +253,7 @@ ngx_http_init_connection(ngx_connection_t *c)
             /* the last address is "*" */
 
             for (i = 0; i < port->naddrs - 1; i++) {
-                if (ngx_memcmp(&addr6[i].addr6, &sin6->sin6_addr, 16) == 0) {
+                if (memcmp(&addr6[i].addr6, &sin6->sin6_addr, 16) == 0) {
                     break;
                 }
             }
diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
index 0e45a7b2..2d54b5ec 100644
--- a/src/http/v2/ngx_http_v2.c
+++ b/src/http/v2/ngx_http_v2.c
@@ -877,7 +877,7 @@ ngx_http_v2_state_preface(ngx_http_v2_connection_t *h2c, u_char *pos,
         return ngx_http_v2_state_save(h2c, pos, end, ngx_http_v2_state_preface);
     }
 
-    if (ngx_memcmp(pos, preface, sizeof(preface) - 1) != 0) {
+    if (memcmp(pos, preface, sizeof(preface) - 1) != 0) {
         ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
                       "invalid connection preface");
 
@@ -899,7 +899,7 @@ ngx_http_v2_state_preface_end(ngx_http_v2_connection_t *h2c, u_char *pos,
                                       ngx_http_v2_state_preface_end);
     }
 
-    if (ngx_memcmp(pos, preface, sizeof(preface) - 1) != 0) {
+    if (memcmp(pos, preface, sizeof(preface) - 1) != 0) {
         ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
                       "invalid connection preface");
 
@@ -1838,7 +1838,7 @@ ngx_http_v2_state_process_header(ngx_http_v2_connection_t *h2c, u_char *pos,
     }
 
     if (header->name.len == cookie.len
-        && ngx_memcmp(header->name.data, cookie.data, cookie.len) == 0)
+        && memcmp(header->name.data, cookie.data, cookie.len) == 0)
     {
         if (ngx_http_v2_cookie(r, header) != NGX_OK) {
             return ngx_http_v2_connection_error(h2c,
@@ -3496,33 +3496,25 @@ ngx_http_v2_pseudo_header(ngx_http_request_t *r, ngx_http_v2_header_t *header)
 
     switch (header->name.len) {
     case 4:
-        if (ngx_memcmp(header->name.data, "path", sizeof("path") - 1)
-            == 0)
-        {
+        if (memcmp(header->name.data, "path", sizeof("path") - 1) == 0) {
             return ngx_http_v2_parse_path(r, &header->value);
         }
 
         break;
 
     case 6:
-        if (ngx_memcmp(header->name.data, "method", sizeof("method") - 1)
-            == 0)
-        {
+        if (memcmp(header->name.data, "method", sizeof("method") - 1) == 0) {
             return ngx_http_v2_parse_method(r, &header->value);
         }
 
-        if (ngx_memcmp(header->name.data, "scheme", sizeof("scheme") - 1)
-            == 0)
-        {
+        if (memcmp(header->name.data, "scheme", sizeof("scheme") - 1) == 0) {
             return ngx_http_v2_parse_scheme(r, &header->value);
         }
 
         break;
 
     case 9:
-        if (ngx_memcmp(header->name.data, "authority", sizeof("authority") - 1)
-            == 0)
-        {
+        if (memcmp(header->name.data, "authority", sizeof("authority") - 1) == 0) {
             return ngx_http_v2_parse_authority(r, &header->value);
         }
 
diff --git a/src/mail/ngx_mail_handler.c b/src/mail/ngx_mail_handler.c
index 246ba97c..0d408025 100644
--- a/src/mail/ngx_mail_handler.c
+++ b/src/mail/ngx_mail_handler.c
@@ -76,7 +76,7 @@ ngx_mail_init_connection(ngx_connection_t *c)
             /* the last address is "*" */
 
             for (i = 0; i < port->naddrs - 1; i++) {
-                if (ngx_memcmp(&addr6[i].addr6, &sin6->sin6_addr, 16) == 0) {
+                if (memcmp(&addr6[i].addr6, &sin6->sin6_addr, 16) == 0) {
                     break;
                 }
             }
diff --git a/src/stream/ngx_stream_geo_module.c b/src/stream/ngx_stream_geo_module.c
index 4b4cad8f..677f0086 100644
--- a/src/stream/ngx_stream_geo_module.c
+++ b/src/stream/ngx_stream_geo_module.c
@@ -1423,7 +1423,7 @@ ngx_stream_geo_include_binary_base(ngx_conf_t *cf,
 
     header = (ngx_stream_geo_header_t *) base;
 
-    if (size < 16 || ngx_memcmp(&ngx_stream_geo_header, header, 12) != 0) {
+    if (size < 16 || memcmp(&ngx_stream_geo_header, header, 12) != 0) {
         ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
              "incompatible binary geo range base \"%s\"", name->data);
         goto failed;
diff --git a/src/stream/ngx_stream_handler.c b/src/stream/ngx_stream_handler.c
index 669b6a18..7b6f5f38 100644
--- a/src/stream/ngx_stream_handler.c
+++ b/src/stream/ngx_stream_handler.c
@@ -70,7 +70,7 @@ ngx_stream_init_connection(ngx_connection_t *c)
             /* the last address is "*" */
 
             for (i = 0; i < port->naddrs - 1; i++) {
-                if (ngx_memcmp(&addr6[i].addr6, &sin6->sin6_addr, 16) == 0) {
+                if (memcmp(&addr6[i].addr6, &sin6->sin6_addr, 16) == 0) {
                     break;
                 }
             }
-- 
2.37.2



More information about the nginx-devel mailing list