Skip to content

Commit ed0f7bd

Browse files
gitstergitster
committed
Merge branch 'jk/gpg-interface-cleanup'
A new run-command API function pipe_command() is introduced to sanely feed data to the standard input while capturing data from the standard output and the standard error of an external process, which is cumbersome to hand-roll correctly without deadlocking. The codepath to sign data in a prepared buffer with GPG has been updated to use this API to read from the status-fd to check for errors (instead of relying on GPG's exit status). * jk/gpg-interface-cleanup: gpg-interface: check gpg signature creation status sign_buffer: use pipe_command verify_signed_buffer: use pipe_command run-command: add pipe_command helper verify_signed_buffer: use tempfile object verify_signed_buffer: drop pbuf variable gpg-interface: use child_process.args
2 parents 1d77bed + efee955 commit ed0f7bd

File tree

4 files changed

+214
-71
lines changed

4 files changed

+214
-71
lines changed

‎gpg-interface.c‎

Lines changed: 35 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include "strbuf.h"
44
#include "gpg-interface.h"
55
#include "sigchain.h"
6+
#include "tempfile.h"
67

78
static char *configured_signing_key;
89
static const char *gpg_program = "gpg";
@@ -150,42 +151,30 @@ const char *get_signing_key(void)
150151
int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
151152
{
152153
struct child_process gpg = CHILD_PROCESS_INIT;
153-
const char *args[4];
154-
ssize_t len;
154+
int ret;
155155
size_t i, j, bottom;
156+
struct strbuf gpg_status = STRBUF_INIT;
156157

157-
gpg.argv = args;
158-
gpg.in = -1;
159-
gpg.out = -1;
160-
args[0] = gpg_program;
161-
args[1] = "-bsau";
162-
args[2] = signing_key;
163-
args[3] = NULL;
158+
argv_array_pushl(&gpg.args,
159+
gpg_program,
160+
"--status-fd=2",
161+
"-bsau", signing_key,
162+
NULL);
164163

165-
if (start_command(&gpg))
166-
return error(_("could not run gpg."));
164+
bottom = signature->len;
167165

168166
/*
169167
* When the username signingkey is bad, program could be terminated
170168
* because gpg exits without reading and then write gets SIGPIPE.
171169
*/
172170
sigchain_push(SIGPIPE, SIG_IGN);
173-
174-
if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
175-
close(gpg.in);
176-
close(gpg.out);
177-
finish_command(&gpg);
178-
return error(_("gpg did not accept the data"));
179-
}
180-
close(gpg.in);
181-
182-
bottom = signature->len;
183-
len = strbuf_read(signature, gpg.out, 1024);
184-
close(gpg.out);
185-
171+
ret = pipe_command(&gpg, buffer->buf, buffer->len,
172+
signature, 1024, &gpg_status, 0);
186173
sigchain_pop(SIGPIPE);
187174

188-
if (finish_command(&gpg) || !len || len < 0)
175+
ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
176+
strbuf_release(&gpg_status);
177+
if (ret)
189178
return error(_("gpg failed to sign the data"));
190179

191180
/* Strip CR from the line endings, in case we are on Windows. */
@@ -210,50 +199,38 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
210199
struct strbuf *gpg_output, struct strbuf *gpg_status)
211200
{
212201
struct child_process gpg = CHILD_PROCESS_INIT;
213-
const char *args_gpg[] = {NULL, "--status-fd=1", "--verify", "FILE", "-", NULL};
214-
char path[PATH_MAX];
202+
static struct tempfile temp;
215203
int fd, ret;
216204
struct strbuf buf = STRBUF_INIT;
217-
struct strbuf *pbuf = &buf;
218205

219-
args_gpg[0] = gpg_program;
220-
fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
206+
fd = mks_tempfile_t(&temp, ".git_vtag_tmpXXXXXX");
221207
if (fd < 0)
222-
return error_errno(_("could not create temporary file '%s'"), path);
223-
if (write_in_full(fd, signature, signature_size) < 0)
224-
return error_errno(_("failed writing detached signature to '%s'"), path);
208+
return error_errno(_("could not create temporary file"));
209+
if (write_in_full(fd, signature, signature_size) < 0) {
210+
error_errno(_("failed writing detached signature to '%s'"),
211+
temp.filename.buf);
212+
delete_tempfile(&temp);
213+
return -1;
214+
}
225215
close(fd);
226216

227-
gpg.argv = args_gpg;
228-
gpg.in = -1;
229-
gpg.out = -1;
230-
if (gpg_output)
231-
gpg.err = -1;
232-
args_gpg[3] = path;
233-
if (start_command(&gpg)) {
234-
unlink(path);
235-
return error(_("could not run gpg."));
236-
}
217+
argv_array_pushl(&gpg.args,
218+
gpg_program,
219+
"--status-fd=1",
220+
"--verify", temp.filename.buf, "-",
221+
NULL);
237222

238-
sigchain_push(SIGPIPE, SIG_IGN);
239-
write_in_full(gpg.in, payload, payload_size);
240-
close(gpg.in);
223+
if (!gpg_status)
224+
gpg_status = &buf;
241225

242-
if (gpg_output) {
243-
strbuf_read(gpg_output, gpg.err, 0);
244-
close(gpg.err);
245-
}
246-
if (gpg_status)
247-
pbuf = gpg_status;
248-
strbuf_read(pbuf, gpg.out, 0);
249-
close(gpg.out);
250-
251-
ret = finish_command(&gpg);
226+
sigchain_push(SIGPIPE, SIG_IGN);
227+
ret = pipe_command(&gpg, payload, payload_size,
228+
gpg_status, 0, gpg_output, 0);
252229
sigchain_pop(SIGPIPE);
253230

254-
unlink_or_warn(path);
231+
delete_tempfile(&temp);
255232

256-
ret |= !strstr(pbuf->buf, "\n[GNUPG:] GOODSIG ");
233+
ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
257234
strbuf_release(&buf); /* no matter it was used or not */
258235

259236
return ret;

‎run-command.c‎

Lines changed: 147 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -864,19 +864,161 @@ int run_hook_le(const char *const *env, const char *name, ...)
864864
return ret;
865865
}
866866

867-
int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)
867+
struct io_pump {
868+
/* initialized by caller */
869+
int fd;
870+
int type; /* POLLOUT or POLLIN */
871+
union {
872+
struct {
873+
const char *buf;
874+
size_t len;
875+
} out;
876+
struct {
877+
struct strbuf *buf;
878+
size_t hint;
879+
} in;
880+
} u;
881+
882+
/* returned by pump_io */
883+
int error; /* 0 for success, otherwise errno */
884+
885+
/* internal use */
886+
struct pollfd *pfd;
887+
};
888+
889+
static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
890+
{
891+
int pollsize = 0;
892+
int i;
893+
894+
for (i = 0; i < nr; i++) {
895+
struct io_pump *io = &slots[i];
896+
if (io->fd < 0)
897+
continue;
898+
pfd[pollsize].fd = io->fd;
899+
pfd[pollsize].events = io->type;
900+
io->pfd = &pfd[pollsize++];
901+
}
902+
903+
if (!pollsize)
904+
return 0;
905+
906+
if (poll(pfd, pollsize, -1) < 0) {
907+
if (errno == EINTR)
908+
return 1;
909+
die_errno("poll failed");
910+
}
911+
912+
for (i = 0; i < nr; i++) {
913+
struct io_pump *io = &slots[i];
914+
915+
if (io->fd < 0)
916+
continue;
917+
918+
if (!(io->pfd->revents & (POLLOUT|POLLIN|POLLHUP|POLLERR|POLLNVAL)))
919+
continue;
920+
921+
if (io->type == POLLOUT) {
922+
ssize_t len = xwrite(io->fd,
923+
io->u.out.buf, io->u.out.len);
924+
if (len < 0) {
925+
io->error = errno;
926+
close(io->fd);
927+
io->fd = -1;
928+
} else {
929+
io->u.out.buf += len;
930+
io->u.out.len -= len;
931+
if (!io->u.out.len) {
932+
close(io->fd);
933+
io->fd = -1;
934+
}
935+
}
936+
}
937+
938+
if (io->type == POLLIN) {
939+
ssize_t len = strbuf_read_once(io->u.in.buf,
940+
io->fd, io->u.in.hint);
941+
if (len < 0)
942+
io->error = errno;
943+
if (len <= 0) {
944+
close(io->fd);
945+
io->fd = -1;
946+
}
947+
}
948+
}
949+
950+
return 1;
951+
}
952+
953+
static int pump_io(struct io_pump *slots, int nr)
954+
{
955+
struct pollfd *pfd;
956+
int i;
957+
958+
for (i = 0; i < nr; i++)
959+
slots[i].error = 0;
960+
961+
ALLOC_ARRAY(pfd, nr);
962+
while (pump_io_round(slots, nr, pfd))
963+
; /* nothing */
964+
free(pfd);
965+
966+
/* There may be multiple errno values, so just pick the first. */
967+
for (i = 0; i < nr; i++) {
968+
if (slots[i].error) {
969+
errno = slots[i].error;
970+
return -1;
971+
}
972+
}
973+
return 0;
974+
}
975+
976+
977+
int pipe_command(struct child_process *cmd,
978+
const char *in, size_t in_len,
979+
struct strbuf *out, size_t out_hint,
980+
struct strbuf *err, size_t err_hint)
868981
{
869-
cmd->out = -1;
982+
struct io_pump io[3];
983+
int nr = 0;
984+
985+
if (in)
986+
cmd->in = -1;
987+
if (out)
988+
cmd->out = -1;
989+
if (err)
990+
cmd->err = -1;
991+
870992
if (start_command(cmd) < 0)
871993
return -1;
872994

873-
if (strbuf_read(buf, cmd->out, hint) < 0) {
874-
close(cmd->out);
995+
if (in) {
996+
io[nr].fd = cmd->in;
997+
io[nr].type = POLLOUT;
998+
io[nr].u.out.buf = in;
999+
io[nr].u.out.len = in_len;
1000+
nr++;
1001+
}
1002+
if (out) {
1003+
io[nr].fd = cmd->out;
1004+
io[nr].type = POLLIN;
1005+
io[nr].u.in.buf = out;
1006+
io[nr].u.in.hint = out_hint;
1007+
nr++;
1008+
}
1009+
if (err) {
1010+
io[nr].fd = cmd->err;
1011+
io[nr].type = POLLIN;
1012+
io[nr].u.in.buf = err;
1013+
io[nr].u.in.hint = err_hint;
1014+
nr++;
1015+
}
1016+
1017+
if (pump_io(io, nr) < 0) {
8751018
finish_command(cmd); /* throw away exit code */
8761019
return -1;
8771020
}
8781021

879-
close(cmd->out);
8801022
return finish_command(cmd);
8811023
}
8821024

‎run-command.h‎

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -79,17 +79,34 @@ int run_command_v_opt(const char **argv, int opt);
7979
int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);
8080

8181
/**
82-
* Execute the given command, capturing its stdout in the given strbuf.
82+
* Execute the given command, sending "in" to its stdin, and capturing its
83+
* stdout and stderr in the "out" and "err" strbufs. Any of the three may
84+
* be NULL to skip processing.
85+
*
8386
* Returns -1 if starting the command fails or reading fails, and otherwise
84-
* returns the exit code of the command. The output collected in the
85-
* buffer is kept even if the command returns a non-zero exit. The hint field
86-
* gives a starting size for the strbuf allocation.
87+
* returns the exit code of the command. Any output collected in the
88+
* buffers is kept even if the command returns a non-zero exit. The hint fields
89+
* gives starting sizes for the strbuf allocations.
8790
*
8891
* The fields of "cmd" should be set up as they would for a normal run_command
89-
* invocation. But note that there is no need to set cmd->out; the function
90-
* sets it up for the caller.
92+
* invocation. But note that there is no need to set the in, out, or err
93+
* fields; pipe_command handles that automatically.
94+
*/
95+
int pipe_command(struct child_process *cmd,
96+
const char *in, size_t in_len,
97+
struct strbuf *out, size_t out_hint,
98+
struct strbuf *err, size_t err_hint);
99+
100+
/**
101+
* Convenience wrapper around pipe_command for the common case
102+
* of capturing only stdout.
91103
*/
92-
int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint);
104+
static inline int capture_command(struct child_process *cmd,
105+
struct strbuf *out,
106+
size_t hint)
107+
{
108+
return pipe_command(cmd, NULL, 0, out, hint, NULL, 0);
109+
}
93110

94111
/*
95112
* The purpose of the following functions is to feed a pipe by running

‎t/t7004-tag.sh‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1202,10 +1202,17 @@ test_expect_success GPG,RFC1991 \
12021202
# try to sign with bad user.signingkey
12031203
git config user.signingkey BobTheMouse
12041204
test_expect_success GPG \
1205-
'git tag -s fails if gpg is misconfigured' \
1205+
'git tag -s fails if gpg is misconfigured (bad key)' \
12061206
'test_must_fail git tag -s -m tail tag-gpg-failure'
12071207
git config --unset user.signingkey
12081208

1209+
# try to produce invalid signature
1210+
test_expect_success GPG \
1211+
'git tag -s fails if gpg is misconfigured (bad signature format)' \
1212+
'test_config gpg.program echo &&
1213+
test_must_fail git tag -s -m tail tag-gpg-failure'
1214+
1215+
12091216
# try to verify without gpg:
12101217

12111218
rm -rf gpghome

0 commit comments

Comments
 (0)