mirror of
https://relay.ngit.dev/npub15qydau2hjma6ngxkl2cyar74wzyjshvl65za5k5rl69264ar2exs5cyejr/ngit-grasp.git
synced 2026-10-05 15:08:24 +00:00
fix(git): reject incomplete receive-pack success responses
The merged status path could synthesize a complete success after truncated Git output or release buffered success statuses despite a failed subprocess exit. Require both inner and outer report flushes and a successful process before completing the response. Treat these as transport failures, without claiming that refs already written were rolled back. Valid native per-ref rejections retain their existing semantics. Signed and noncanonical request passthrough remains outside the merged report path. Validation: reproduced truncation and failed-exit false successes before fixing them. All 958 library tests, 209 selected integration tests, the final 11 receive-pack planner tests, and strict all-target Clippy passed. Assisted-by: GPT-6
This commit is contained in:
@@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
|
|
||||||
### Fixed
|
### Fixed
|
||||||
|
|
||||||
|
- Fail interrupted push responses instead of completing a success report when
|
||||||
|
Git emits truncated status data or exits unsuccessfully.
|
||||||
|
|
||||||
- Drain Git output while uploading push data, preventing pipe deadlocks when
|
- Drain Git output while uploading push data, preventing pipe deadlocks when
|
||||||
receive-pack produces progress or diagnostics before consuming the full pack.
|
receive-pack produces progress or diagnostics before consuming the full pack.
|
||||||
|
|
||||||
|
|||||||
@@ -207,6 +207,8 @@ See [`src/git/handlers.rs:22-98`](src/git/handlers.rs:22-98) for the info-refs i
|
|||||||
Receive-pack feeds stdin while concurrently draining stdout and stderr, so
|
Receive-pack feeds stdin while concurrently draining stdout and stderr, so
|
||||||
progress or diagnostics cannot deadlock a large upload on full pipes. Its task
|
progress or diagnostics cannot deadlock a large upload on full pipes. Its task
|
||||||
reaps the child before releasing verification locks on cancellation or I/O error.
|
reaps the child before releasing verification locks on cancellation or I/O error.
|
||||||
|
Merged push reports require complete native status framing and a successful Git
|
||||||
|
exit; transport failures cannot manufacture a terminal success report.
|
||||||
|
|
||||||
The upload-pack streaming task retains a repository tracing span. Unexpected
|
The upload-pack streaming task retains a repository tracing span. Unexpected
|
||||||
process failures include the exit status and stream outcome even when Git
|
process failures include the exit status and stream outcome even when Git
|
||||||
|
|||||||
@@ -950,6 +950,22 @@ async fn stream_receive_pack_output<S, E, I>(
|
|||||||
};
|
};
|
||||||
|
|
||||||
if !status.success() {
|
if !status.success() {
|
||||||
|
if push_plan.is_some() {
|
||||||
|
// The merged report is still buffered. A failed process must not
|
||||||
|
// complete the push response, even if some ref updates succeeded.
|
||||||
|
record_git_operation(&metrics, "push", "error");
|
||||||
|
error!(
|
||||||
|
"Git receive-pack failed with {status}: {}",
|
||||||
|
String::from_utf8_lossy(&stderr_output)
|
||||||
|
);
|
||||||
|
let _ = tx
|
||||||
|
.send(Err(io::Error::other(format!(
|
||||||
|
"git receive-pack failed with {status}"
|
||||||
|
))))
|
||||||
|
.await;
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
if let Some(flush) = terminal_flush {
|
if let Some(flush) = terminal_flush {
|
||||||
sent_stdout = true;
|
sent_stdout = true;
|
||||||
if send_body_bytes(&tx, flush).await.is_err() {
|
if send_body_bytes(&tx, flush).await.is_err() {
|
||||||
|
|||||||
@@ -329,7 +329,10 @@ impl ReceivePackPlan {
|
|||||||
let mut header = [0; 4];
|
let mut header = [0; 4];
|
||||||
let first = stdout.read(&mut header[..1]).await?;
|
let first = stdout.read(&mut header[..1]).await?;
|
||||||
if first == 0 {
|
if first == 0 {
|
||||||
break;
|
return Err(io::Error::new(
|
||||||
|
io::ErrorKind::UnexpectedEof,
|
||||||
|
"Git status ended before flush",
|
||||||
|
));
|
||||||
}
|
}
|
||||||
stdout.read_exact(&mut header[1..]).await?;
|
stdout.read_exact(&mut header[1..]).await?;
|
||||||
let len = std::str::from_utf8(&header)
|
let len = std::str::from_utf8(&header)
|
||||||
@@ -337,6 +340,9 @@ impl ReceivePackPlan {
|
|||||||
.and_then(|s| usize::from_str_radix(s, 16).ok())
|
.and_then(|s| usize::from_str_radix(s, 16).ok())
|
||||||
.ok_or_else(|| io::Error::other("invalid Git status framing"))?;
|
.ok_or_else(|| io::Error::other("invalid Git status framing"))?;
|
||||||
if len == 0 {
|
if len == 0 {
|
||||||
|
if !self.sideband {
|
||||||
|
report.extend(b"0000");
|
||||||
|
}
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
if !(5..=65520).contains(&len) {
|
if !(5..=65520).contains(&len) {
|
||||||
@@ -371,6 +377,21 @@ impl ReceivePackPlan {
|
|||||||
return Err(io::Error::other("Git status report exceeds command budget"));
|
return Err(io::Error::other("Git status report exceeds command budget"));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// Sideband can split the inner report at any byte. Validate its
|
||||||
|
// complete framing before manufacturing a merged terminal response.
|
||||||
|
let mut remaining = report.as_slice();
|
||||||
|
loop {
|
||||||
|
let (packet, tail) = PktLine::parse(remaining).map_err(|_| {
|
||||||
|
io::Error::new(io::ErrorKind::UnexpectedEof, "incomplete Git status report")
|
||||||
|
})?;
|
||||||
|
remaining = tail;
|
||||||
|
if packet == PktLine::Flush {
|
||||||
|
if !remaining.is_empty() {
|
||||||
|
return Err(io::Error::other("trailing Git status data"));
|
||||||
|
}
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
Ok(())
|
Ok(())
|
||||||
};
|
};
|
||||||
let result: io::Result<()> = tokio::select! {
|
let result: io::Result<()> = tokio::select! {
|
||||||
@@ -659,6 +680,46 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn truncated_reports_are_transport_errors_not_synthetic_successes() {
|
||||||
|
for sideband in [false, true] {
|
||||||
|
for missing_inner_flush in [false, true] {
|
||||||
|
let (dir, oid) = fixture();
|
||||||
|
let caps = if sideband {
|
||||||
|
"report-status side-band-64k"
|
||||||
|
} else {
|
||||||
|
"report-status"
|
||||||
|
};
|
||||||
|
let mut plan =
|
||||||
|
ReceivePackPlan::prepare(&request(&oid, caps, false), &HashMap::new()).unwrap();
|
||||||
|
plan.verify(dir.path());
|
||||||
|
let mut report = native_report("ok refs/heads/feature\n");
|
||||||
|
if missing_inner_flush || !sideband {
|
||||||
|
report.truncate(report.len() - 4);
|
||||||
|
}
|
||||||
|
let bytes = if sideband {
|
||||||
|
let mut payload = vec![1];
|
||||||
|
payload.extend(report);
|
||||||
|
let mut bytes = PktLine::data(payload).encode();
|
||||||
|
if missing_inner_flush {
|
||||||
|
bytes.extend(b"0000");
|
||||||
|
}
|
||||||
|
bytes
|
||||||
|
} else {
|
||||||
|
report
|
||||||
|
};
|
||||||
|
let (tx, mut rx) = mpsc::channel(4);
|
||||||
|
let (outcome, response) =
|
||||||
|
timeout(Duration::from_secs(5), plan.pump(bytes.as_slice(), &tx))
|
||||||
|
.await
|
||||||
|
.expect("truncated report should terminate");
|
||||||
|
assert_eq!(outcome, PumpResult::ReadError);
|
||||||
|
assert!(response.is_none());
|
||||||
|
assert!(rx.try_recv().unwrap().is_err());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn sideband_progress_streams_before_final_status_and_status_v2_survives() {
|
async fn sideband_progress_streams_before_final_status_and_status_v2_survives() {
|
||||||
let (dir, oid) = fixture();
|
let (dir, oid) = fixture();
|
||||||
|
|||||||
@@ -61,6 +61,15 @@ impl Drop for PathOverride {
|
|||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn receive_pack_response_streams_stdout_before_subprocess_exit() {
|
async fn receive_pack_response_streams_stdout_before_subprocess_exit() {
|
||||||
|
exercise_receive_pack_stream(false).await;
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn receive_pack_failed_exit_does_not_emit_a_buffered_success_report() {
|
||||||
|
exercise_receive_pack_stream(true).await;
|
||||||
|
}
|
||||||
|
|
||||||
|
async fn exercise_receive_pack_stream(failed_exit: bool) {
|
||||||
let _env_lock = PATH_ENV_LOCK.lock().await;
|
let _env_lock = PATH_ENV_LOCK.lock().await;
|
||||||
|
|
||||||
let fake_bin = tempfile::tempdir().expect("fake git bin tempdir");
|
let fake_bin = tempfile::tempdir().expect("fake git bin tempdir");
|
||||||
@@ -69,6 +78,11 @@ async fn receive_pack_response_streams_stdout_before_subprocess_exit() {
|
|||||||
// Force output before consuming an upload larger than a pipe buffer.
|
// Force output before consuming an upload larger than a pipe buffer.
|
||||||
let script_path = fake_bin.path().join("git");
|
let script_path = fake_bin.path().join("git");
|
||||||
let script = std::fs::read_to_string(&script_path).unwrap();
|
let script = std::fs::read_to_string(&script_path).unwrap();
|
||||||
|
let script = if failed_exit {
|
||||||
|
script.replacen("exit 0", "exit 1", 1)
|
||||||
|
} else {
|
||||||
|
script
|
||||||
|
};
|
||||||
std::fs::write(
|
std::fs::write(
|
||||||
&script_path,
|
&script_path,
|
||||||
script
|
script
|
||||||
@@ -110,6 +124,19 @@ async fn receive_pack_response_streams_stdout_before_subprocess_exit() {
|
|||||||
let mut streamed = read_first_progress(&mut body).await;
|
let mut streamed = read_first_progress(&mut body).await;
|
||||||
// Git cannot exit until this test releases it, regardless of scheduling.
|
// Git cannot exit until this test releases it, regardless of scheduling.
|
||||||
git_gate.release().await;
|
git_gate.release().await;
|
||||||
|
if failed_exit {
|
||||||
|
timeout(Duration::from_secs(3), async {
|
||||||
|
while let Ok(frame) = body.frame().await.expect("transport error before EOF") {
|
||||||
|
streamed.extend_from_slice(&frame_data(frame));
|
||||||
|
}
|
||||||
|
})
|
||||||
|
.await
|
||||||
|
.expect("failed Git exit should fail the response");
|
||||||
|
assert!(!streamed
|
||||||
|
.windows(b"unpack ok".len())
|
||||||
|
.any(|part| part == b"unpack ok"));
|
||||||
|
return;
|
||||||
|
}
|
||||||
finish_body(&mut body, &mut streamed).await;
|
finish_body(&mut body, &mut streamed).await;
|
||||||
assert_eq!(streamed, expected_git_output());
|
assert_eq!(streamed, expected_git_output());
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user