threads / patch / 22445

patchrun-command.c: fix build warnings on Ubuntu

Subject: [PATCH] run-command.c: fix build warnings on Ubuntu

## tl;dr

8 messages between Jan 29, 2010 and Mar 17, 2011. Diffs are folded; open one to read it.

replies: 7people: 5as markdown or json

Michael Wookey· Jan 29, 2010, 22:38 UTC · lore

Building git on Ubuntu 9.10 warns that the return value of write(2) isn't checked. These warnings were introduced in commits:

  2b541bf8 ("start_command: detect execvp failures early")
  a5487ddf ("start_command: report child process setup errors to the
parent's stderr")
GCC details:
  $ gcc --version
  gcc (Ubuntu 4.4.1-4ubuntu9) 4.4.1

Silence the warnings by reading (but not making use of) the return value of write(2).

Signed-off-by: Michael Wookey <michaelwookey@gmail.com>
---
Although this will fix the build warnings, I am unsure if there is a
better way to achieve the same result. Using "(void)write(...)" still
gives warnings and I am unaware of any annotations that will silence
gcc.
 run-command.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)
Show changes to run-command.c +6 −4
diff --git a/run-command.c b/run-command.c
index 2feb493..3206d61 100644
--- a/run-command.c
+++ b/run-command.c
@@ -67,19 +67,21 @@ static int child_notifier = -1;

 static void notify_parent(void)
 {
-	write(child_notifier, "", 1);
+	ssize_t unused;
+	unused = write(child_notifier, "", 1);
 }

 static NORETURN void die_child(const char *err, va_list params)
 {
 	char msg[4096];
+	ssize_t unused;
 	int len = vsnprintf(msg, sizeof(msg), err, params);
 	if (len > sizeof(msg))
 		len = sizeof(msg);

-	write(child_err, "fatal: ", 7);
-	write(child_err, msg, len);
-	write(child_err, "\n", 1);
+	unused = write(child_err, "fatal: ", 7);
+	unused = write(child_err, msg, len);
+	unused = write(child_err, "\n", 1);
 	exit(128);
 }

-- 
1.7.0.rc0.48.gdace5
Markus Heidelberg· Jan 30, 2010, 16:43 UTC · re: Michael Wookey · lore

Re: [PATCH] run-command.c: fix build warnings on Ubuntu

Michael Wookey, 2010-01-29:
Show 10 quoted lines
> Building git on Ubuntu 9.10 warns that the return value of write(2)
> isn't checked.
> 
> GCC details:
> 
>   $ gcc --version
>   gcc (Ubuntu 4.4.1-4ubuntu9) 4.4.1
> 
> Silence the warnings by reading (but not making use of) the return value
> of write(2).

Since a few weeks I get several warnings about fwrite(), currently 28 times this: warning: ignoring return value of ‘fwrite’, declared with attribute warn_unused_result

gcc (Gentoo 4.3.4 p1.0, pie-10.1.5) 4.3.4
Not sure if it should be muted, that are really many places.
Markus
Jonathan Nieder· Mar 16, 2011, 03:51 UTC · re: Michael Wookey · lore

[PATCH] run-command: prettify -D_FORTIFY_SOURCE workaround

Current gcc + glibc with -D_FORTIFY_SOURCE try very aggressively to protect against a programming style which uses write(...) without checking the return value for errors. Even the usual hint of casting to (void) does not suppress the warning.

Sometimes when there is an output error, especially right before exit, there really is nothing to be done. The obvious solution, adopted in v1.7.0.3~20^2 (run-command.c: fix build warnings on Ubuntu, 2010-01-30), is to save the return value to a dummy variable:

	ssize_t dummy;
	dummy = write(...);

But that (1) is ugly and (2) triggers -Wunused-but-set-variable warnings with gcc-4.6 -Wall, so we are not much better off than when we started.

Instead, use an "if" statement with an empty body to make the intent clear.

	if (write(...))
		; /* yes, yes, there was an error. */
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,
Michael Wookey wrote:
> Although this will fix the build warnings, I am unsure if there is a
> better way to achieve the same result. Using "(void)write(...)" still
> gives warnings and I am unaware of any annotations that will silence
> gcc.

It's been a long time (and meanwhile the patch has been working; thanks!). How about something like this?

 run-command.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
Show changes to run-command.c +6 −6
diff --git a/run-command.c b/run-command.c
index 3206d61..5b68907 100644
--- a/run-command.c
+++ b/run-command.c
@@ -67,21 +67,21 @@ static int child_notifier = -1;
 
 static void notify_parent(void)
 {
-	ssize_t unused;
-	unused = write(child_notifier, "", 1);
+	if (write(child_notifier, "", 1))
+		; /* ok. */
 }
 
 static NORETURN void die_child(const char *err, va_list params)
 {
 	char msg[4096];
-	ssize_t unused;
 	int len = vsnprintf(msg, sizeof(msg), err, params);
 	if (len > sizeof(msg))
 		len = sizeof(msg);
 
-	unused = write(child_err, "fatal: ", 7);
-	unused = write(child_err, msg, len);
-	unused = write(child_err, "\n", 1);
+	if (write(child_err, "fatal: ", 7) ||
+	    write(child_err, msg, len) ||
+	    write(child_err, "\n", 1))
+		; /* ok. */
 	exit(128);
 }
 

-- 
1.7.4.1
Junio C Hamano· Mar 16, 2011, 05:37 UTC · re: Jonathan Nieder · lore

Re: [PATCH] run-command: prettify -D_FORTIFY_SOURCE workaround

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 5 quoted lines
> Instead, use an "if" statement with an empty body to make the intent
> clear.
>
> 	if (write(...))
> 		; /* yes, yes, there was an error. */

Yuck --- and that is not meant against your workaround, but against the compiler bogosity. The above is reasonable (for some definition of the word) and the comment makes the yuckiness tolerable by being somewhat amusing.

But your comment in the actual patch is not amusing at all.

It certainly is _not_ "ok" to see errors from write(2); we are _ignoring_ the error because at that point in the codepath there isn't any better alternative. The unusual "if ()" whose condition is solely for its side effect, with an empty body, is a strong enough sign to any reader that there is something fishy going on, and it would be helpful to the reader to hint _why_ such an unusual construct is there. It would be much better for the longer term maintainability to say at least "gcc" in the comment, i.e.

	if (write(...))
        	; /* we know we are ignoring the error, mr gcc! */
or something.
Thanks for another amusing patch.
Jonathan Nieder· Mar 16, 2011, 07:32 UTC · re: Junio C Hamano · lore

[PATCH v2] run-command: prettify -D_FORTIFY_SOURCE workaround

Current gcc + glibc with -D_FORTIFY_SOURCE try very aggressively to protect against a programming style which uses write(...) without checking the return value for errors. Even the usual hint of casting to (void) does not suppress the warning.

Sometimes when there is an output error, especially right before exit, there really is nothing to be done. The obvious solution, adopted in v1.7.0.3~20^2 (run-command.c: fix build warnings on Ubuntu, 2010-01-30), is to save the return value to a dummy variable:

	ssize_t dummy;
	dummy = write(...);

But that (1) is ugly and (2) triggers -Wunused-but-set-variable warnings with gcc-4.6 -Wall, so we are not much better off than when we started.

Instead, use an "if" statement with an empty body to make the intent clear.

	if (write(...))
		; /* yes, yes, there was an error. */
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Junio C Hamano <gitster@pobox.com>
---
Junio C Hamano wrote:
Show 9 quoted lines
>               The unusual "if ()" whose condition is solely for its side
> effect, with an empty body, is a strong enough sign to any reader that
> there is something fishy going on, and it would be helpful to the reader
> to hint _why_ such an unusual construct is there.  It would be much better
> for the longer term maintainability to say at least "gcc" in the comment,
> i.e.
> 
> 	if (write(...))
>         	; /* we know we are ignoring the error, mr gcc! */
Very true.  Some comments to that effect below.
 run-command.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)
Show changes to run-command.c +11 −6
diff --git a/run-command.c b/run-command.c
index 3206d61..ecd9d1c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -67,21 +67,26 @@ static int child_notifier = -1;
 
 static void notify_parent(void)
 {
-	ssize_t unused;
-	unused = write(child_notifier, "", 1);
+	/*
+	 * execvp failed.  If possible, we'd like to let start_command
+	 * know, so failures like ENOENT can be handled right away; but
+	 * otherwise, finish_command will still report the error.
+	 */
+	if (write(child_notifier, "", 1))
+		; /* yes, dear gcc -D_FORTIFY_SOURCE, there was an error. */
 }
 
 static NORETURN void die_child(const char *err, va_list params)
 {
 	char msg[4096];
-	ssize_t unused;
 	int len = vsnprintf(msg, sizeof(msg), err, params);
 	if (len > sizeof(msg))
 		len = sizeof(msg);
 
-	unused = write(child_err, "fatal: ", 7);
-	unused = write(child_err, msg, len);
-	unused = write(child_err, "\n", 1);
+	if (write(child_err, "fatal: ", 7) ||
+	    write(child_err, msg, len) ||
+	    write(child_err, "\n", 1))
+		; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */
 	exit(128);
 }
 

-- 
1.7.4.1
Junio C Hamano· Mar 17, 2011, 22:34 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2] run-command: prettify -D_FORTIFY_SOURCE workaround

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 10 quoted lines
>  static NORETURN void die_child(const char *err, va_list params)
>  {
> ...
> -	unused = write(child_err, "fatal: ", 7);
> -	unused = write(child_err, msg, len);
> -	unused = write(child_err, "\n", 1);
> +	if (write(child_err, "fatal: ", 7) ||
> +	    write(child_err, msg, len) ||
> +	    write(child_err, "\n", 1))
> +		; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */

Strictly speaking, this changes behaviour by stopping at the first failure from write(2), but I don't think we care.

Thanks.
Johannes Sixt· Mar 16, 2011, 09:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] run-command: prettify -D_FORTIFY_SOURCE workaround

Am 3/16/2011 6:37, schrieb Junio C Hamano:
Show 11 quoted lines
> It certainly is _not_ "ok" to see errors from write(2); we are _ignoring_
> the error because at that point in the codepath there isn't any better
> alternative.  The unusual "if ()" whose condition is solely for its side
> effect, with an empty body, is a strong enough sign to any reader that
> there is something fishy going on, and it would be helpful to the reader
> to hint _why_ such an unusual construct is there.  It would be much better
> for the longer term maintainability to say at least "gcc" in the comment,
> i.e.
> 
> 	if (write(...))
>         	; /* we know we are ignoring the error, mr gcc! */
And what about compilers that warn:
	';' : empty controlled statement found; is this the intent?
That's from MSVC. Perhaps:
	if (write(...))
		(void)0; /* we know we are ignoring the error, mr gcc! */
-- Hannes
Jonathan Nieder· Mar 16, 2011, 09:25 UTC · re: Johannes Sixt · lore

Re: [PATCH] run-command: prettify -D_FORTIFY_SOURCE workaround

Johannes Sixt wrote:
Show 8 quoted lines
> And what about compilers that warn:
> 
> 	';' : empty controlled statement found; is this the intent?
>
> That's from MSVC. Perhaps:
>
> 	if (write(...))
> 		(void)0; /* we know we are ignoring the error, mr gcc! */
Mm, thanks for pointing it out.

Your suggestion is part of a bigger change that imho should go in a separate patch:

	$ git grep -F -e '	; /*' origin/master | wc -l
	65
I would prefer to see such a patch do
	if (write(...)) {
		/* ... explanation goes here ... */
	}
or something like
	#define do_nothing() do { /* nothing */ } while (0)
	if (write(...))
		do_nothing();	/* ... explanation ... */
but that is a small detail.

← back to recent threads