tool: use our own stderr variable by vszakats · Pull Request #11958 · curl/curl

vszakats · 2023-09-26T22:14:34Z

Earlier this year we changed our own stderr variable to use the standard
name stderr (to avoid bugs where someone is using stderr instead of
the curl-tool specific variable). This solution needed to override the
standard stderr symbol via the preprocessor. This in turn didn't play
well with unity builds and caused curl tool to crash or stay silent due
to an uninitialized stderr. This was a hard to find issue, fixed by
manually breaking out one file from the unity sources.

To avoid two these two tricks, this patch implements a different
solution: Restore using our own local variable for our stderr output and
leave stderr as-is. To avoid using stderr by mistake, add a
checksrc rule (based on logic we already used in lib for strerror)
that detects any stderr use in src and points to using our own
variable instead: tool_stderr.

Follow-up to 06133d3
Follow-up to 2f17a9b

Closes #11958

TOTEST: See how this work when running tests. There is no UNITTEST mode now, tool_stderr needs to be initialized to stderr for unittests. [PASSED]

We've recently change our own stderr variable to use the standard name `stderr` (presumably to avoid bugs where someone is using `stderr` instead of the curl-tool specific variable). This solution needed to override the standard `stderr` symbol via the preprocessor. This in turn didn't play well with unity builds and caused curl tool to crash or stay silent due to an uninitialized stderr. This hard to find issue was fixed by manually breaking out one file from the unity sources. To avoid two these two tricks, this patch implements a different solution: Restore using our own local variable for our stderr output and leave `stderr` as-is. To avoid using `stderr` by mistake, add a `checksrc` rule (based on logic we already used in lib for `strerror`) that detects any `stderr` use in `src` and points to using our own variable instead: `tool_stderr`. Follow-up to 06133d3 Follow-up to 2f17a9b Closes curl#11958

Romain-Geissler-1A · 2023-10-11T08:07:44Z

Hi,

FYI, we are trying to build curl 8.4.0 (following the CVE announcement), and somehow the changes here makes the build (we use autotools by default, not cmake) error by default. It errors actually with what checksrc considers only as warnings (so maybe we should use some option to not stop the build ?):

09:20:40  ./tool_stderr.c:62:1: warning: STDERR already disabled from line 35 (BADCOMMAND)
09:20:40     /* !checksrc! disable STDERR 1 */
09:20:40   ^
09:20:40  ./tool_stderr.c:69:1: warning: STDERR already disabled from line 35 (BADCOMMAND)
09:20:40     /* !checksrc! disable STDERR 1 */
09:20:40   ^
09:20:40  ./tool_stderr.c:35:18: warning: Unused ignore: STDERR (UNUSEDIGNORE)
09:20:40     /* !checksrc! disable STDERR 1 */
09:20:40                    ^
09:20:40  checksrc: 0 errors and 3 warnings
09:20:40  checksrc: 0 errors and 10 warnings suppressed

and I tried to apply the following patch:

--- src/tool_stderr.c
+++ src/tool_stderr.c
@@ -32,7 +32,6 @@
 
 void tool_init_stderr(void)
 {
-  /* !checksrc! disable STDERR 1 */
   tool_stderr = stderr;
 }
 
@@ -59,13 +58,11 @@

   /* freopen the actual stderr (stdio.h stderr) instead of tool_stderr since
      the latter may be set to stdout. */
-  /* !checksrc! disable STDERR 1 */
   fp = freopen(filename, FOPEN_WRITETEXT, stderr);
   if(!fp) {
     /* stderr may have been closed by freopen. there is nothing to be done. */
     DEBUGASSERT(0);
     return;
   }
-  /* !checksrc! disable STDERR 1 */
   tool_stderr = stderr;
 }

which "unblocked" the build, however it really doesn't sound expected at all considering what checksrc is meant to check ! ;) So I am not sure if we are the only ones or if everyone has this problem.

Romain-Geissler-1A · 2023-10-11T08:22:28Z

Ah actually it's the file src/.checksrc that this pull request adds which is missing in the curl 8.4.0 tarball. It works if I apply this patch instead:

This file is committed in git, but missing from the curl 8.4.0 tarball, and makes the build fail if missing.

--- /dev/null
+++ src/.checksrc
@@ -0,0 +1 @@
+enable STDERR

I will open an issue for that, so a new release tarball is created (as everyone will try to rebuild curl following the CVE announcement).

Regression from e5bb88b curl#11958 Bug: curl#11958 (comment) Reported-by: Romain Geissler Closes #xxxxx

Romain-Geissler-1A · 2023-10-11T08:32:41Z

@vszakats I see you have already a commit to fix the tarball generation. FYI, I reported this issue as #12084.

vszakats · 2023-10-11T08:40:51Z

Yes, thanks for reporting this. I've linked #12084 after noticing it.

vszakats · 2023-10-11T08:42:35Z

I wonder why this slipped through the distcheck CI run. Maybe
hidden files are exempt from it?

Regression from e5bb88b #11958 Bug: #11958 (comment) Reported-by: Romain Geissler Fixes #12084 Closes #12085

This caused no error, but it's incorrect and introduced a potential difference to builds without `STDERR_FILENO`, which in practice means MSVC. The macro's only use needs to use `stderr` always. Follow-up to e5bb88b curl#11958

Before this patch the signal handler called `logmsg()` which in turn called `printf()` variants (internal implementations), and `FILE *` functions, `localtime()`. Some of these called `malloc`/`free`, which isn't supported in s signal handler. Replace them with `write` calls, losing some logging functionality. Also: - De-dupe and move `STD*_FILENO` macros to `lib/curl_setup.h`. Revert the `src` definition to point to `stderr`, instead of `tool_stderr`. Follow-up to e5bb88b #11958 POSIX specs with list of functions allowed in a signal handler: 2004: https://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html#tag_02_04_03 2017: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03 2024: https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html#tag_16_04_03 Linux CI run with the thread sanitizer going crazy when hitting the signal handler in test 1238 and 1242 (TFTP): ``` WARNING: ThreadSanitizer: signal-unsafe call inside of a signal (pid=12582) #0 malloc <null> (servers+0x5ed70) #1 _IO_file_doallocate <null> (libc.so.6+0x851b4) #2 formatf /home/runner/work/curl/curl/bld/tests/server/../../lib/../../lib/mprintf.c:886:9 (servers+0xdff77) [...] WARNING: ThreadSanitizer: signal-unsafe call inside of a signal (pid=12582) #0 free <null> (servers+0x5f453) #1 fclose <null> (libc.so.6+0x8532f) #2 logmsg /home/runner/work/curl/curl/bld/tests/server/../../../tests/server/util.c:134:5 (servers+0xe684d) ``` Ref: https://github.com/curl/curl/actions/runs/14118903372/job/39555309490?pr=16851 Closes #16852

Before this patch the signal handler called `logmsg()` which in turn called `printf()` variants (internal implementations), and `FILE *` functions, `localtime()`. Some of these called `malloc`/`free`, which isn't supported in s signal handler. Replace them with `write` calls, losing some logging functionality. Also: - De-dupe and move `STD*_FILENO` macros to `lib/curl_setup.h`. Revert the `src` definition to point to `stderr`, instead of `tool_stderr`. Follow-up to e5bb88b curl#11958 POSIX specs with list of functions allowed in a signal handler: 2004: https://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html#tag_02_04_03 2017: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03 2024: https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html#tag_16_04_03 Linux CI run with the thread sanitizer going crazy when hitting the signal handler in test 1238 and 1242 (TFTP): ``` WARNING: ThreadSanitizer: signal-unsafe call inside of a signal (pid=12582) #0 malloc <null> (servers+0x5ed70) #1 _IO_file_doallocate <null> (libc.so.6+0x851b4) curl#2 formatf /home/runner/work/curl/curl/bld/tests/server/../../lib/../../lib/mprintf.c:886:9 (servers+0xdff77) [...] WARNING: ThreadSanitizer: signal-unsafe call inside of a signal (pid=12582) #0 free <null> (servers+0x5f453) #1 fclose <null> (libc.so.6+0x8532f) curl#2 logmsg /home/runner/work/curl/curl/bld/tests/server/../../../tests/server/util.c:134:5 (servers+0xe684d) ``` Ref: https://github.com/curl/curl/actions/runs/14118903372/job/39555309490?pr=16851 Closes curl#16852

vszakats added build cmake cmdline tool tidy-up labels Sep 26, 2023

vszakats force-pushed the src-stderr-unity-friendly branch from 82a29a6 to 174cdcd Compare September 26, 2023 22:15

vszakats added 2 commits September 26, 2023 22:16

fix reuse compliance

21fa8b8

vszakats force-pushed the src-stderr-unity-friendly branch from 174cdcd to 21fa8b8 Compare September 26, 2023 22:17

vszakats closed this in e5bb88b Sep 28, 2023

vszakats deleted the src-stderr-unity-friendly branch September 28, 2023 10:52

Romain-Geissler-1A mentioned this pull request Oct 11, 2023

Curl 8.4.0 fails to build because of a checksrc error. File src/.checksrc is missing from release tarball #12084

Closed

vszakats added a commit to vszakats/curl that referenced this pull request Oct 11, 2023

build: add src/.checksrc to source tarball

1080915

Regression from e5bb88b curl#11958 Bug: curl#11958 (comment) Reported-by: Romain Geissler Closes #xxxxx

vszakats mentioned this pull request Oct 11, 2023

build: add src/.checksrc to source tarball #12085

Closed

vszakats added a commit that referenced this pull request Oct 11, 2023

build: add src/.checksrc to source tarball

fac90bd

Regression from e5bb88b #11958 Bug: #11958 (comment) Reported-by: Romain Geissler Fixes #12084 Closes #12085

vszakats mentioned this pull request Mar 28, 2025

tests/server: make the signal handler signal-safe #16852

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

tool: use our own stderr variable#11958

tool: use our own stderr variable#11958
vszakats wants to merge 2 commits intocurl:masterfrom
vszakats:src-stderr-unity-friendly

vszakats commented Sep 26, 2023 •

edited

Loading

Uh oh!

Romain-Geissler-1A commented Oct 11, 2023

Uh oh!

Romain-Geissler-1A commented Oct 11, 2023

Uh oh!

Romain-Geissler-1A commented Oct 11, 2023

Uh oh!

vszakats commented Oct 11, 2023

Uh oh!

vszakats commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vszakats commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Romain-Geissler-1A commented Oct 11, 2023

Uh oh!

Romain-Geissler-1A commented Oct 11, 2023

Uh oh!

Romain-Geissler-1A commented Oct 11, 2023

Uh oh!

vszakats commented Oct 11, 2023

Uh oh!

vszakats commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

vszakats commented Sep 26, 2023 •

edited

Loading