Skip to content
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

autotools: stop setting -std=gnu89 with --enable-warnings #12346

Closed
wants to merge 11 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Nov 16, 2023

Do not alter the C standard when building with --enable-warnings when
building with gcc.

On one hand this alters warning results compared to a default build.
On the other, it may produce different binaries, which is unexpected.

Also fix new warnings that appeared after removing -std=gnu89:

  • include: fix public curl headers to use the correct printf mask for
    CURL_FORMAT_CURL_OFF_T and CURL_FORMAT_CURL_OFF_TU with mingw-w64
    and Visual Studio 2013 and newer. This fixes the printf mask warnings
    in examples and tests. E.g. [1]

  • conncache: fix printf format string [2].

  • http2: fix potential null pointer dereference [3].
    (seen on Slackware with gcc 11.)

  • libssh: fix printf format string in SFTP code [4].
    Also make MSVC builds compatible with old CRT versions.

  • libssh2: fix printf format string in SFTP code for MSVC.
    Applying the same fix as for libssh above.

  • unit1395: fix argument is null and related issues [5]:

    • stop calling strcmp() with NULL to avoid undefined behaviour.
    • fix checking results if some of them were NULL.
    • do not pass NULL to printf %s.
  • ci: keep a build job with -std=gnu89 to continue testing for
    C89-compliance. We can apply this to other gcc jobs as needed.
    Ref: b23ce2c (2022-09-23) curl-compilers.m4: for gcc + want warnings, set gnu89 standard #9542

[1] https://dev.azure.com/daniel0244/curl/_build/results?buildId=18581&view=logs&jobId=ccf9cc6d-2ef1-5cf2-2c09-30f0c14f923b
[2] https://github.com/curl/curl/actions/runs/6896854263/job/18763831142?pr=12346#step:6:67
[3] https://github.com/curl/curl/actions/runs/6896854253/job/18763839238?pr=12346#step:30:214
[4] https://github.com/curl/curl/actions/runs/6896854253/job/18763838007?pr=12346#step:29:895
[5] https://github.com/curl/curl/actions/runs/6896854253/job/18763836775?pr=12346#step:33:1689

Closes #12346

@vszakats vszakats marked this pull request as draft November 16, 2023 21:45
@vszakats vszakats force-pushed the test-autotools-warn-without-std branch 3 times, most recently from 75a5954 to 56318f9 Compare November 17, 2023 01:48
@vszakats vszakats marked this pull request as ready for review November 17, 2023 02:04
docs/examples/ftpgetinfo.c Outdated Show resolved Hide resolved
- stop calling `strcmp()` with NULL to avoid undefined behaviour.
- fix checking results if some of them were NULL.
- do not pass NULL to printf `%s`.

```
In file included from ../../tests/libtest/test.h:43,
                 from curlcheck.h:24,
                 from unit1395.c:24:
unit1395.c: In function ‘test’:
../../lib/curl_printf.h:44:18: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
   44 | # define fprintf curl_mfprintf
unit1395.c:93:7: note: in expansion of macro ‘fprintf’
   93 |       fprintf(stderr, "Test %u: '%s' gave '%s' instead of NULL\n",
      |       ^~~~~~~
```
Ref: https://github.com/curl/curl/actions/runs/6896854253/job/18763836775?pr=12346#step:33:1689
```
vssh/libssh.c: In function ‘myssh_statemach_act’:
vssh/libssh.c:1162:29: error: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 2 has type ‘uint64_t’ {aka ‘long unsigned int’} [-Werror=format=]
 1162 |         char *tmp = aprintf("statvfs:\n"
      |                             ^~~~~~~~~~~~
......
 1169 |                             statvfs->f_bsize, statvfs->f_frsize,
      |                             ~~~~~~~~~~~~~~~~
      |                                    |
      |                                    uint64_t {aka long unsigned int}
vssh/libssh.c:1163:42: note: format string is defined here
 1163 |                             "f_bsize: %llu\n" "f_frsize: %llu\n"
      |                                       ~~~^
      |                                          |
      |                                          long long unsigned int
      |                                       %lu
```
Ref: https://github.com/curl/curl/actions/runs/6896854253/job/18763838007?pr=12346#step:29:895
```
In file included from curl_setup.h:656,
                 from conncache.c:26:
conncache.c: In function ‘Curl_conncache_add_conn’:
conncache.c:246:22: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘curl_off_t’ {aka ‘long long int’} [-Werror=format=]
  246 |   DEBUGF(infof(data, "Added connection %ld. "
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~
  247 |                "The cache now contains %zu members",
  248 |                conn->connection_id, connc->num_conn));
      |                ~~~~~~~~~~~~~~~~~~~
      |                    |
      |                    curl_off_t {aka long long int}
curl_setup_once.h:286:19: note: in definition of macro ‘DEBUGF’
  286 | #define DEBUGF(x) x
      |                   ^
conncache.c:246:10: note: in expansion of macro ‘infof’
  246 |   DEBUGF(infof(data, "Added connection %ld. "
      |          ^~~~~
conncache.c:246:42: note: format string is defined here
  246 |   DEBUGF(infof(data, "Added connection %ld. "
      |                                        ~~^
      |                                          |
      |                                          long int
      |                                        %lld
```
Ref: https://github.com/curl/curl/actions/runs/6896854263/job/18763831142?pr=12346#step:6:67
```
http2.c: In function 'nw_out_writer':
http2.c:374:14: error: null pointer dereference [-Werror=null-dereference]
  374 |   nwritten = Curl_conn_cf_send(cf->next, data, (const char *)buf, buflen, err);
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
Ref: https://github.com/curl/curl/actions/runs/6896854253/job/18763839238?pr=12346#step:30:214
E.g.:
```
ftpgetinfo.c: In function 'main':
ftpgetinfo.c:78:16: error: ISO C does not support the 'I' printf flag [-Werror=format=]
   78 |         printf("filesize %s: %" CURL_FORMAT_CURL_OFF_T " bytes\n",
      |                ^~~~~~~~~~~~~~~~
ftpgetinfo.c:78:16: error: format '%d' expects argument of type 'int', but argument 3 has type 'curl_off_t' {aka 'long long int'} [-Werror=format=]
   78 |         printf("filesize %s: %" CURL_FORMAT_CURL_OFF_T " bytes\n",
      |                ^~~~~~~~~~~~~~~~
   79 |                filename, filesize);
      |                          ~~~~~~~~
      |                          |
      |                          curl_off_t {aka long long int}
In file included from ../../include/curl/curl.h:54,
                 from ftpgetinfo.c:27:
../../include/curl/system.h:205:42: note: format string is defined here
  205 | #  define CURL_FORMAT_CURL_OFF_T     "I64d"
```
Ref: https://dev.azure.com/daniel0244/curl/_build/results?buildId=18581&view=logs&jobId=ccf9cc6d-2ef1-5cf2-2c09-30f0c14f923b
Caveat: mingw old, I have no info if `inttypes.h` is supported there.
No longer necessary after fixing curl public headers.
@vszakats
Copy link
Member Author

vszakats commented Nov 17, 2023

Looked up the history of this option. Added in b23ce2c (last year) via #9542, with the goal of testing for C89 compliance. I think this would be better achieved by passing -std=gnu89 via CFLAGS in specific CI jobs where we want to test for this.

The original goal of this option was to test C89-compliance.
Keep that feature, but limited to a specific CI job. We can extended
to other gcc jobs as needed.

Don't add them to these though, as in these revealed actual issues by
building without `-std=gnu89`:

- openssl3-O3
- event-based
- Slackware-openssl-with-gssapi-gcc
- mingw-w64 builds
@github-actions github-actions bot added the CI Continuous Integration label Nov 17, 2023
@vszakats
Copy link
Member Author

Restored -std=gnu89 as CFLAGS in the linux / openssl3 CI job.

@vszakats
Copy link
Member Author

vszakats commented Nov 17, 2023

This old old-mingw header package (from 2010) does contain inttypes.h:
mingwrt-3.18-mingw32-dev.tar.gz via https://sourceforge.net/projects/mingw/files/MinGW/Base/mingwrt/mingwrt-3.18/

It means old-mingw likely continues to work fine with public headers after this change, though I haven't actually tried.

@vszakats vszakats force-pushed the test-autotools-warn-without-std branch 2 times, most recently from 71385a3 to 547fb72 Compare November 18, 2023 22:53
@vszakats
Copy link
Member Author

vszakats commented Nov 18, 2023

I couldn't replicate the same issue with libssh2 [1]. I cannot even grasp it for libssh, as it seems rooted from the fact that uint64_t somehow becomes long unsigned int "uint64_t {aka long unsigned int}" in that CI job.

Anyway, for libssh2 the MSVC-issue seems to apply too:

curl/lib/vssh/libssh2.c

Lines 1965 to 1971 in a9fd0d0

char *tmp = aprintf("statvfs:\n"
"f_bsize: %llu\n" "f_frsize: %llu\n"
"f_blocks: %llu\n" "f_bfree: %llu\n"
"f_bavail: %llu\n" "f_files: %llu\n"
"f_ffree: %llu\n" "f_favail: %llu\n"
"f_fsid: %llu\n" "f_flag: %llu\n"
"f_namemax: %llu\n",

Any thoughts to include this MSVC fix for libssh2 too?:

diff --git a/lib/vssh/libssh2.c b/lib/vssh/libssh2.c
index 543b6fb43..4eb5cf63f 100644
--- a/lib/vssh/libssh2.c
+++ b/lib/vssh/libssh2.c
@@ -1962,13 +1962,23 @@ static CURLcode ssh_statemach_act(struct Curl_easy *data, bool *block)
         break;
       }
       else if(rc == 0) {
+        #ifdef _MSC_VER
+        #define LIBSSH2_VFS_SIZE_MASK "I64u"
+        #else
+        #define LIBSSH2_VFS_SIZE_MASK "llu"
+        #endif
         char *tmp = aprintf("statvfs:\n"
-                            "f_bsize: %llu\n" "f_frsize: %llu\n"
-                            "f_blocks: %llu\n" "f_bfree: %llu\n"
-                            "f_bavail: %llu\n" "f_files: %llu\n"
-                            "f_ffree: %llu\n" "f_favail: %llu\n"
-                            "f_fsid: %llu\n" "f_flag: %llu\n"
-                            "f_namemax: %llu\n",
+                            "f_bsize: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_frsize: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_blocks: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_bfree: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_bavail: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_files: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_ffree: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_favail: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_fsid: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_flag: %" LIBSSH2_VFS_SIZE_MASK "\n"
+                            "f_namemax: %" LIBSSH2_VFS_SIZE_MASK "\n",
                             statvfs.f_bsize, statvfs.f_frsize,
                             statvfs.f_blocks, statvfs.f_bfree,
                             statvfs.f_bavail, statvfs.f_files,

Applying the same fix as used with libssh.
@vszakats vszakats force-pushed the test-autotools-warn-without-std branch from 547fb72 to a972c26 Compare November 18, 2023 23:13
@vszakats
Copy link
Member Author

vszakats commented Nov 19, 2023

To make libssh/libssh2 masks the same and future-proof, I'm thinking of moving from:
libssh:

        #ifdef _MSC_VER
        #define LIBSSH_VFS_SIZE_MASK "I64u"
        #else
        #define LIBSSH_VFS_SIZE_MASK PRIu64
        #endif

libssh2:

        #ifdef _MSC_VER
        #define LIBSSH2_VFS_SIZE_MASK "I64u"
        #else
        #define LIBSSH2_VFS_SIZE_MASK "llu"
        #endif

to this for both:

        #ifdef PRIu64
        #define LIBSSH_VFS_SIZE_MASK PRIu64
        #elif defined(_MSC_VER)
        #define LIBSSH_VFS_SIZE_MASK "I64u"
        #else
        #define LIBSSH_VFS_SIZE_MASK "llu"
        #endif

This would use the official uint64_t mask whenever available from inttypes.h either in via libssh.h (next release will include it for all targets) or curl/system.h. And fall back to the universal MSVC value, or the universal standard one for others.

...well, maybe just for libssh. For libssh2 the 64-bit type libssh2_uint64_t isn't stricly mapped to uint64_t:
https://github.com/libssh2/libssh2/blob/8c320a93a48775b74f40415e46f84bf68b4d5ae8/include/libssh2.h#L128-L145

UPDATE: Talked myself out of this. Once libssh gets a release with universal inttypes.h, we can revisit this.

@vszakats vszakats closed this in 413a0fe Nov 20, 2023
@vszakats vszakats deleted the test-autotools-warn-without-std branch November 20, 2023 22:29
vszakats added a commit to vszakats/curl that referenced this pull request Dec 17, 2023
Avoid stomping the libssh and libssh2 macro namespaces by prefixing
these with `CURL_`.

Follow-up to 413a0fe curl#12346

Closes #xxxxx
vszakats added a commit that referenced this pull request Dec 17, 2023
Avoid using the libssh and libssh2 macro namespaces by prefixing
these local macro names with `CURL_`.

Follow-up to 413a0fe #12346

Reviewed-by: Daniel Stenberg
Closes #12544
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants