git/send-pack.c, branch v2.9.2

send-pack: isolate sigpipe in demuxer thread

2016-04-20T20:33:53Z

If we get an error from pack-objects, we may exit send_pack() early, before reading the server's status response. In such a case, we may racily see SIGPIPE from our async demuxer (which is trying to write that status back to us), and we'd prefer to continue pushing the error up the call stack, rather than taking down the whole process with signal death. This is safe to do because our demuxer just calls recv_sideband, whose data writes are all done with write_or_die(), which will notice SIGPIPE. We do also write sideband 2 to stderr, and we would no longer die on SIGPIPE there (if it were piped in the first place, and if the piped program went away). But that's probably a good thing, as it likewise should not abort the push process at all (neither immediately by signal, nor eventually by reporting failure back to the main thread). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

send-pack: close demux pipe before finishing async process

2016-04-20T20:33:53Z

This fixes a deadlock on the client side when pushing a large number of refs from a corrupted repo. There's a reproduction script below, but let's start with a human-readable explanation. The client side of a push goes something like this: 1. Start an async process to demux sideband coming from the server. 2. Run pack-objects to send the actual pack, and wait for its status via finish_command(). 3. If pack-objects failed, abort immediately. 4. If pack-objects succeeded, read the per-ref status from the server, which is actually coming over a pipe from the demux process started in step 1. We run finish_async() to wait for and clean up the demux process in two places. In step 3, if we see an error, we want it to end early. And after step 4, it should be done writing any data and we are just cleaning it up. Let's focus on the error case first. We hand the output descriptor to the server over to pack-objects. So by the time it has returned an error to us, it has closed the descriptor and the server has gotten EOF. The server will mark all refs as failed with "unpacker error" and send us back the status for each (followed by EOF). This status goes to the demuxer thread, which relays it over a pipe to the main thread. But the main thread never even tries reading the status. It's trying to bail because of the pack-objects error, and is waiting for the demuxer thread to finish. If there are a small number of refs, that's OK; the demuxer thread writes into the pipe buffer, sees EOF from the server, and quits. But if there are a large number of refs, it may block on write() back to the main thread, leading to a deadlock (the main thread is waiting for the demuxer to finish, the demuxer is waiting for the main thread to read). We can break this deadlock by closing the pipe between the demuxer and the main thread before calling finish_async(). Then the demuxer gets a write() error and exits. The non-error case usually just works, because we will have read all of the data from the other side. We do close demux.out already, but we only do so _after_ calling finish_async(). This is OK because there shouldn't be any more data coming from the server. But technically we've only read to a flush packet, and a broken or malicious server could be sending more cruft. In such a case, we would hit the same deadlock. Closing the pipe first doesn't affect the normal case, and means that for a cruft-sending server, we'll notice a write() error rather than deadlocking. Note that when write() sees this error, we'll actually deliver SIGPIPE to the thread, which will take down the whole process (unless we're compiled with NO_PTHREADS). This isn't ideal, but it's an improvement over the status quo, which is deadlocking. And SIGPIPE handling in async threads is a bigger problem that we can deal with separately. A simple reproduction for the error case is below. It's technically racy (we could exit the main process and take down the async thread with us before it even reads the status), though in practice it seems to fail pretty consistently. git init repo && cd repo && # make some commits; we need two so we can simulate corruption # in the history later. git commit --allow-empty -m one && one=$(git rev-parse HEAD) && git commit --allow-empty -m two && two=$(git rev-parse HEAD) && # now make a ton of refs; our goal here is to overflow the pipe buffer # when reporting the ref status, which will cause the demuxer to block # on write() for i in $(seq 20000); do echo "create refs/heads/this-is-a-really-long-branch-name-$i $two" done | git update-ref --stdin && # now make a corruption in the history such that pack-objects will fail rm -vf .git/objects/$(echo $one | sed 's}..}&/}') && # and then push the result git init --bare dst.git && git push --mirror dst.git Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

Convert struct ref to use object_id.

2015-11-20T13:02:05Z

Use struct object_id in three fields in struct ref and convert all the necessary places that use it. Signed-off-by: brian m. carlson Signed-off-by: Jeff King

Merge branch 'db/push-sign-if-asked'

2015-08-31T22:39:08Z

The client side codepaths in "git push" have been cleaned up and the user can request to perform an optional "signed push", i.e. sign only when the other end accepts signed push. * db/push-sign-if-asked: push: add a config option push.gpgSign for default signed pushes push: support signing pushes iff the server supports it builtin/send-pack.c: use parse_options API config.c: rename git_config_maybe_bool_text and export it as git_parse_maybe_bool transport: remove git_transport_options.push_cert gitremote-helpers.txt: document pushcert option Documentation/git-send-pack.txt: document --signed Documentation/git-send-pack.txt: wrap long synopsis line Documentation/git-push.txt: document when --signed may fail

push: support signing pushes iff the server supports it

2015-08-19T19:58:45Z

Add a new flag --sign=true (or --sign=false), which means the same thing as the original --signed (or --no-signed). Give it a third value --sign=if-asked to tell push and send-pack to send a push certificate if and only if the server advertised a push cert nonce. If not, warn the user that their push may not be as secure as they thought. Signed-off-by: Dave Borowitz Signed-off-by: Junio C Hamano

Merge branch 'bc/object-id'

2015-05-06T04:00:23Z

Identify parts of the code that knows that we use SHA-1 hash to name our objects too much, and use (1) symbolic constants instead of hardcoded 20 as byte count and/or (2) use struct object_id instead of unsigned char [20] for object names. * bc/object-id: apply: convert threeway_stage to object_id patch-id: convert to use struct object_id commit: convert parts to struct object_id diff: convert struct combine_diff_path to object_id bulk-checkin.c: convert to use struct object_id zip: use GIT_SHA1_HEXSZ for trailers archive.c: convert to use struct object_id bisect.c: convert leaf functions to use struct object_id define utility functions for object IDs define a structure for object IDs

Merge branch 'jc/push-cert'

2015-04-20T22:28:31Z

The "git push --signed" protocol extension did not limit what the "nonce" that is a server-chosen string can contain or how long it can be, which was unnecessarily lax. Limit both the length and the alphabet to a reasonably small space that can still have enough entropy. * jc/push-cert: push --signed: tighten what the receiving end can ask to sign

Merge branch 'sb/atomic-push'

2015-04-02T19:34:43Z

* sb/atomic-push: send-pack: unify error messages for unsupported capabilities

push --signed: tighten what the receiving end can ask to sign

2015-04-02T18:05:18Z

Instead of blindly trusting the receiving side to give us a sensible nonce to sign, limit the length (max 256 bytes) and the alphabet (alnum and a few selected punctuations, enough to encode in base64) that can be used in nonce. Signed-off-by: Junio C Hamano

send-pack: unify error messages for unsupported capabilities

2015-04-02T18:02:52Z

If --signed is not supported, the error message names the remote "receiving end". If --atomic is not supported, the error message names the remote "server". Unify the naming to "receiving end" as we're in the context of "push". Signed-off-by: Ralf Thielow Signed-off-by: Junio C Hamano