Re: [PATCH 3/3] diff: add "fasttextconv" config option

From: Jeff King <peff@peff.net>
Date: Mar 30, 2010, 16:30 UTC
Message-ID: <20100330163004.GC17763@coredump.intra.peff.net>
In-Reply-To: <201003282023.00913.j6t@kdbg.org>

On Sun, Mar 28, 2010 at 08:23:00PM +0200, Johannes Sixt wrote:

Show 9 quoted lines

> On Sonntag, 28. März 2010, Jeff King wrote:
> > +	if (start_command(&child) != 0 ||
> > +	    strbuf_read(&buf, child.out, 0) < 0 ||
> > +	    finish_command(&child) != 0) {
> 
> This conditional is somewhat dubious. If strbuf_read fails, you do not wait 
> for the child, and a zombie remains.
> 
> The have this sequence already in run_textconv().

Ugh. I would blame the author of run_textconv, but that is also me. :)

I doubt it is a big deal in practice, as the read() call is the least likely to produce an error, and even if we do get a zombie, it will die along with the diffing process. But it is still worth fixing. Thanks for noticing.

Junio, can you apply the patch below to maint?

-- >8 --
Subject: [PATCH] diff: fix textconv error zombies

To make the code simpler, run_textconv lumps all of its error checking into one conditional. However, the short-circuit means that an error in reading will prevent us from calling finish_command, leaving a zombie child.

The cleanup requirements are actually different for each of the three error checks, so let's just write them out longhand.

Signed-off-by: Jeff King <peff@peff.net>
---
Yes, there are clever ways to make this shorter, but I think it is
clearer just written out.

 diff.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 0d465fa..c268cfc 100644
--- a/diff.c
+++ b/diff.c
@@ -3875,9 +3875,19 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,
 	child.use_shell = 1;
 	child.argv = argv;
 	child.out = -1;
-	if (start_command(&child) != 0 ||
-	    strbuf_read(&buf, child.out, 0) < 0 ||
-	    finish_command(&child) != 0) {
+	if (start_command(&child) != 0) {
+		remove_tempfile();
+		error("error running textconv command '%s'", pgm);
+		return NULL;
+	}
+	if (strbuf_read(&buf, child.out, 0) < 0) {
+		close(child.out);
+		finish_command(&child);
+		remove_tempfile();
+		error("error reading from textconv command '%s'", pgm);
+		return NULL;
+	}
+	if (finish_command(&child) != 0) {
 		close(child.out);
 		strbuf_release(&buf);
 		remove_tempfile();

-- 
1.7.0.3.460.g6f052