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.

Sign up for GitHub

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

Jump to bottom

tool: use our own stderr variable #11958

Closed
wants to merge 2 commits into from
Closed

tool: use our own stderr variable #11958

wants to merge 2 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 26, 2023 •
edited

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]

@vszakats
tool: use our own stderr variable
5eb6a37 
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
@vszakats
fix reuse compliance
21fa8b8
vszakats added a commit to vszakats/curl that referenced this pull request Sep 27, 2023
@vszakats
tool: use our own stderr variable
1e5bac0 
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 curl#11958
@vszakats vszakats closed this in e5bb88b Sep 28, 2023
@vszakats vszakats deleted the src-stderr-unity-friendly branch September 28, 2023 10:52
@Romain-Geissler-1A
Copy link
Contributor

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
Copy link
Contributor

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).

@Romain-Geissler-1A Romain-Geissler-1A mentioned this pull request Oct 11, 2023
Closed
vszakats added a commit to vszakats/curl that referenced this pull request Oct 11, 2023
@vszakats
build: add src/.checksrc to source tarball
1080915 
Regression from e5bb88b curl#11958

Bug: curl#11958 (comment)
Reported-by: Romain Geissler
Closes #xxxxx
@vszakats vszakats mentioned this pull request Oct 11, 2023
Closed
@Romain-Geissler-1A
Copy link
Contributor

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

@vszakats
Copy link
Member Author

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

@vszakats
Copy link
Member Author

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

vszakats added a commit that referenced this pull request Oct 11, 2023
@vszakats
build: add src/.checksrc to source tarball
fac90bd 
Regression from e5bb88b #11958

Bug: #11958 (comment)
Reported-by: Romain Geissler
Fixes #12084
Closes #12085
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build cmake cmdline tool tidy-up
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vszakats @Romain-Geissler-1A