Skip to content

Commit f8c9e76

Browse files
author
Roy Lin
committed
fix(cri): close stdin / send port-forward CLOSE on streaming error paths
Three confirmed cleanup-on-error gaps from the CRI audit, hand-verified: - handle_attach_stream: the stdin_once close-cleanup ran only on the clean loop break; every error path (?/return Err on tcp read, stdin write, or tcp write) returned early and skipped it, leaking an open stdin on an abnormal client disconnect. Convert the error returns to break-with-stored-error so the cleanup runs on every exit, then return the error. - spdy serve_attach reader_task: the loop also breaks on a frame-read error/EOF before a fin stdin frame, but only closed stdin on the fin path. Close stdin after the loop too (stdin_once). - handle_port_forward_stream tcp_to_guest: the CLOSE frame was sent only on a clean client close (n==0); a read/write error left the guest's TCP connection open waiting for data. Send CLOSE on every exit path (best-effort). clippy -D warnings clean; 237 cri unit tests pass.
1 parent a3e02d8 commit f8c9e76

2 files changed

Lines changed: 57 additions & 21 deletions

File tree

src/cri/src/spdy.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -618,6 +618,14 @@ pub async fn serve_attach(
618618
Ok(None) | Err(_) => break,
619619
}
620620
}
621+
// The loop also exits on a frame-read error (or EOF) before a `fin`
622+
// stdin frame; close stdin here so a stdin_once attach doesn't leak an
623+
// open stdin on an abnormal client disconnect.
624+
if stdin_once && !closed {
625+
if let Some(input) = stdin.as_ref() {
626+
let _ = input.close_stdin().await;
627+
}
628+
}
621629
}
622630
};
623631

src/cri/src/streaming.rs

Lines changed: 49 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -600,6 +600,10 @@ async fn handle_attach_stream(
600600
let mut input_buffer = vec![0u8; 16 * 1024];
601601
let (mut tcp_read, mut tcp_write) = tokio::io::split(stream);
602602

603+
// Track any I/O error so the stdin_once close-cleanup below runs on EVERY
604+
// exit path (including an abnormal client disconnect / write error), not only
605+
// the clean break — otherwise a stdin_once attach leaks an open stdin.
606+
let mut loop_result: Result<(), Box<dyn std::error::Error + Send + Sync>> = Ok(());
603607
loop {
604608
tokio::select! {
605609
input = tcp_read.read(&mut input_buffer), if !stdin_closed => {
@@ -608,7 +612,10 @@ async fn handle_attach_stream(
608612
stdin_closed = true;
609613
if session.stdin_once {
610614
if let Some(stdin) = stdin.as_ref() {
611-
stdin.close_stdin().await?;
615+
if let Err(e) = stdin.close_stdin().await {
616+
loop_result = Err(e);
617+
break;
618+
}
612619
}
613620
}
614621
if !session.stdout && !session.stderr {
@@ -617,20 +624,32 @@ async fn handle_attach_stream(
617624
}
618625
Ok(n) => {
619626
if let Some(stdin) = stdin.as_ref() {
620-
stdin.write_stdin(&input_buffer[..n]).await?;
627+
if let Err(e) = stdin.write_stdin(&input_buffer[..n]).await {
628+
loop_result = Err(e);
629+
break;
630+
}
621631
}
622632
}
623-
Err(e) => return Err(Box::new(e)),
633+
Err(e) => {
634+
loop_result = Err(Box::new(e));
635+
break;
636+
}
624637
}
625638
}
626639
event = receiver.recv() => {
627640
match event {
628641
Ok(a3s_box_core::exec::ExecEvent::Chunk(chunk)) => match chunk.stream {
629642
a3s_box_core::exec::StreamType::Stdout if session.stdout => {
630-
tcp_write.write_all(&chunk.data).await?;
643+
if let Err(e) = tcp_write.write_all(&chunk.data).await {
644+
loop_result = Err(Box::new(e));
645+
break;
646+
}
631647
}
632648
a3s_box_core::exec::StreamType::Stderr if session.stderr => {
633-
tcp_write.write_all(&chunk.data).await?;
649+
if let Err(e) = tcp_write.write_all(&chunk.data).await {
650+
loop_result = Err(Box::new(e));
651+
break;
652+
}
634653
}
635654
_ => {}
636655
},
@@ -655,7 +674,7 @@ async fn handle_attach_stream(
655674
}
656675
}
657676

658-
Ok(())
677+
loop_result
659678
}
660679

661680
/// Handle port-forward streaming: TCP proxy to guest ports.
@@ -732,28 +751,37 @@ async fn handle_port_forward_stream(
732751

733752
let tcp_to_guest = async {
734753
let mut buf = vec![0u8; 4096];
735-
loop {
736-
let n = tcp_read.read(&mut buf).await?;
754+
let result = loop {
755+
let n = match tcp_read.read(&mut buf).await {
756+
Ok(n) => n,
757+
Err(e) => break Err(e),
758+
};
737759
if n == 0 {
738-
write_port_forward_frame(
739-
&mut control_write,
740-
PORT_FORWARD_FRAME_CLOSE,
741-
PORT_FORWARD_STREAM_ID,
742-
&[],
743-
)
744-
.await?;
745-
break;
760+
break Ok::<(), std::io::Error>(());
746761
}
747-
748-
write_port_forward_frame(
762+
if let Err(e) = write_port_forward_frame(
749763
&mut control_write,
750764
PORT_FORWARD_FRAME_DATA,
751765
PORT_FORWARD_STREAM_ID,
752766
&buf[..n],
753767
)
754-
.await?;
755-
}
756-
Ok::<_, std::io::Error>(())
768+
.await
769+
{
770+
break Err(e);
771+
}
772+
};
773+
// Tell the guest the forward ended on EVERY exit path (clean client close
774+
// OR a client-side read/write error), so it doesn't leak the guest TCP
775+
// connection waiting for more data. Best-effort: a dead control channel
776+
// makes this a no-op.
777+
let _ = write_port_forward_frame(
778+
&mut control_write,
779+
PORT_FORWARD_FRAME_CLOSE,
780+
PORT_FORWARD_STREAM_ID,
781+
&[],
782+
)
783+
.await;
784+
result
757785
};
758786

759787
let guest_to_tcp = async {

0 commit comments

Comments
 (0)