[Fwd: Bug#538187: gzip doesn't call close() when writing to stdout]

[Bdale Garbee]

This patch proposed by a user of my Debian packaging of gzip seems like
a reasonable addition.  Opinions?

Bdale


Package: gzip
Version: 1.3.12-6
Severity: important
Tags: patch

When gzip is compressing / decompressing regular files, it checks the
return status of close on the outfile. But when compressing / decompressing to
stdout, it doesn't call close before exiting. This can lead to
undetectable write errors on NFS v3 filesystems, where writes can be
buffered locally until the final close or exit triggers an NFS v3 commit.

-- System Information:
Debian Release: 5.0
  APT prefers stable
  APT policy: (500, 'stable')
Architecture: amd64 (x86_64)

Kernel: Linux 2.6.24.2-ls4 (SMP w/2 CPU cores)
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=ANSI_X3.4-1968) (ignored: LC_ALL set to POSIX)
Shell: /bin/sh linked to /bin/bash

Versions of packages gzip depends on:
ii  debianutils                   2.30       Miscellaneous utilities specific t
ii  libc6                         2.7-18     GNU C Library: Shared libraries

gzip recommends no packages.

Versions of packages gzip suggests:
ii  less                          418-1      Pager program similar to more

-- no debconf information

---

--- gzip-1.3.12.orig/gzip.c
+++ gzip-1.3.12/gzip.c
@@ -182,6 +182,7 @@
 
 int ascii = 0;        /* convert end-of-lines to local OS conventions */
 int to_stdout = 0;    /* output to stdout (-c) */
+int used_stdout = 0;  /* set to 1 if compressed output ever went to stdout */
 int decompress = 0;   /* decompress (-d) */
 int force = 0;        /* don't ask questions, compress links (-f) */
 int no_name = -1;     /* don't save or restore the original file name */
@@ -582,6 +583,13 @@
     if (list && !quiet && file_count > 1) {
  do_list(-1, -1); /* print totals */
     }
+
+    /* if compressed data ever written to stdout,
+     * check for write errors that show up in close (NFS) */
+    if ((used_stdout || to_stdout) && fclose(stdout) != 0) {
+        write_error();
+    }
+
     do_exit(exit_code);
     return exit_code; /* just to avoid lint warning */
 }
@@ -654,6 +662,7 @@
 
     clear_bufs(); /* clear input and output buffers */
     to_stdout = 1;
+    used_stdout = 1;
     part_nb = 0;
 
     if (decompress) {

---

[Jim Meyering]

Bdale Garbee wrote:
> This patch proposed by a user of my Debian packaging of gzip seems like
> a reasonable addition.  Opinions?

Good catch.
However, there are many exit points, not just that one
at the end of main.  This patch ensures that no matter
which one is taken, that close is performed and checked.
It also uses gnulib's closein module so that it also closes
stdin, just as programs like cp and rm do, since they
too may prompt the user.

Here's a better patch:

2009-07-29  Jim Meyering  <meyering@...>

        avoid silent data loss e.g., on NFS, due to unchecked close of stdout
        * gzip.c: Include "closein.h".
        (main): Use atexit (close_stdin);
        * bootstrap.conf (gnulib_modules): Add closein.
        Prompted by Mark Kidwell's report and patch in
        http://bugs.debian.org/538187

Index: gzip.c
===================================================================
RCS file: /sources/gzip/gzip/gzip.c,v
retrieving revision 1.21
diff -u -p -r1.21 gzip.c
--- gzip.c 25 Nov 2007 17:19:46 -0000 1.21
+++ gzip.c 29 Jul 2009 15:34:53 -0000
@@ -1,6 +1,6 @@
 /* gzip (GNU zip) -- compress files with zip algorithm and 'compress' interface

-   Copyright (C) 1999, 2001, 2002, 2006, 2007 Free Software Foundation, Inc.
+   Copyright (C) 1999, 2001, 2002, 2006, 2007, 2009 Free Software Foundation, Inc.
    Copyright (C) 1992-1993 Jean-loup Gailly

    This program is free software; you can redistribute it and/or modify
@@ -64,6 +64,7 @@ static char rcsid[] = "$Id: gzip.c,v 1.2
 #include <sys/stat.h>
 #include <errno.h>

+#include "closein.h"
 #include "tailor.h"
 #include "gzip.h"
 #include "lzw.h"
@@ -417,6 +418,8 @@ int main (argc, argv)
     program_name = gzip_base_name (argv[0]);
     proglen = strlen (program_name);

+    atexit (close_stdin);
+
     /* Suppress .exe for MSDOS, OS/2 and VMS: */
     if (4 < proglen && strequ (program_name + proglen - 4, ".exe"))
       program_name[proglen - 4] = '\0';
Index: bootstrap.conf
===================================================================
RCS file: /sources/gzip/gzip/bootstrap.conf,v
retrieving revision 1.6
diff -u -p -r1.6 bootstrap.conf
--- bootstrap.conf 3 Jul 2007 20:37:07 -0000 1.6
+++ bootstrap.conf 29 Jul 2009 15:34:53 -0000
@@ -1,6 +1,6 @@
 # Bootstrap configuration.

-# Copyright (C) 2006 Free Software Foundation, Inc.
+# Copyright (C) 2006, 2009 Free Software Foundation, Inc.

 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -23,6 +23,7 @@ IMPORT_FROM_GETTEXT=no

 # gnulib modules used by this package.
 gnulib_modules='
+ closein
  fcntl
  fcntl-safer
  fdl