test557 fails when ubsan is enabled

[jay]

I did this

3558e14 2017-05-29

When the undefined behavior sanitizer is enabled (CFLAGS and LDFLAGS -fsanitize=address,undefined -fno-sanitize-recover) test 557 will not complete due to undefined behavior of int left shift 1 << 31, refer to lib557.c:1620.

Output

gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609

=== Start of file curlverout.log
 curl 7.54.1-DEV (x86_64-pc-linux-gnu) libcurl/7.54.1-DEV OpenSSL/1.0.2l zlib/1.2.8 nghttp2/1.23.0 librtmp/2.3
 Release-Date: [unreleased]
 Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp smb smbs smtp smtps telnet tftp 
 Features: Debug TrackMemory IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy 
=== End of file curlverout.log
=== Start of file stderr557
 URL: nothing
 lib557.c:1625:45: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
=== End of file stderr557

Solution

I think the most correct one would be use INT_MIN since afaics the purpose would be the same?

diff --git a/tests/libtest/lib557.c b/tests/libtest/lib557.c
index ca3118a..8004548 100644
--- a/tests/libtest/lib557.c
+++ b/tests/libtest/lib557.c
@@ -1621,8 +1621,8 @@ static int test_float_formatting(void)
                  123456789123456789123456789.2987654);
   errors += strlen_check(buf, 325);
 
-  /* 1<<31 turns negative (-2147483648) when used signed */
-  curl_msnprintf(buf, sizeof(buf), "%*f", (1<<31), 9.1);
+  /* check negative when used signed */
+  curl_msnprintf(buf, sizeof(buf), "%*f", INT_MIN, 9.1);
   errors += string_check(buf, "9.100000");
 
   /* curl_msnprintf() limits a single float output to 325 bytes maximum

[bagder]

Yes agreed, that should be the better way to do the same thing! 👍

[MarcelRaad]

Maybe also explicitly include limits.h? It's probably implicitly included through test.h somewhere?

This is also a -Wshift-sign-overflow warning. Unfortunately it's too easy to miss them as it's not enabled by default and --enable-werror doesn't work for the test directory.

Do you think it would make sense to enable -Wshift-sign-overflow?

[jay]

I included limits.h, thanks. I don't know why we're guarding it with HAVE_LIMITS_H, I don't know how it would be possible to build curl without it. Also it is required by the C standard even when the standard library doesn't exist (freestanding).

I don't see any problem with using -Wshift-sign-overflow, have you tried it? Mine doesn't have it. gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609. You'd probably need some checking in m4/curl-compilers.m4. Also I'd submit it as PR to gather feedback, maybe there is some non-obvious reason not to use it.

[MarcelRaad]

@jay Thanks, I'll do that! I just noticed that the warning doesn't exist in GCC, I was using clang.