Re: Some fixes to GPG signature verification in pkg_install

[Pierre Pronchery <khorben%defora.org@localhost>]

			Hey,

On 05/28/15 11:39, Joerg Sonnenberger wrote:
> On Thu, May 28, 2015 at 12:52:25AM +0200, Pierre Pronchery wrote:
>> 2/4: error codes
>>      Ensures error codes from child processes are below 126 (2 instead
>>      of 255) for failed executions attempts of the GPG binary.
> 
> Why?

To disambiguate from:
- (1: bad syntax, wrong signature)
- 126: shell could not execute
- 127: command not found
- >= 128: termination due to a signal

>> diff --git a/external/bsd/pkg_install/dist/lib/gpgsig.c b/external/bsd/pkg_install/dist/lib/gpgsig.c
>> index 645d04c..ae21f3f 100644
>> --- a/external/bsd/pkg_install/dist/lib/gpgsig.c
>> +++ b/external/bsd/pkg_install/dist/lib/gpgsig.c
>> @@ -92,6 +92,7 @@ verify_signature(const char *input, size_t input_len, const char *keyring,
>>  		*argvp = NULL;
>>  
>>  		execvp(gpg_cmd, __UNCONST(argv));
>> +		warn("%s", gpg_cmd);
>>  		_exit(2);
>>  	}
>>  	close(fd[0]);
>> @@ -216,6 +217,7 @@ detached_gpg_sign(const char *content, size_t len, char **sig, size_t *sig_len,
>>  		*argvp = NULL;
>>  
>>  		execvp(gpg_cmd, __UNCONST(argv));
>> +		warn("%s", gpg_cmd);
>>  		_exit(2);
>>  	}
>>  	close(fd_in[0]);
>>
> 
> This part is wrong. You must not use stdio in a vfork'd child (and it is
> generally bad to do it in a fork'd child either).

It is not instantly obvious from the warn(3) manpage that it uses stdio,
but it probably does; I can correct this in another version of the patch
in any case.

>>  	if (child == 0) {
>>  		close(fd[1]);
>>  		close(STDIN_FILENO);
>>  		if (dup2(fd[0], STDIN_FILENO) == -1) {
>> -			static const char err_msg[] =
>> -			    "Cannot redirect stdin of GPG process\n";
>> -			write(STDERR_FILENO, err_msg, sizeof(err_msg) - 1);
>> +			warn("Cannot redirect stdin of GPG process");
>>  			_exit(2);
>>  		}
>>  		close(fd[0]);
> 
> Same. Generally, I think this is the wrong approach. If you want to
> improve the temp file logic, drive gpg as filter and avoid them in first
> place. Libarchive has an implementation of the necessary logic.

Not sure I see the relevant API calls but I can give it more thought.

Cheers,
-- 
khorben