[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