diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 738839c1..9e6e0b5f 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -287,8 +287,10 @@ fn handle_connect( // this into one POST deadlocks a multi-round fetch (#117). // // git-receive-pack (push): - // Git sends ref-update commands + the complete PACK blob, then closes its - // write pipe. read_to_end is safe and correct here, a single POST. + // Git sends the ref-update command list (pkt-lines ending in a flush), + // then the complete PACK blob only when a command has a non-zero new-oid. + // A delete-only push sends no pack, and git holds its write pipe open for + // the report-status response, so EOF cannot delimit the request (#369). let post_url = format!("{}/{}", repo_base, service); @@ -296,10 +298,7 @@ fn handle_connect( return negotiate_upload_pack(&client, &post_url, service, signing_key, stdin, &mut stdout); } - let mut request_body = Vec::new(); - stdin - .read_to_end(&mut request_body) - .context("reading receive-pack request")?; + let request_body = read_receive_pack_request(stdin)?; tracing::debug!("pack request: {} bytes from git", request_body.len()); if request_body.is_empty() { @@ -535,6 +534,108 @@ fn read_upload_pack_round(stdin: &mut R) -> Result<(Vec, RoundEnd)> } } +/// Read one git-receive-pack request: optional `shallow` lines, then EITHER a +/// command list (pkt-lines terminated by a flush) or a push-cert block +/// (pkt-lines terminated by a "push-cert-end" line), then the negotiated +/// push-options block, then the pack ONLY when a command carries a non-zero +/// new-oid. +/// +/// A delete-only push sends no pack, and git holds its write pipe open waiting +/// for the report-status response, so a plain read_to_end deadlocks the two +/// (#369). A signed push is the other trap: when the client sends a push-cert +/// it sends NO command list at all (the commands live inside the cert), so the +/// request starts with a "push-cert" line and no flush ever arrives. The +/// negotiated capability list is on the push-cert line, or on the first +/// command line after its NUL when there is no cert. EOF at any point also +/// ends the read, covering git versions that close the pipe early. +fn read_receive_pack_request(stdin: &mut R) -> Result> { + /// Read one pkt-line into `body` (length prefix + data), returning its data + /// or `None` on flush or EOF. Malformed lengths bail rather than collapse + /// into a synthesized flush (#192 F5). + fn read_pkt(stdin: &mut R, body: &mut Vec) -> Result>> { + let mut len_bytes = [0u8; 4]; + match stdin.read_exact(&mut len_bytes) { + Ok(()) => {} + Err(e) if e.kind() == io::ErrorKind::UnexpectedEof => return Ok(None), + Err(e) => return Err(e.into()), + } + let Ok(len_hex) = std::str::from_utf8(&len_bytes) else { + bail!("malformed pkt-line length prefix: non-UTF-8 bytes {len_bytes:02x?}"); + }; + let Ok(pkt_len) = usize::from_str_radix(len_hex, 16) else { + bail!("malformed pkt-line length prefix: non-hex {len_hex:?}"); + }; + body.extend_from_slice(&len_bytes); + if pkt_len == 0 { + return Ok(None); + } + if pkt_len < 4 { + bail!("invalid pkt-line length: {pkt_len}"); + } + let mut data = vec![0u8; pkt_len - 4]; + stdin + .read_exact(&mut data) + .context("reading pkt-line data")?; + body.extend_from_slice(&data); + Ok(Some(data)) + } + + /// "<40-hex old> <40-hex new> " — the command shape, whether it + /// appears in a command list or inside a push-cert. + fn is_command(data: &[u8]) -> bool { + data.len() >= 82 + && data[..40].iter().all(|b| b.is_ascii_hexdigit()) + && data[40] == b' ' + && data[41..81].iter().all(|b| b.is_ascii_hexdigit()) + && data[81] == b' ' + } + + let mut body = Vec::new(); + let mut caps = String::new(); + let mut pack_expected = false; + let mut in_cert = false; + loop { + let Some(data) = read_pkt(stdin, &mut body)? else { + // Flush ends a command list; EOF ends anything. A cert has no + // legal flush inside it, so the same break covers malformed + // input without hanging. + break; + }; + if !in_cert && data.starts_with(b"push-cert") { + in_cert = true; + if let Some(nul) = data.iter().position(|&b| b == 0) { + caps = String::from_utf8_lossy(&data[nul + 1..]).into_owned(); + } + continue; + } + if in_cert && data == b"push-cert-end\n" { + break; + } + if is_command(&data) { + if caps.is_empty() { + if let Some(nul) = data.iter().position(|&b| b == 0) { + caps = String::from_utf8_lossy(&data[nul + 1..]).into_owned(); + } + } + if data[41..81].iter().any(|&b| b != b'0') { + pack_expected = true; + } + } + } + // When both are negotiated the push-options block follows the cert; when + // only options are negotiated it follows the command flush. Either way it + // is pkt-lines to a flush. + if caps.split(' ').any(|c| c.starts_with("push-options")) { + while read_pkt(stdin, &mut body)?.is_some() {} + } + if pack_expected { + stdin + .read_to_end(&mut body) + .context("reading receive-pack pack")?; + } + Ok(body) +} + /// POST one self-contained stateless-RPC request body and stream the response to /// `stdout`. Signs the request when `signing_key` is present, so every round of a /// private-repo fetch carries the caller's signature (the node visibility-gates @@ -2017,6 +2118,184 @@ mod tests { assert!(help.ends_with('\n')); } + // ── #369 delete-only push deadlock ─────────────────────────────────────── + + /// A delete-only receive-pack request ends at the command flush; git then + /// holds the write pipe open for the report and sends no pack. A reader that + /// blocks after the command list reproduces the hang that a Cursor (EOF) + /// hides. The read must return at the flush, inside the timeout. + #[test] + fn delete_only_receive_pack_request_returns_at_command_flush() { + let old = "a".repeat(40); + let zero = "0".repeat(40); + let mut seed = Vec::new(); + seed.extend_from_slice(&pkt(&format!("{old} {zero} refs/heads/gone\n"))); + seed.extend_from_slice(b"0000"); // command-list flush; git then blocks + + let (tx, rx) = std::sync::mpsc::channel::<()>(); + let mut reader = BlockAfterSeed { + seed: io::Cursor::new(seed), + gate: rx, + }; + + let (done_tx, done_rx) = std::sync::mpsc::channel::>(); + let handle = std::thread::spawn(move || { + let _ = done_tx.send(read_receive_pack_request(&mut reader).unwrap()); + }); + + let got = done_rx.recv_timeout(std::time::Duration::from_secs(2)); + let _ = tx.send(()); // unblock the reader so the thread can exit + let _ = handle.join(); + + let body = got.expect( + "read_receive_pack_request blocked past the command flush waiting for \ + a pack a delete-only push never sends (#369)", + ); + assert!( + body.ends_with(b"0000"), + "the request body keeps the command list and its flush terminator" + ); + } + + /// A push carrying objects still reads the pack through to EOF, and the + /// command flush sits between the commands and the pack in the body. + #[test] + fn receive_pack_request_with_pack_reads_through_eof() { + let old = "a".repeat(40); + let new = "b".repeat(40); + let mut stream = Vec::new(); + stream.extend_from_slice(&pkt(&format!( + "{old} {new} refs/heads/main\0 report-status-v2\n" + ))); + stream.extend_from_slice(b"0000"); + stream.extend_from_slice(b"PACK\x00\x00\x00\x02fakepack"); + + let mut stdin = io::Cursor::new(stream); + let body = read_receive_pack_request(&mut stdin).unwrap(); + let pos = body.windows(4).position(|w| w == b"0000").unwrap(); + assert_eq!( + &body[pos + 4..], + b"PACK\x00\x00\x00\x02fakepack", + "a non-zero new-oid means the pack follows the flush" + ); + } + + /// A signed delete-only push sends NO command list: the request starts + /// with the "push-cert" line, the commands live inside the cert, and the + /// cert ends with a "push-cert-end" pkt-line (not a flush). The reader + /// must return after push-cert-end without waiting on the pipe. + #[test] + fn signed_delete_only_push_returns_after_push_cert_end() { + let old = "a".repeat(40); + let zero = "0".repeat(40); + let mut seed = Vec::new(); + seed.extend_from_slice(&pkt("push-cert\0 report-status-v2 push-cert\n")); + seed.extend_from_slice(&pkt("certificate version 0.1\n")); + seed.extend_from_slice(&pkt("pusher did:key:z6Mk\n")); + seed.extend_from_slice(&pkt("\n")); // empty line ends the headers + seed.extend_from_slice(&pkt(&format!("{old} {zero} refs/heads/gone\n"))); + seed.extend_from_slice(&pkt("sig-line\n")); + seed.extend_from_slice(&pkt("push-cert-end\n")); + // git now blocks for the report; the pipe stays open. + + let (tx, rx) = std::sync::mpsc::channel::<()>(); + let mut reader = BlockAfterSeed { + seed: io::Cursor::new(seed), + gate: rx, + }; + + let (done_tx, done_rx) = std::sync::mpsc::channel::>(); + let handle = std::thread::spawn(move || { + let _ = done_tx.send(read_receive_pack_request(&mut reader).unwrap()); + }); + let got = done_rx.recv_timeout(std::time::Duration::from_secs(2)); + let _ = tx.send(()); + let _ = handle.join(); + + let body = got.expect( + "read_receive_pack_request blocked past push-cert-end on a signed \ + delete-only push (#369)", + ); + assert!( + body.windows(6).any(|w| w == b"pusher"), + "the cert block is part of the request body" + ); + } + + /// A signed pack-bearing push: cert with the commands inside, then the + /// pack. Everything must land in the body. + #[test] + fn signed_pack_push_keeps_cert_and_pack() { + let old = "a".repeat(40); + let new = "b".repeat(40); + let mut stream = Vec::new(); + stream.extend_from_slice(&pkt("push-cert\0 report-status-v2 push-cert\n")); + stream.extend_from_slice(&pkt("certificate version 0.1\n")); + stream.extend_from_slice(&pkt("\n")); + stream.extend_from_slice(&pkt(&format!("{old} {new} refs/heads/main\n"))); + stream.extend_from_slice(&pkt("push-cert-end\n")); + stream.extend_from_slice(b"PACK\x00\x00\x00\x02fakepack"); + + let mut stdin = io::Cursor::new(stream); + let body = read_receive_pack_request(&mut stdin).unwrap(); + assert!( + body.windows(5).any(|w| w == b"PACK\x00"), + "the pack follows the cert into the body" + ); + } + + /// A negotiated push-options block is pkt-lines ending in its own flush. + /// With no cert it follows the command flush; with a cert it follows + /// push-cert-end. + #[test] + fn receive_pack_request_consumes_a_push_options_block() { + let old = "a".repeat(40); + let new = "b".repeat(40); + let mut stream = Vec::new(); + stream.extend_from_slice(&pkt(&format!( + "{old} {new} refs/heads/main\0 report-status-v2 push-options\n" + ))); + stream.extend_from_slice(b"0000"); // command flush + stream.extend_from_slice(&pkt("ci.skip\n")); + stream.extend_from_slice(b"0000"); // options flush + stream.extend_from_slice(b"PACK\x00\x00\x00\x02fakepack"); + + let mut stdin = io::Cursor::new(stream); + let body = read_receive_pack_request(&mut stdin).unwrap(); + assert!(body.windows(7).any(|w| w == b"ci.skip"), "options survived"); + assert!( + body.ends_with(b"PACK\x00\x00\x00\x02fakepack"), + "the pack lands after the options block" + ); + } + + /// Cert and options together: the cert comes first and the options block + /// sits between push-cert-end and the pack. + #[test] + fn signed_push_with_options_reads_cert_then_options_then_pack() { + let old = "a".repeat(40); + let new = "b".repeat(40); + let mut stream = Vec::new(); + stream.extend_from_slice(&pkt( + "push-cert\0 report-status-v2 push-cert push-options\n", + )); + stream.extend_from_slice(&pkt("certificate version 0.1\n")); + stream.extend_from_slice(&pkt("\n")); + stream.extend_from_slice(&pkt(&format!("{old} {new} refs/heads/main\n"))); + stream.extend_from_slice(&pkt("push-cert-end\n")); + stream.extend_from_slice(&pkt("ci.skip\n")); + stream.extend_from_slice(b"0000"); // options flush + stream.extend_from_slice(b"PACK\x00\x00\x00\x02fakepack"); + + let mut stdin = io::Cursor::new(stream); + let body = read_receive_pack_request(&mut stdin).unwrap(); + assert!(body.windows(7).any(|w| w == b"ci.skip"), "options survived"); + assert!( + body.ends_with(b"PACK\x00\x00\x00\x02fakepack"), + "the pack lands after the options block" + ); + } + // ── #117 multi-round fetch negotiation ─────────────────────────────────── /// Encode a git pkt-line: 4-byte hex length (incl. the 4 bytes) + data. diff --git a/crates/git-remote-gitlawb/tests/real_git_fetch.rs b/crates/git-remote-gitlawb/tests/real_git_fetch.rs index 72fdd5c2..d6d66db7 100644 --- a/crates/git-remote-gitlawb/tests/real_git_fetch.rs +++ b/crates/git-remote-gitlawb/tests/real_git_fetch.rs @@ -95,6 +95,36 @@ fn upload_pack(repo: &Path, advertise: bool, input: &[u8]) -> Vec { out.stdout } +/// Run `git receive-pack --stateless-rpc [--advertise-refs] `, optionally +/// feeding `input` on stdin. Returns raw stdout (the wire bytes). The receive +/// half of `upload_pack`, used so the shim can serve a real push. +fn receive_pack(repo: &Path, advertise: bool, input: &[u8]) -> Vec { + let mut cmd = Command::new("git"); + cmd.arg("receive-pack").arg("--stateless-rpc"); + if advertise { + cmd.arg("--advertise-refs"); + } + cmd.arg(repo) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + let mut child = cmd.spawn().expect("spawn git receive-pack"); + let mut stdin = child.stdin.take().unwrap(); + let input = input.to_vec(); + let writer = std::thread::spawn(move || { + let _ = stdin.write_all(&input); + // drop closes stdin + }); + let out = child.wait_with_output().expect("wait receive-pack"); + writer.join().ok(); + assert!( + out.status.success(), + "git receive-pack failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + out.stdout +} + #[derive(Clone, Copy)] enum ShimMode { /// Faithful v0 serving: pipe each POST to `git upload-pack --stateless-rpc`. @@ -218,7 +248,21 @@ fn handle_conn(stream: TcpStream, repo: &Path, mode: ShimMode, posts: &AtomicUsi reader.read_exact(&mut body).ok(); } - let (content_type, payload) = if method == "GET" && target.contains("/info/refs") { + let (content_type, payload) = if method == "GET" && target.contains("service=git-receive-pack") + { + // Receive-pack advertisement, same wrapper shape as upload-pack. + let adv = receive_pack(repo, true, b""); + let mut wrapped = pkt(b"# service=git-receive-pack\n"); + wrapped.extend_from_slice(b"0000"); + wrapped.extend_from_slice(&adv); + ("application/x-git-receive-pack-advertisement", wrapped) + } else if method == "POST" && target.ends_with("/git-receive-pack") { + posts.fetch_add(1, Ordering::SeqCst); + ( + "application/x-git-receive-pack-result", + receive_pack(repo, false, &body), + ) + } else if method == "GET" && target.contains("/info/refs") { // v0 advertisement, wrapped exactly as the node's info_refs does. let adv = upload_pack(repo, true, b""); let mut wrapped = pkt(b"# service=git-upload-pack\n"); @@ -293,6 +337,34 @@ fn fetch_with_helper(clone: &Path, node_url: &str) -> (bool, std::process::Outpu run_bounded(cmd, Duration::from_secs(30)) } +/// Run `git push` in `local` through the helper, same wiring and hard timeout as +/// [`fetch_with_helper`]. `args` carries the push spec, e.g. +/// `["--delete", "doomed"]`. +fn push_with_helper(local: &Path, node_url: &str, args: &[&str]) -> (bool, std::process::Output) { + let helper_bin = PathBuf::from(env!("CARGO_BIN_EXE_git-remote-gitlawb")); + let helper_dir = helper_bin.parent().unwrap().to_path_buf(); + let path_env = match std::env::var_os("PATH") { + Some(p) => { + let mut dirs = vec![helper_dir.clone()]; + dirs.extend(std::env::split_paths(&p)); + std::env::join_paths(dirs).unwrap() + } + None => helper_dir.clone().into_os_string(), + }; + + let mut cmd = Command::new("git"); + cmd.arg("-C") + .arg(local) + .arg("push") + .arg("origin") + .args(args) + .env("PATH", path_env) + .env("LC_ALL", "C") + .env("GITLAWB_NODE", node_url) + .env("GITLAWB_KEY", "/nonexistent-key-for-anon-push"); + run_bounded(cmd, Duration::from_secs(30)) +} + /// A synthetic non-success `ExitStatus` used only on the unix timeout path, where /// `reap_group` has already consumed the child and the later `wait` returns ECHILD. /// The timeout branch already reports `completed == false`, so the exact status is @@ -1242,3 +1314,77 @@ fn run_bounded_bounds_join_when_leader_exits_leaving_a_pipe_holder() { held pipes do not stall the joins)" ); } + +/// #369 end-to-end: a real `git push --delete` through the helper completes. +/// The receive-pack request for a delete-only push is commands + flush with NO +/// pack, and git holds its write pipe open for the report — so a helper that +/// reads stdin to EOF deadlocks against it. The shim pipes the POST to a real +/// `git receive-pack --stateless-rpc`, so the branch actually has to be deleted +/// server-side for the assertions to pass. +#[test] +fn real_git_push_delete_completes() { + let dir = tempfile::tempdir().unwrap(); + let server = dir.path().join("server.git"); + let local = dir.path().join("local"); + + // A bare repo holding the doomed branch, plus a live one. + git( + dir.path(), + &[ + "init", + "--bare", + server.to_str().unwrap(), + "--initial-branch=main", + ], + ); + let work = dir.path().join("work"); + git( + dir.path(), + &["init", work.to_str().unwrap(), "--initial-branch=main"], + ); + git(&work, &["config", "user.email", "t@t"]); + git(&work, &["config", "user.name", "t"]); + std::fs::write(work.join("f"), "x").unwrap(); + git(&work, &["add", "f"]); + git(&work, &["commit", "-m", "c"]); + git(&work, &["branch", "doomed"]); + git(&work, &["push", server.to_str().unwrap(), "main", "doomed"]); + + // The pusher: a local repo whose origin is the gitlawb remote pointing at + // the shim. Nothing needs to be fetched first; git takes the old-oid from + // the receive-pack advertisement. + git( + dir.path(), + &["init", local.to_str().unwrap(), "--initial-branch=main"], + ); + git( + &local, + &["remote", "add", "origin", "gitlawb://did:key:z6MkTest/repo"], + ); + + let shim = start_shim(server.clone(), ShimMode::Normal); + let (completed, out) = push_with_helper(&local, &shim.base_url, &["--delete", "doomed"]); + let posts = shim.posts.load(Ordering::SeqCst); + + assert!( + completed, + "git push --delete did not complete within the timeout (deadlock signature). stderr:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + assert!( + out.status.success(), + "git push --delete failed. stderr:\n{}", + String::from_utf8_lossy(&out.stderr) + ); + assert_eq!(posts, 1, "a delete-only push is exactly one POST"); + let gone = Command::new("git") + .arg("-C") + .arg(&server) + .args(["rev-parse", "--verify", "--quiet", "refs/heads/doomed"]) + .output() + .unwrap(); + assert!( + !gone.status.success(), + "refs/heads/doomed must be deleted in the server repo" + ); +}