Re: [Bug-tar] free(valloc()): invalid pointer

[Paul Eggert]

Justin Pryzby <address@hidden> writes:

> Please see Debian bug #248897 at [1] and its sibling at [2].  tar uses
> valloc() to get aligned memory, and later free()s it.  However, the
> pointer returned by glibc valloc() is not necessarily free()able.

Thanks for reporting this.  I see that the underlying problem is a bug
in Debian glibc (Debian bug 262782, which you just filed).  It's
clearly a bug, as the glibc manual says:

  With the GNU library, you can use `free' to free the blocks that
  `memalign', `posix_memalign', and `valloc' return.
  
<http://www.gnu.org/software/libc/manual/html_node/Aligned-Memory-Blocks.html#Aligned%20Memory%20Blocks>

That being said, tar should work around the Debian bug, as similar
problems apparently occur on a few other platforms.  I installed the
following patch into the tar CVS main branch.

2004-08-02  Paul Eggert  <address@hidden>

        * bootstrap (gnulib_modules): Add getpagesize.
        * configure.ac (valloc): Remove check; valloc no longer used.
        * lib/.cvsignore: Add getpagesize.h.
        * m4/.cvsignore: Add getpagesize.m4.
        * src/buffer.c (record_buffer): New var.
        (open_archive): Don't use valloc; on older or buggy hosts, you can't
        free the result.  Use page_aligned_alloc instead.
        * src/compare.c (diff_init): Likewise.
        * src/buffer.c (open_archive): Record the pointer to be freed
        into record_buffer.
        (close_archive): Free record_buffer.
        * src/common.h (page_aligned_alloc): New decl.
        * src/misc.c (quote_n, quote): Remove these redundant functions.
        (ptr_align): New function, from coreutils/src/system.h.
        (page_aligned_alloc): New function.
        * src/system.h (valloc): Remove.

Index: bootstrap
===================================================================
RCS file: /cvsroot/tar/tar/bootstrap,v
retrieving revision 1.16
diff -p -u -r1.16 bootstrap
--- bootstrap   19 May 2004 10:32:52 -0000      1.16
+++ bootstrap   3 Aug 2004 04:41:05 -0000
@@ -150,6 +150,7 @@ full-write
 getdate
 getline
 getopt
+getpagesize
 gettext
 gettime
 hash
Index: configure.ac
===================================================================
RCS file: /cvsroot/tar/tar/configure.ac,v
retrieving revision 1.43
diff -p -u -r1.43 configure.ac
--- configure.ac        16 May 2004 20:47:07 -0000      1.43
+++ configure.ac        3 Aug 2004 04:41:05 -0000
@@ -132,7 +132,6 @@ LIBS=$tar_save_LIBS
 AC_CHECK_FUNCS(fsync lstat mkfifo readlink strerror symlink setlocale utimes)
 AC_CHECK_DECLS([getgrgid],,, [#include <grp.h>])
 AC_CHECK_DECLS([getpwuid],,, [#include <pwd.h>])
-AC_CHECK_DECLS([valloc])
 AC_CHECK_DECLS([time],,, [#include <time.h>])
 
 # Set LIB_SETSOCKOPT to -lnsl -lsocket if necessary.
Index: lib/.cvsignore
===================================================================
RCS file: /cvsroot/tar/tar/lib/.cvsignore,v
retrieving revision 1.9
diff -p -u -r1.9 .cvsignore
--- lib/.cvsignore      10 Jul 2004 06:51:38 -0000      1.9
+++ lib/.cvsignore      3 Aug 2004 04:41:05 -0000
@@ -40,6 +40,7 @@ getopt.c
 getopt.h
 getopt1.c
 getopt_int.h
+getpagesize.h
 gettext.h
 gettime.c
 gettimeofday.c
Index: m4/.cvsignore
===================================================================
RCS file: /cvsroot/tar/tar/m4/.cvsignore,v
retrieving revision 1.10
diff -p -u -r1.10 .cvsignore
--- m4/.cvsignore       16 May 2004 20:55:18 -0000      1.10
+++ m4/.cvsignore       3 Aug 2004 04:41:05 -0000
@@ -21,6 +21,7 @@ getdate.m4
 getline.m4
 getndelim2.m4
 getopt.m4
+getpagesize.m4
 gettext.m4
 gettime.m4
 gettimeofday.m4
Index: src/buffer.c
===================================================================
RCS file: /cvsroot/tar/tar/src/buffer.c,v
retrieving revision 1.68
diff -p -u -r1.68 buffer.c
--- src/buffer.c        19 May 2004 14:25:27 -0000      1.68
+++ src/buffer.c        3 Aug 2004 04:41:05 -0000
@@ -40,6 +40,7 @@
 
 static tarlong prev_written;   /* bytes written on previous volumes */
 static tarlong bytes_written;  /* bytes written on this volume */
+static void *record_buffer;    /* allocated memory */
 
 /* FIXME: The following variables should ideally be static to this
    module.  However, this cannot be done yet.  The cleanup continues!  */
@@ -270,17 +271,12 @@ open_archive (enum access_mode wanted_ac
   save_name = 0;
   real_s_name = 0;
 
+  record_start =
+    page_aligned_alloc (&record_buffer,
+                       (record_size
+                        + (multi_volume_option ? 2 * BLOCKSIZE : 0)));
   if (multi_volume_option)
-    {
-      record_start = valloc (record_size + (2 * BLOCKSIZE));
-      if (record_start)
-       record_start += 2;
-    }
-  else
-    record_start = valloc (record_size);
-  if (!record_start)
-    FATAL_ERROR ((0, 0, _("Cannot allocate memory for blocking factor %d"),
-                 blocking_factor));
+    record_start += 2;
 
   current_block = record_start;
   record_end = record_start + blocking_factor;
@@ -946,7 +942,7 @@ close_archive (void)
     free (save_name);
   if (real_s_name)
     free (real_s_name);
-  free (multi_volume_option ? record_start - 2 : record_start);
+  free (record_buffer);
 }
 
 /* Called to initialize the global volume number.  */
Index: src/common.h
===================================================================
RCS file: /cvsroot/tar/tar/src/common.h,v
retrieving revision 1.37
diff -p -u -r1.37 common.h
--- src/common.h        29 Jun 2004 10:10:26 -0000      1.37
+++ src/common.h        3 Aug 2004 04:41:06 -0000
@@ -586,6 +586,8 @@ void write_fatal_details (char const *, 
 pid_t xfork (void);
 void xpipe (int[2]);
 
+void *page_aligned_alloc (void **, size_t);
+
 /* Module names.c.  */
 
 extern struct name *gnu_list_name;
Index: src/compare.c
===================================================================
RCS file: /cvsroot/tar/tar/src/compare.c,v
retrieving revision 1.20
diff -p -u -r1.20 compare.c
--- src/compare.c       5 Apr 2004 02:29:18 -0000       1.20
+++ src/compare.c       3 Aug 2004 04:41:06 -0000
@@ -54,9 +54,8 @@ static char *diff_buffer;
 void
 diff_init (void)
 {
-  diff_buffer = valloc (record_size);
-  if (!diff_buffer)
-    xalloc_die ();
+  void *ptr;
+  diff_buffer = page_aligned_alloc (&ptr, record_size);
 }
 
 /* Sigh about something that differs by writing a MESSAGE to stdlis,
Index: src/misc.c
===================================================================
RCS file: /cvsroot/tar/tar/src/misc.c,v
retrieving revision 1.18
diff -p -u -r1.18 misc.c
--- src/misc.c  16 May 2004 20:50:56 -0000      1.18
+++ src/misc.c  3 Aug 2004 04:41:06 -0000
@@ -916,18 +916,29 @@ xpipe (int fd[2])
     call_arg_fatal ("pipe", _("interprocess channel"));
 }
 
-/* Return an unambiguous printable representation, allocated in slot N,
-   for NAME, suitable for diagnostics.  */
-char const *
-quote_n (int n, char const *name)
+/* Return PTR, aligned upward to the next multiple of ALIGNMENT.
+   ALIGNMENT must be nonzero.  The caller must arrange for ((char *)
+   PTR) through ((char *) PTR + ALIGNMENT - 1) to be addressable
+   locations.  */
+
+static inline void *
+ptr_align (void *ptr, size_t alignment)
 {
-  return quotearg_n_style (n, locale_quoting_style, name);
+  char *p0 = ptr;
+  char *p1 = p0 + alignment - 1;
+  return p1 - (size_t) p1 % alignment;
 }
 
-/* Return an unambiguous printable representation of NAME, suitable
-   for diagnostics.  */
-char const *
-quote (char const *name)
+/* Return the address of a page-aligned buffer of at least SIZE bytes.
+   The caller should free *PTR when done with the buffer.  */
+
+void *
+page_aligned_alloc (void **ptr, size_t size)
 {
-  return quote_n (0, name);
+  size_t alignment = getpagesize ();
+  size_t size1 = size + alignment;
+  if (size1 < size)
+    xalloc_die ();
+  *ptr = xmalloc (size1);
+  return ptr_align (*ptr, alignment);
 }
Index: src/system.h
===================================================================
RCS file: /cvsroot/tar/tar/src/system.h,v
retrieving revision 1.19
diff -p -u -r1.19 system.h
--- src/system.h        16 May 2004 20:56:05 -0000      1.19
+++ src/system.h        3 Aug 2004 04:41:06 -0000
@@ -502,10 +502,6 @@ time_t time ();
 #define _(msgid) gettext (msgid)
 #define N_(msgid) msgid
 
-#if ! defined valloc && ! HAVE_DECL_VALLOC
-# define valloc(size) malloc (size)
-#endif
-
 #if MSDOS
 # include <process.h>
 # define SET_BINARY_MODE(arc) setmode(arc, O_BINARY)