Re: [PATCH] quickfetch(): Prevent overflow of the rev-list command line

From: Johannes Sixt <j.sixt@viscovery.net>
Date: Jul 9, 2009, 12:22 UTC
Message-ID: <4A55E100.9010700@viscovery.net>
In-Reply-To: <200907091134.45492.johan@herland.net>

Johan Herland schrieb:

> However, using rev-list --stdin is not entirely straightforward: rev-list
> terminates immediately when encountering an unknown object, which can
> trigger SIGPIPE if we are still writing object's to its standard input.
> We therefore ignore SIGPIPE so that the fetch process is not terminated.

I removed the "signal(SIGPIPE, SIG_IGN)", but the test suite still passes. IOW, there is no test case that has the configuration that you describe here. Would you please add such a test (perhaps in t5502)? It would also help me verify the patch works as intended on Windows.

> Signed-off-by: Johan Herland <johan@herland.net>
> Improved-by: Johannes Sixt <j.sixt@viscovery.net>

Please make this <j6t@kdbg.org> despite the email address I'm using right now.

> Improved-by: Alex Riesen <raa.lkml@gmail.com>
> Tested-by: Peter Krefting <peter@softwolves.pp.se>

Show 15 quoted lines

> +	for (ref = ref_map; ref; ref = ref->next) {
> +		if (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||
> +		    write_in_full(revlist.in, "\n", 1) < 0) {
> +			err = errno;
> +			if (err != EPIPE && err != EINVAL)
> +				error("failed write to rev-list");
> +			break;
> +		}
> +	}
> +
> +	if (close(revlist.in)) {
> +		err = errno;
> +		error("failed to close rev-list's stdin");
> +	}
> +	return finish_command(&revlist) || err;

The call site of quickfetch() is not interested in the errno, only on whether the return value is non-zero: You can just assign -1 to err (that's our convention for failure). OTOH, it would be helpful to include strerror(errno) in the error message.

Shouldn't you reset signal(SIGPIPE) to its previous value?

-- Hannes