Hello François, On Thursday, March 13, 2014 14:43:55 François Ouellet wrote: > When dumping a file which is being actively updated by an application > (in our case it was an outlook pst file on our samba server), safe_read() > can sometimes return 0. > > When it happens, sparse_dump_region() goes into an infinite loop. > > I don't know what the proper fix would be. I just fixed it on our server > with this:
thanks for clear report! You can try the attached patch if you wanted (but it has the same effects as yours one, just the message is little bit different). Reproducer: $ truncate -s 10M file $ tar cSf archive file $ truncate -s 5M file $ tar df archive Could we please apply at least something like the patch attached? Looking at the code, I don't like duplications. I was thinking about safe_read/blocking_read wrapper with sth. like 'bool may_eof' argument (and others) to make it possible to deal with errors at one place (stopped once I realized that it will be somehow bigger code change). Would you be interested in such patch? Pavel
>From c008273c7488e47dbf942ec79e83e95d34ff873f Mon Sep 17 00:00:00 2001 From: Pavel Raiskup <prais...@redhat.com> Date: Sun, 16 Mar 2014 22:26:33 +0100 Subject: [PATCH] tar: fix infinite loop in --diff for sparse files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When sparse file got truncated, tar went into infinite loop (reliably when the file ended by hole originally, unlikely when file ended with data). Fixed also potential loop (in similar fashion) in sparse files dumping. Original bugreport: http://lists.gnu.org/archive/html/bug-tar/2014-03/msg00052.html Thanks François Ouellet * src/sparse.c (check_sparse_region): Return false when safe_read returns 0. Also print the error on correct position in file. (check_data_region): Report error when safe_read returns 0. (sparse_dump_region): Fail if file got truncated. * THANKS: Adjust. --- THANKS | 1 + src/sparse.c | 48 ++++++++++++++++++++++++++++++++---------------- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/THANKS b/THANKS index b4c5427..e74f71c 100644 --- a/THANKS +++ b/THANKS @@ -175,6 +175,7 @@ Fabio d'Alessi c...@civ.bio.unipd.it Frank Heckenbach fr...@g-n-u.de Frank Koenen koe...@lidp.com Franz-Werner Gergen ger...@edvulx.mpi-stuttgart.mpg.de +François Ouellet fou...@gmail.com François Pinard pin...@iro.umontreal.ca Fritz Elfert fr...@fsun.triltsch.de George Chyu gsc...@ccgate.dp.beckman.com diff --git a/src/sparse.c b/src/sparse.c index 6a97676..53c1868 100644 --- a/src/sparse.c +++ b/src/sparse.c @@ -301,6 +301,7 @@ sparse_dump_region (struct tar_sparse_file *file, size_t i) { union block *blk; off_t bytes_left = file->stat_info->sparse_map[i].numbytes; + const char *file_name = file->stat_info->orig_file_name; if (!lseek_or_error (file, file->stat_info->sparse_map[i].offset)) return false; @@ -314,13 +315,23 @@ sparse_dump_region (struct tar_sparse_file *file, size_t i) bytes_read = safe_read (file->fd, blk->buffer, bufsize); if (bytes_read == SAFE_READ_ERROR) { - read_diag_details (file->stat_info->orig_file_name, + read_diag_details (file_name, (file->stat_info->sparse_map[i].offset + file->stat_info->sparse_map[i].numbytes - bytes_left), bufsize); return false; } + if (bytes_read == 0) + { + char buf[UINTMAX_STRSIZE_BOUND]; + FATAL_ERROR ((0, 0, + ngettext ("%s: File shrank by %s byte", + "%s: File shrank by %s bytes", + bytes_left), + quotearg_colon (file_name), + offtostr (bytes_left, buf))); + } memset (blk->buffer + bytes_read, 0, BLOCKSIZE - bytes_read); bytes_left -= bytes_read; @@ -475,33 +486,37 @@ sparse_skip_file (struct tar_stat_info *st) static bool check_sparse_region (struct tar_sparse_file *file, off_t beg, off_t end) { - if (!lseek_or_error (file, beg)) + off_t offset = beg; + + if (!lseek_or_error (file, offset)) return false; - while (beg < end) + while (offset < end) { size_t bytes_read; - size_t rdsize = BLOCKSIZE < end - beg ? BLOCKSIZE : end - beg; + size_t rdsize = BLOCKSIZE < end - offset ? BLOCKSIZE : end - offset; char diff_buffer[BLOCKSIZE]; bytes_read = safe_read (file->fd, diff_buffer, rdsize); if (bytes_read == SAFE_READ_ERROR) { read_diag_details (file->stat_info->orig_file_name, - beg, - rdsize); - return false; - } - if (!zero_block_p (diff_buffer, bytes_read)) - { - char begbuf[INT_BUFSIZE_BOUND (off_t)]; - report_difference (file->stat_info, - _("File fragment at %s is not a hole"), - offtostr (beg, begbuf)); + offset, rdsize); return false; } - beg += bytes_read; + if (bytes_read == 0 + || !zero_block_p (diff_buffer, bytes_read)) + { + char begbuf[INT_BUFSIZE_BOUND (off_t)]; + const char *msg = bytes_read ? _("File fragment at %s is not a hole") + : _("Hole starting at %s is truncated"); + + report_difference (file->stat_info, msg, offtostr (beg, begbuf)); + return false; + } + + offset += bytes_read; } return true; } @@ -542,7 +557,8 @@ check_data_region (struct tar_sparse_file *file, size_t i) file->dumped_size += bytes_read; size_left -= bytes_read; mv_size_left (file->stat_info->archive_file_size - file->dumped_size); - if (memcmp (blk->buffer, diff_buffer, rdsize)) + if (bytes_read == 0 + || memcmp (blk->buffer, diff_buffer, bytes_read)) { report_difference (file->stat_info, _("Contents differ")); return false; -- 1.8.5.3
