On Sun, Mar 28, 2010 at 08:23:00PM +0200, Johannes Sixt wrote:
> 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@xxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html