[ I'm not subscribed to the list, so please keep me in Cc ]
Greetings tar users and developers,
For the past 2 days I've been hunting a tar-related failure that I could only
reproduce by running dpkg-source, running the tar incantation dpkg-source uses
internally on its own doesn't appear to work. I'll document my debugging
endeavour here to make sure I have my facts straight, as well as to help anyone
else stumbling onto this.
The error is as follows:
$ dpkg-source -b .
dpkg-source: info: using source format '3.0 (quilt)'
dpkg-source: info: building newsboat using existing
./newsboat_2.20.1.orig.tar.xz
dpkg-source: info: building newsboat using existing
./newsboat_2.20.1.orig.tar.xz.asc
dpkg-source: info: building newsboat in newsboat_2.20.1-1.debian.tar.xz
tar: debian/changelog: File shrank by 36 bytes; padding with zeros
dpkg-source: error: tar -cf - subprocess returned exit status 1
On further investigation, this bug is triggered if all of the following
conditions are met (on my system):
1. Have a changelog file >4096 bytes
2. The build directory should be in a 9p filesystem (in a VM)
3. Kernel version should be >=5.6 (it doesn't error on 4.19)
4. Do the exact combination of steps dpkg-source does (running the tar command
directly
doesn't appear to cause it)
Condition 3 threw me off and I started looking into the kernel as the source of
this, however, I now believe it's some change in the 9p driver that simply
uncovered it.
$ strace -f dpkg-source -b .
[...]
72606 execve("/home/nikos/tar-1.32/src//tar", ["tar", "-cf", "-",
"--format=gnu", "--sort=name", "--mtime", "@1595708407", "--clamp-mtime",
"--null", "--numeric-owner", "--owner=0", "--group=0", "--exclude=*.a",
"--exclude=*.la", "--exclude=*.o", "--exclude=*.so", "--exclude=.*.sw?",
"--exclude=*/*~", "--exclude=,,*", "--exclude=.[#~]*", "--exclude=.arch-ids",
"--exclude=.arch-inventory", "--exclude=.be", "--exclude=.bzr",
"--exclude=.bzr.backup", "--exclude=.bzr.tags", "--exclude=.bzrignore",
"--exclude=.cvsignore", "--exclude=.deps", "--exclude=.git",
"--exclude=.gitattributes", "--exclude=.gitignore", ...], 0x55b9e7b2b4d0 /* 22
vars */ <unfinished ...>
[...]
72606 newfstatat(3, "changelog", {st_mode=S_IFREG|0644, st_size=4132, ...},
AT_SYMLINK_NOFOLLOW) = 0
72606 openat(3, "changelog", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC)
= 4
72606 fstat(4, {st_mode=S_IFREG|0644, st_size=4132, ...}) = 0
72606 read(4, "newsboat (2.20.1-1) UNRELEASED; "..., 4132) = 4096
72606 write(2, "tar: ", 5) = 5
72606 write(2, "debian/changelog: File shrank by"..., 61) = 61
72606 write(2, "\n", 1) = 1
72606 close(4) = 0
[...]
The read() call returns less bytes than requested and then tar assumed it was
truncated and issues a warning. I traced this call to create.c:dump_regular_file
which indeed doesn't seem to handle this possibility.
Attached are two patches, the first replaces the safe_read call in
blocking_read with
full_read from gnulib, I have verified that this change fixes the bug.
I can't seem to understand what's the point of distinguishing between
safe_{read,write} and full_{read,write}, is there a situation where not handling
short read/writes would be important? Why not merge these functions?
So finally, since I couldn't find any reason not to, the second patch replaces
all instances of safe_read with full_read to avoid such problems in other areas
of the code. (note: Without much verification!)
>From b5a8b6a29a11cded297f18095d522b87f8aee406 Mon Sep 17 00:00:00 2001
From: Nikos Tsipinakis <[email protected]>
Date: Sun, 26 Jul 2020 23:32:12 +0300
Subject: [PATCH] Use full_read to avoid errors due to short reads
read(2) can return any number of bytes short of the requested without
raising an error or being interrupted (which were already
checked/handled), however, in that case, tar did not attempt to read the
remaining bytes and raised errors.
---
gnulib.modules | 1 +
src/common.h | 1 +
src/misc.c | 4 ++--
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/gnulib.modules b/gnulib.modules
index 82f5e1c..9ded7da 100644
--- a/gnulib.modules
+++ b/gnulib.modules
@@ -44,6 +44,7 @@ fprintftime
fseeko
fstatat
full-write
+full-read
futimens
getline
getopt-gnu
diff --git a/src/common.h b/src/common.h
index a451999..7f696c5 100644
--- a/src/common.h
+++ b/src/common.h
@@ -57,6 +57,7 @@
#include "arith.h"
#include <backupfile.h>
#include <exclude.h>
+#include <full-read.h>
#include <full-write.h>
#include <modechange.h>
#include <quote.h>
diff --git a/src/misc.c b/src/misc.c
index d833b8d..81e2a96 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -804,7 +804,7 @@ deref_stat (char const *name, struct stat *buf)
size_t
blocking_read (int fd, void *buf, size_t count)
{
- size_t bytes = safe_read (fd, buf, count);
+ size_t bytes = full_read (fd, buf, count);
#if defined F_SETFL && O_NONBLOCK
if (bytes == SAFE_READ_ERROR && errno == EAGAIN)
@@ -812,7 +812,7 @@ blocking_read (int fd, void *buf, size_t count)
int flags = fcntl (fd, F_GETFL);
if (0 <= flags && flags & O_NONBLOCK
&& fcntl (fd, F_SETFL, flags & ~O_NONBLOCK) != -1)
- bytes = safe_read (fd, buf, count);
+ bytes = full_read (fd, buf, count);
}
#endif
--
2.28.0.rc2
>From a8f4ef2723fc29ac8f3135a157b234c9f61ce704 Mon Sep 17 00:00:00 2001
From: Nikos Tsipinakis <[email protected]>
Date: Mon, 27 Jul 2020 01:06:30 +0300
Subject: [PATCH 2/2] Remove remaining usages of safe_read
Continuation of the previous commit, remove all remaining usages of
safe_read to avoid errors due to short reads, and instead replace it
with full_read which handles them gracefully.
---
src/sparse.c | 6 +++---
src/system.c | 2 +-
src/update.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/sparse.c b/src/sparse.c
index cc3c515..39e1627 100644
--- a/src/sparse.c
+++ b/src/sparse.c
@@ -417,7 +417,7 @@ sparse_dump_region (struct tar_sparse_file *file, size_t i)
size_t bytes_read;
blk = find_next_block ();
- bytes_read = safe_read (file->fd, blk->buffer, bufsize);
+ bytes_read = full_read (file->fd, blk->buffer, bufsize);
if (bytes_read == SAFE_READ_ERROR)
{
read_diag_details (file->stat_info->orig_file_name,
@@ -614,7 +614,7 @@ check_sparse_region (struct tar_sparse_file *file, off_t beg, off_t end)
size_t rdsize = BLOCKSIZE < end - beg ? BLOCKSIZE : end - beg;
char diff_buffer[BLOCKSIZE];
- bytes_read = safe_read (file->fd, diff_buffer, rdsize);
+ bytes_read = full_read (file->fd, diff_buffer, rdsize);
if (bytes_read == SAFE_READ_ERROR)
{
read_diag_details (file->stat_info->orig_file_name,
@@ -667,7 +667,7 @@ check_data_region (struct tar_sparse_file *file, size_t i)
}
set_next_block_after (blk);
file->dumped_size += BLOCKSIZE;
- bytes_read = safe_read (file->fd, diff_buffer, rdsize);
+ bytes_read = full_read (file->fd, diff_buffer, rdsize);
if (bytes_read == SAFE_READ_ERROR)
{
read_diag_details (file->stat_info->orig_file_name,
diff --git a/src/system.c b/src/system.c
index f90e47d..5b93228 100644
--- a/src/system.c
+++ b/src/system.c
@@ -431,7 +431,7 @@ sys_child_open_for_compress (void)
{
size_t size = record_size - length;
- status = safe_read (STDIN_FILENO, cursor, size);
+ status = full_read (STDIN_FILENO, cursor, size);
if (status == SAFE_READ_ERROR)
read_fatal (use_compress_program_option);
if (status == 0)
diff --git a/src/update.c b/src/update.c
index 511df18..b1add03 100644
--- a/src/update.c
+++ b/src/update.c
@@ -77,7 +77,7 @@ append_file (char *file_name)
memset (start->buffer + bytes_left, 0, BLOCKSIZE - status);
}
- status = safe_read (handle, start->buffer, buffer_size);
+ status = full_read (handle, start->buffer, buffer_size);
if (status == SAFE_READ_ERROR)
read_fatal_details (file_name, stat_data.st_size - bytes_left,
buffer_size);
--
2.28.0.rc2
