implicit conversion reports from `~FLAG` / `~MASK` macros (suggest `U` suffix)

[xmoezzz]

I did this

Hi,
I'm using curl 8.16.0, when building with UBSan I see the following implicit conversion reports.

call stacks

/root/build/curl-8.16.0/lib/urlapi.c:1548:14: runtime error: implicit conversion from type 'int' of value -65 (32-bit, signed) to type 'unsigned int' changed the value to 4294967231 (32-bit, unsigned)
    #0 0x69f8a2 in curl_url_get /root/build/curl-8.16.0/lib/urlapi.c:1548:14
    #1 0x4d0726 in url_proto_and_rewrite /root/build/curl-8.16.0/src/config2setopts.c:139:9
    #2 0x4ca8e6 in config2setopts /root/build/curl-8.16.0/src/config2setopts.c:799:21
    #3 0x535047 in single_transfer /root/build/curl-8.16.0/src/tool_operate.c:1310:14
    #4 0x52ffcb in transfer_per_config /root/build/curl-8.16.0/src/tool_operate.c:2058:14
    #5 0x52ee41 in create_transfer /root/build/curl-8.16.0/src/tool_operate.c:2077:14
    #6 0x528f45 in serial_transfers /root/build/curl-8.16.0/src/tool_operate.c:1834:12
    #7 0x527743 in run_all_transfers /root/build/curl-8.16.0/src/tool_operate.c:2101:16
    #8 0x527235 in operate /root/build/curl-8.16.0/src/tool_operate.c:2240:22
    #9 0x52543a in main /root/build/curl-8.16.0/src/tool_main.c:199:14
    #10 0x7382b1bd8c86 in __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:310
    #11 0x41c389 in _start (/work/build/curl-8.16.0/obj-gcov2/src/curl+0x41c389)

/root/build/curl-8.16.0/lib/urlapi.c:1575:14: runtime error: implicit conversion from type 'int' of value -65 (32-bit, signed) to type 'unsigned int' changed the value to 4294967231 (32-bit, unsigned)
    #0 0x69feb5 in curl_url_get /root/build/curl-8.16.0/lib/urlapi.c:1575:14
    #1 0x67cec2 in parseurlandfillconn /root/build/curl-8.16.0/lib/url.c:1996:8
    #2 0x66e50c in create_conn /root/build/curl-8.16.0/lib/url.c:3472:12
    #3 0x66dd6e in Curl_connect /root/build/curl-8.16.0/lib/url.c:3862:12
    #4 0x5e47a2 in state_connect /root/build/curl-8.16.0/lib/multi.c:2286:21
    #5 0x5d2bdc in multi_runsingle /root/build/curl-8.16.0/lib/multi.c:2414:12
    #6 0x5d132a in curl_multi_perform /root/build/curl-8.16.0/lib/multi.c:2771:18
    #7 0x56fd23 in easy_transfer /root/build/curl-8.16.0/lib/easy.c:720:15
    #8 0x56958e in easy_perform /root/build/curl-8.16.0/lib/easy.c:828:42
    #9 0x568a16 in curl_easy_perform /root/build/curl-8.16.0/lib/easy.c:846:10
    #10 0x5291e2 in serial_transfers /root/build/curl-8.16.0/src/tool_operate.c:1878:18
    #11 0x527743 in run_all_transfers /root/build/curl-8.16.0/src/tool_operate.c:2101:16
    #12 0x527235 in operate /root/build/curl-8.16.0/src/tool_operate.c:2240:22
    #13 0x52543a in main /root/build/curl-8.16.0/src/tool_main.c:199:14
    #14 0x7382b1bd8c86 in __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:310
    #15 0x41c389 in _start (/work/build/curl-8.16.0/obj-gcov2/src/curl+0x41c389)

/root/build/curl-8.16.0/lib/uint-spbset.c:99:32: runtime error: implicit conversion from type 'int' of value -256 (32-bit, signed) to type 'unsigned int' changed the value to 4294967040 (32-bit, unsigned)
    #0 0x65ac67 in uint_spbset_get_chunk /root/build/curl-8.16.0/lib/uint-spbset.c:99:32
    #1 0x65a8a2 in Curl_uint_spbset_add /root/build/curl-8.16.0/lib/uint-spbset.c:143:11
    #2 0x5c5e16 in Curl_attach_connection /root/build/curl-8.16.0/lib/multi.c:901:3
    #3 0x67375d in create_conn /root/build/curl-8.16.0/lib/url.c:3754:7
    #4 0x66dd6e in Curl_connect /root/build/curl-8.16.0/lib/url.c:3862:12
    #5 0x5e47a2 in state_connect /root/build/curl-8.16.0/lib/multi.c:2286:21
    #6 0x5d2bdc in multi_runsingle /root/build/curl-8.16.0/lib/multi.c:2414:12
    #7 0x5d132a in curl_multi_perform /root/build/curl-8.16.0/lib/multi.c:2771:18
    #8 0x56fd23 in easy_transfer /root/build/curl-8.16.0/lib/easy.c:720:15
    #9 0x56958e in easy_perform /root/build/curl-8.16.0/lib/easy.c:828:42
    #10 0x568a16 in curl_easy_perform /root/build/curl-8.16.0/lib/easy.c:846:10
    #11 0x5291e2 in serial_transfers /root/build/curl-8.16.0/src/tool_operate.c:1878:18
    #12 0x527743 in run_all_transfers /root/build/curl-8.16.0/src/tool_operate.c:2101:16
    #13 0x527235 in operate /root/build/curl-8.16.0/src/tool_operate.c:2240:22
    #14 0x52543a in main /root/build/curl-8.16.0/src/tool_main.c:199:14
    #15 0x7382b1bd8c86 in __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:310
    #16 0x41c389 in _start (/work/build/curl-8.16.0/obj-gcov2/src/curl+0x41c389)

I expected the following

I guess we can make the macro constants unsigned (adding U) to suppress these reports without changing bitmask semantics, e.g.:

#define CURLU_URLDECODE (1U<<6)
/* and for masks */
#define CURL_UINT_SPBSET_CH_MASK ((CURL_UINT_SPBSET_CH_SLOTS * 64U) - 1U)

I also ran ast-grep to locate similar sites:

sg -p '$X &= ~$Y;' -l c .

This found additional &= ~MACRO usages; would it be acceptable to address similar cases by making the relevant mask macros unsigned via U suffixes?

related PR

PR #11814

curl/libcurl version

curl 8.16.0

operating system

Fedora Linux 42

[dfandrich]

Are you seeing the same issue on the latest release, or more preferably, the git master branch? We fix these kind of things regularly when they come up there's a good chance it's already gone (that file has seen 21 updates since 8.16.0).

[xmoezzz]

Are you seeing the same issue on the latest release, or more preferably, the
git master branch? We fix these kind of things regularly when they come up
there's a good chance it's already gone (that file has seen 21 updates since
8.16.0).

I checked the latest master. The CURLU_URLDECODE macro is still affected.
If this kind of change is acceptable, may I open a PR to make this kind of mask macros explicitly unsigned?

Thanks!

[bagder]

I'm hesitant to modify CURLU_URLDECODE like that since it is public and may very well be used in signed contexts already.

The mentioned uint-spbset.c code seems to already be different in master.

I instead propose a fix without touching the define:

diff --git a/lib/urlapi.c b/lib/urlapi.c
index c59f239756..3c84fcba7f 100644
--- a/lib/urlapi.c
+++ b/lib/urlapi.c
@@ -1553,11 +1553,11 @@ CURLUcode curl_url_get(const CURLU *u, CURLUPart what,
 
   switch(what) {
   case CURLUPART_SCHEME:
     ptr = u->scheme;
     ifmissing = CURLUE_NO_SCHEME;
-    flags &= ~CURLU_URLDECODE; /* never for schemes */
+    flags &= ~(unsigned int)CURLU_URLDECODE; /* never for schemes */
     if((flags & CURLU_NO_GUESS_SCHEME) && u->guessed_scheme)
       return CURLUE_NO_SCHEME;
     break;
   case CURLUPART_USER:
     ptr = u->user;
@@ -1580,11 +1580,11 @@ CURLUcode curl_url_get(const CURLU *u, CURLUPart what,
     ifmissing = CURLUE_NO_ZONEID;
     break;
   case CURLUPART_PORT:
     ptr = u->port;
     ifmissing = CURLUE_NO_PORT;
-    flags &= ~CURLU_URLDECODE; /* never for port */
+    flags &= ~(unsigned int)CURLU_URLDECODE; /* never for port */
     if(!ptr && (flags & CURLU_DEFAULT_PORT) && u->scheme) {
       /* there is no stored port number, but asked to deliver
          a default one for the scheme */
       const struct Curl_scheme *h = Curl_get_scheme(u->scheme);
       if(h) {

[bagder]

@vszakats can you understand why @xmoezzz gets undefined sanitizer hits on these when our CI jobs don't?

[vszakats]

@vszakats can you understand why @xmoezzz gets undefined sanitizer hits on these when our CI jobs don't?

I was wondering about it too. I haven't taken a close look at these jobs before.
Will do it now.

@xmoezzz If you don't mind posting the compiler command-line that successfully triggered
the warnings for your, it'd be helpful to see it, and compare with curl's CI.

[xmoezzz]

@vszakats can you understand why @xmoezzz gets undefined sanitizer hits on these when our CI jobs don't?

I was wondering about it too. I haven't taken a close look at these jobs before. Will do it now.

@xmoezzz If you don't mind posting the compiler command-line that successfully triggered the warnings for your, it'd be helpful to see it, and compare with curl's CI.

I think -fsanitize=undefined,integer,bounds can reproduce this report.

[vszakats]

@xmoezzz Thank you. I managed to trigger it with this option in CI, at least for urlapi.c line 1558. integer was the missing bit, I think, but still need to reduce changes to tell for sure. Unfortunately integer makes tests execute very slow, reaching into test 0350 in about 18 minutes. May take an hour or 2 to run them all: https://github.com/curl/curl/actions/runs/22606123719/job/65498425580?pr=20785#step:48:144

/home/runner/work/curl/curl/lib/urlapi.c:1558:14: runtime error: implicit conversion
 from type 'int' of value -65 (32-bit, signed) to type 'unsigned int'
 changed the value to 4294967231 (32-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/runner/work/curl/curl/lib/urlapi.c:1558:14 

I'll continue experimenting.