Bug#1050334: bookworm-pu: package reprepro/5.3.1-1+deb12u1
Package: release.debian.org
Severity: normal
Tags: bookworm
User: release.debian.org@packages.debian.org
Usertags: pu
X-Debbugs-Cc: reprepro@packages.debian.org, bage@debian.org
Control: affects -1 + src:reprepro
reprepro uses internal decompressors for most compression formats except
zstd. When dealing with zstd compressed .debs (such as those for
Ubuntu), decompression may fail due to a race condition. Naturally, this
bug originally surfaced in Ubuntu as
https://bugs.launchpad.net/ubuntu/+source/reprepro/+bug/2008508. While
originally, it seemed only reproducible on s390x, it turned out that if
you generate Contents indices, it is more widely reproducible. I can
reliably reproduce it on Freexian infrastructure and filed #1050321.
[ Reason ]
The condition for reproducing the issue seem sufficiently common:
* Interact with zstd-compressed .debs
* Enable Contents indices
[ Impact ]
Once a zstd compressed .deb is included, most interactions leave
messages such as the following and Contents indices are incomplete.
zstd: /*stdout*\: Broken pipe
reading data.tar within './pool/main/p/php7.0/php7.0_7.0.33-67+freexian22.04.1+php+1_all.deb' failed: /usr/bin/unzstd exited with code 1
[ Tests ]
Simon Chopin, who originally fixed the bug in Ubuntu, was only able to
reproduce it with a wrapper to unzstd. By enabling Contents indices, I
was able to reproduce it reliably on amd64 both in bookworm and
unstable. Once updating to experimental (where the Simon's MR is
merged), the issue went away. I've also deployed the proposed update to
the affected Freexian server and the issue is gone there as well.
[ Risks ]
When not dealing with zstd-compressed .debs, the patched code paths are
not exercised. Therefore, there only really is any risk for people
interacting with zstd-compressed .debs.
[ Checklist ]
[x] *all* changes are documented in the d/changelog
[x] I reviewed all changes and I approve them
[x] attach debdiff against the package in (old)stable
[ ] the issue is verified as fixed in unstable
-> The issue is only fixed in experimental as of this writing, but
the maintainer intends to upload to unstable and Ubuntu uses a
very similar backport.
[ Changes ]
I think Simon Chopin does a much better job in is git commit message
than I could do here. The message is included in the .debdiff
[ Other info ]
The maintainer agreed to me doing the SPU.
Thanks for considering
Helmut
diff --minimal -Nru reprepro-5.3.1/debian/changelog reprepro-5.3.1/debian/changelog
--- reprepro-5.3.1/debian/changelog 2022-07-19 19:00:04.000000000 +0200
+++ reprepro-5.3.1/debian/changelog 2023-08-23 12:33:31.000000000 +0200
@@ -1,3 +1,14 @@
+reprepro (5.3.1-1+deb12u1) bookworm; urgency=medium
+
+ * Upload to stable with ack from maintainer.
+
+ [ Simon Chopin ]
+ * d/p/0001-uncompress-wait-until-the-child-as-exited-to-close-t.patch:
+ Fix a race condition when using external decompressors
+ (Closes: #1050321, LP: #2008508)
+
+ -- Helmut Grohne <helmut@subdivi.de> Wed, 23 Aug 2023 12:33:31 +0200
+
reprepro (5.3.1-1) unstable; urgency=medium
* Update debhelper-compat to level 12
diff --minimal -Nru reprepro-5.3.1/debian/patches/0001-uncompress-wait-until-the-child-as-exited-to-close-t.patch reprepro-5.3.1/debian/patches/0001-uncompress-wait-until-the-child-as-exited-to-close-t.patch
--- reprepro-5.3.1/debian/patches/0001-uncompress-wait-until-the-child-as-exited-to-close-t.patch 1970-01-01 01:00:00.000000000 +0100
+++ reprepro-5.3.1/debian/patches/0001-uncompress-wait-until-the-child-as-exited-to-close-t.patch 2023-08-23 12:33:05.000000000 +0200
@@ -0,0 +1,149 @@
+From 1b5a3c557b1ae842ee95b6a7e333046e56632559 Mon Sep 17 00:00:00 2001
+From: Simon Chopin <schopin@ubuntu.com>
+Date: Wed, 1 Mar 2023 17:53:28 +0100
+Subject: [PATCH] uncompress: wait until the child as exited to close the pipe
+
+Since we sometimes interrupt the decompression mid-stream as we're only
+looking for specific data, e.g. the control file in control.tar.zstd, it
+can happen that the child process still has a backlog of data to write
+out to the pipes before exiting. If we close its stdout pipe before
+calling waitpid(), it's going to encounter an EPIPE rather than
+gracefully exit.
+
+The fix is to only close the pipe fd after waitpid() successfully exits.
+
+However, that introduces a new theoretical issue: the child process could
+be blocking while writing into its stdout if the pipe is full, thus
+leading to a deadlock. To avoid this, we have to drain the pipe before
+waiting. Technically this should probably be done in a loop, but since
+it's fairly unlikely to be blocked on stdout in the first place, having
+enough pending data to fill the pipe *twice* seems too rare to bother
+with in the first place.
+
+The initial problem has first been noticed in Ubuntu autopkgtests on
+s390x when upgrading to libarchive 3.6.2, where unzstd would loudly
+complain about an -EPIPE (Ubuntu is using zstd as its default
+compression algorithm). After investigation, it could be consistently
+reproduced on low-powered s390x VMs with older libarchive releases,
+where only the initial invocation would succeed, but subsequent attempts
+would fail, presumably due performance changes via caching?
+
+To reproduce the issue more reliably, I used the following Rust code to
+produce a "proxy" unzstd that I dropped in /usr/local/bin. The output is
+the same, but only written 1kB at a time with 500ms pauses.
+
+```rust
+use std::io::{Read, Write};
+use std::process::{Command, Stdio};
+
+fn main() {
+ let mut child = Command::new("zstd")
+ .arg("-d")
+ .stdin(Stdio::inherit())
+ .stdout(Stdio::piped())
+ .spawn()
+ .expect("Failed to spawn the command");
+ let mut stdout = child.stdout.take().unwrap();
+ let mut buffer = [0u8; 1024];
+ loop {
+ std::thread::sleep(std::time::Duration::from_millis(500));
+ let size = stdout.read(&mut buffer).unwrap();
+ if size > 0 {
+ std::io::stdout().write_all(&buffer[0..size]).unwrap();
+ std::io::stdout().flush().unwrap();
+ continue;
+ }
+ if let Some(status) = child.try_wait().unwrap() {
+ std::process::exit(status.code().unwrap())
+ }
+ }
+}
+```
+
+(to compile it: `rustc unzstd.rs`)
+
+Due to the Debian freeze, we'll probably ship this patch in Ubuntu as a
+delta.
+
+Signed-off-by: Simon Chopin <schopin@ubuntu.com>
+
+Bug-Ubuntu: https://pad.lv/2008508
+
+---
+ uncompression.c | 35 +++++++++++++++++++++++++++++++----
+ 1 file changed, 31 insertions(+), 4 deletions(-)
+
+diff --git a/uncompression.c b/uncompression.c
+index 43819ae..1087b0b 100644
+--- a/uncompression.c
++++ b/uncompression.c
+@@ -1422,12 +1422,34 @@ int uncompress_read(struct compressedfile *file, void *buffer, int size) {
+ }
+ }
+
++static inline retvalue drain_pipe_fd(struct compressedfile *file, int *errno_p, const char **msg_p) {
++ int e = 0;
++ struct pollfd pollfd = {
++ file->fd,
++ POLLIN,
++ 0
++ };
++ unsigned char buffer[4096] = {};
++ while ((e = poll(&pollfd, 1, 0)) > 0) {
++ e = read(file->fd, buffer, 4096);
++ if (e <= 0)
++ break;
++ }
++ if (e < 0) {
++ *errno_p = e;
++ *msg_p = strerror(file->error);
++ return RET_ERRNO(e);
++ }
++ return RET_OK;
++}
++
+ static retvalue uncompress_commonclose(struct compressedfile *file, int *errno_p, const char **msg_p) {
+ retvalue result;
+ int ret;
+ int e;
+ pid_t pid;
+ int status;
++ int output_fd;
+ #define ERRORBUFFERSIZE 100
+ static char errorbuffer[ERRORBUFFERSIZE];
+
+@@ -1436,15 +1458,19 @@ static retvalue uncompress_commonclose(struct compressedfile *file, int *errno_p
+
+ if (file->external) {
+ free(file->intermediate.buffer);
+- (void)close(file->fd);
+ if (file->pipeinfd != -1)
+ (void)close(file->pipeinfd);
++ // Drain the child's stdout in the unlikely case it's blocking on it
++ e = drain_pipe_fd(file, errno_p, msg_p);
++ if (e != RET_OK)
++ return e;
++ output_fd = file->fd;
+ file->fd = file->infd;
+- file->infd = -1;
+ result = RET_OK;
+- if (file->pid <= 0)
++ if (file->pid <= 0) {
++ (void)close(output_fd);
+ return RET_OK;
+- pid = -1;
++ }
+ do {
+ if (interrupted()) {
+ *errno_p = EINTR;
+@@ -1454,6 +1480,7 @@ static retvalue uncompress_commonclose(struct compressedfile *file, int *errno_p
+ pid = waitpid(file->pid, &status, 0);
+ e = errno;
+ } while (pid == -1 && (e == EINTR || e == EAGAIN));
++ (void)close(output_fd);
+ if (pid == -1) {
+ *errno_p = e;
+ *msg_p = strerror(file->error);
+--
+2.37.2
+
diff --minimal -Nru reprepro-5.3.1/debian/patches/series reprepro-5.3.1/debian/patches/series
--- reprepro-5.3.1/debian/patches/series 1970-01-01 01:00:00.000000000 +0100
+++ reprepro-5.3.1/debian/patches/series 2023-08-23 12:33:05.000000000 +0200
@@ -0,0 +1 @@
+0001-uncompress-wait-until-the-child-as-exited-to-close-t.patch
Reply to: