New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib: silence -Wsign-conversion
, tidy-ups, fixes
#13481
Conversation
@@ -409,7 +409,7 @@ static CURLcode getalnum(const char **ptr, char *alpnbuf, size_t buflen) | |||
protop = p; | |||
while(*p && !ISBLANK(*p) && (*p != ';') && (*p != '=')) | |||
p++; | |||
len = p - protop; | |||
len = (size_t)(p - protop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this kind of typecast is perhaps the biggest sign that we should not use this warning option. The diff between two char *
should be possible to store in a size_t
without a typecast.
Or put another way: do these typecasts really help? They will make writing new code a little more awkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It pinpoints all the places where the result might be negative,
but the code assumes positive. For the strchr()
pattern and manual
versions of it, this isn't really useful, but may be in the rest.
I assumed that all existing code is correct, but this isn't readily obvious
while reviewing the 442 cases.
A cast adds a signal that sign conversions were consious decisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It pinpoints all the places where the result might be negative, but the code assumes positive
The code does not assume this, it knows. The logic is quite easy to follow.
In this and similar cases, this typecast does not add anything at all to a reader. In fact, it is mostly confusing to me since a pointer-delta should already be possible to store in a `size_t' just fine. Should it not?
Have you detected and corrected any actual errors with this exercise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some, that were obvious and low-hanging, yes.
But, I try not to touch the actual logic, first because it'd take a very long time to explore each rabbit hole, and second it'd make the patch confusing IMO. Also, it's highly likely the code is correct unless proven otherwise.
What I find that it's difficult to deduct from local contexts that the code is handling the implicit sign conversions conciously.
Am I mistaken that e.g. a 100 - 1000 pointer delta would result in an underflow and make the result a very high number when handled as an unsigned size_t
?
The other thing driving this is that this is one the few warnings actually recommended by OpenSSF. Assuming they know what they are saying, this might actually be useful, though of course there is nothing saying we must follow their recommendations.
Complete list of warnings addressed in #13469 #13470 #13472 and #13481 (this one): log
|
This comment was marked as outdated.
This comment was marked as outdated.
-Wsign-conversion
-Wsign-conversion
, tidy-ups, fixes
``` altsvc.c: In function ‘altsvc_add’: altsvc.c:192:18: warning: conversion to ‘int’ from ‘unsigned int’ may change the sign of the result [-Wsign-conversion] 192 | as->prio = prio; | ^~~~ altsvc.c: In function ‘getalnum’: altsvc.c:412:9: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘long int’ may change the sign of the result [-Wsign-conversion] 412 | len = p - protop; | ^ altsvc.c: In function ‘altsvc_debugtime’: altsvc.c:465:25: warning: conversion to ‘long unsigned int’ from ‘long int’ may change the sign of the result [-Wsign-conversion] 465 | unsigned long val = strtol(timestr, NULL, 10); | ^~~~~~ altsvc.c: In function ‘Curl_altsvc_parse’: altsvc.c:549:19: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘long int’ may change the sign of the result [-Wsign-conversion] 549 | len = p - hostp; | ^ altsvc.c:627:24: warning: conversion to ‘time_t’ {aka ‘long int’} from ‘long unsigned int’ may change the sign of the result [-Wsign-conversion] 627 | maxage = num; | ^~~ altsvc.c: In function ‘Curl_altsvc_lookup’: altsvc.c:699:18: warning: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion] 699 | (versions & as->dst.alpnid)) { | ^ ``` Ref: https://github.com/curl/curl/actions/runs/8819398779/job/24210519501#step:30:25
win32: ``` 177 | vals.keepalivetime = optval; | ~ ^~~~~~ /home/runner/work/curl/curl/curl/lib/cf-socket.c:180:30: error: implicit conversion changes signedness: 'int' to 'u_long' (aka 'unsigned long') [-Werror,-Wsign-conversion] 180 | vals.keepaliveinterval = optval; | ~ ^~~~~~ ``` Ref: https://github.com/curl/curl/actions/runs/8862225447/job/24334941063?pr=13489#step:3:6125
Linux 32-bit gcc: ``` cookie.c: In function ‘cookie_hash_domain’: cookie.c:266:7: error: conversion to ‘size_t’ {aka ‘unsigned int’} from ‘unsigned int’ may change the sign of the result [-Werror=sign-conversion] 266 | h ^= (size_t)Curl_raw_toupper(*domain++); | ^~ ``` Ref: https://github.com/curl/curl/actions/runs/8859933866/job/24330206389?pr=13489#step:6:90 temp: ``` cookie.c:266:11: error: conversion to ‘size_t’ {aka ‘unsigned int’} from ‘unsigned int’ may change the sign of the result [-Werror=sign-conversion] 266 | h = h ^ (size_t)Curl_raw_toupper(*domain++); | ^ ``` Ref: https://github.com/curl/curl/actions/runs/8861448062/job/24333315928?pr=13489#step:6:79
temp: ``` /__w/curl/curl/lib/krb5.c:528:22: error: conversion to 'uint32_t {aka unsigned int}' from 'int' may change the sign of the result [-Werror=sign-conversion] len = (int)ntohl((unsigned long)len); ^ /__w/curl/curl/lib/krb5.c: In function 'do_sec_send': /__w/curl/curl/lib/krb5.c:651:30: error: conversion to 'uint32_t {aka unsigned int}' from 'int' may change the sign of the result [-Werror=sign-conversion] htonl_bytes = (int)htonl((unsigned long)bytes); ^ ``` Ref: https://github.com/curl/curl/actions/runs/8862697118/job/24335936723?pr=13489#step:7:132
``` sws.c:1308:21: warning: implicit conversion changes signedness: 'enum (unnamed enum at sws.c:71:8)' to 'int' [-Wsign-conversion] serverfd = socket(socket_domain, SOCK_STREAM, 0); ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../lib/memdebug.h:140:18: note: expanded from macro 'socket' curl_dbg_socket(domain, type, protocol, __LINE__, __FILE__) ~~~~~~~~~~~~~~~ ^~~~~~ clang -DHAVE_CONFIG_H -I../../include -I../../lib -I../../lib -isystem /home/runner/bearssl/include -Qunused-arguments -g -O0 -pedantic -Wall -Wextra -Wpointer-arith -Wwrite-strings -Wshadow -Winline -Wnested-externs -Wmissing-declarations -Wmissing-prototypes -Wno-long-long -Wfloat-equal -Wsign-compare -Wno-multichar -Wundef -Wno-format-nonliteral -Wendif-labels -Wstrict-prototypes -Wdeclaration-after-statement -Wcast-align -Wno-system-headers -Wshorten-64-to-32 -Wunused -Waddress -Wattributes -Wbad-function-cast -Wconversion -Wdiv-by-zero -Wformat-security -Wempty-body -Wmissing-field-initializers -Wmissing-noreturn -Wold-style-definition -Wredundant-decls -Wtype-limits -Wunreachable-code -Wunused-parameter -Wignored-qualifiers -Wvla -Wsign-conversion -Wshift-sign-overflow -Wlanguage-extension-token -Wformat=2 -Wenum-conversion -Wsometimes-uninitialized -Wmissing-variable-declarations -Wheader-guard -Wunused-const-variable -Wpragmas -Wunreachable-code-break -Wdouble-promotion -Wcomma -Wassign-enum -Wextra-semi-stmt -Wimplicit-fallthrough -Wno-pointer-bool-conversion -MT ../../lib/tftpd-nonblock.o -MD -MP -MF ../../lib/.deps/tftpd-nonblock.Tpo -c -o ../../lib/tftpd-nonblock.o `test -f '../../lib/nonblock.c' || echo './'`../../lib/nonblock.c sws.c:2136:17: warning: implicit conversion changes signedness: 'enum (unnamed enum at sws.c:71:8)' to 'int' [-Wsign-conversion] sock = socket(socket_domain, SOCK_STREAM, 0); ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../lib/memdebug.h:140:18: note: expanded from macro 'socket' curl_dbg_socket(domain, type, protocol, __LINE__, __FILE__) ~~~~~~~~~~~~~~~ ^~~~~~ 2 warnings generated. ``` Ref: https://github.com/curl/curl/actions/runs/8866821106/job/24344717997#step:34:174
Seen on Alpine MUSL. ``` multi.c:1201:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion] 1201 | FD_SET(ps.sockets[i], read_fd_set); | ^~~~~~ multi.c:1201:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion] multi.c:1203:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion] 1203 | FD_SET(ps.sockets[i], write_fd_set); | ^~~~~~ multi.c:1203:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion] ``` Ref: https://github.com/curl/curl/actions/runs/8867959370/job/24347027402?pr=13489#step:31:373
``` ../../lib/tftp.c: In function 'tftp_send_first': ../../lib/tftp.c:531:46: error: conversion to 'socklen_t' {aka 'int'} from 'unsigned int' may change the sign of the result [-Werror=sign-conversion] 531 | data->conn->remote_addr->addrlen); | ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~ ../../lib/tftp.c: In function 'tftp_connect': ../../lib/tftp.c:1036:36: error: conversion to 'socklen_t' {aka 'int'} from 'unsigned int' may change the sign of the result [-Werror=sign-conversion] 1036 | conn->remote_addr->addrlen); | ~~~~~~~~~~~~~~~~~^~~~~~~~~ ``` Ref: https://ci.appveyor.com/project/curlorg/curl/builds/49706723/job/pxrgsssfmunjk2o9#L1212
Closing in favour of #13501 and cherry-picking from there. It has the same commits as this one. |
lib
building without signedness warnings after this PR.Also:
sin_family
/sin6_family
asshort
.Curl_hash_add
.Cherry-picked from #13489
Follow-up to 3829759 #12489
Closes #13481