From: Johannes Sixt <j6t@kdbg.org>
Date: Sun, 03 Apr 2016 06:50:27 GMT
Subject: Re: [PATCH v3 1/4] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface
Message-ID: <5700BD33.6090602@kdbg.org>
In-Reply-To: <1459638975-17705-2-git-send-email-santiago@nyu.edu>

Am 03.04.2016 um 01:16 schrieb santiago@nyu.edu:
> From: Santiago Torres <santiago@nyu.edu>
>
> The verify_signed_buffer comand might cause a SIGPIPE signal when the
> gpg child process terminates early (due to a bad keyid, for example) and
> git tries to write to it afterwards. Previously, ignoring SIGPIPE was
> done on the builtin/gpg-verify.c command to avoid this issue. However,
> any other caller who wanted to use the verify_signed_buffer command
> would have to include this signal call.
>
> Instead, we use sigchain_push(SIGPIPE, SIG_IGN) on the
> verify_signed_buffer call (pretty much like in sign_buffer()) so
> that any caller is not required to perform this task. This will avoid
> possible mistakes by further developers using verify_signed_buffer.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> Notes:
>   I dropped the multiline comment altogheter.
>
>   builtin/verify-tag.c | 3 ---
>   gpg-interface.c      | 3 +++
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 00663f6..77f070a 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -95,9 +95,6 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>   	if (verbose)
>   		flags |= GPG_VERIFY_VERBOSE;
>
> -	/* sometimes the program was terminated because this signal
> -	 * was received in the process of writing the gpg input: */
> -	signal(SIGPIPE, SIG_IGN);
>   	while (i < argc)
>   		if (verify_tag(argv[i++], flags))
>   			had_error = 1;
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 3dc2fe3..c1f6b2d 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -232,6 +232,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>   	if (gpg_output)
>   		gpg.err = -1;
>   	args_gpg[3] = path;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
>   	if (start_command(&gpg)) {
>   		unlink(path);
>   		return error(_("could not run gpg."));

But no sigchain_pop() in the error path that we see here?

Perhaps you can even defer the sigchain_push() until after start_command()?

> @@ -250,6 +252,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>   	close(gpg.out);
>
>   	ret = finish_command(&gpg);
> +	sigchain_pop(SIGPIPE);
>
>   	unlink_or_warn(path);
>
>

-- Hannes