Re: Patches for GNU tar 1.13.25

[Paul Eggert]

> From: Bruce Lilly <address@hidden>
> Date: Sat, 13 Jul 2002 10:51:44 -0400
> 
> The attached patch addresses most of the issues
> when compiling GNU tar 1.13.25 on UWIN

Thanks for doing the port.  Which version of UWIN, and which compiler
and library?

> Several issues noted remain unresolved, some others
> (e.g. application of unary minus to unsigned quantities) have not
> even been addressed

I don't understand this point, as unary minus has well-defined
behavior on unsigned quantities in C.

> most of those issues could be avoided by using double throughout for
> large quantities (double has a 64-bit mantissa) rather than
> non-portable and implementation-dependent constructs].

Sorry, I don't understand this point either.  We do need to avoid
unportable constructs.  But portable code cannot assume that double
has a 64-bit mantissa.  On my SPARC host, for example, double has only
a 53-bit fraction.  On x86, double expressions sometimes have a 64-bit
fraction, and sometimes a 53-bit fraction, depending on the vagaries
of one's compiler and runtime system.

> 1. no supplied conversion between uintmax_t and
>    double. Portable and efficient conversion
>    code supplied.

I take it that your compiler does not support conversion from
uintmax_t to double?  If so, we should fix "configure" so that it
detects that problem, and defines uintmax_t to be 'long'.

Can you supply a little test program that illustrates the compiler
problem?  For example what happens if you compile and run the
following little program?  It should output "(double)
9223372036854775808 == 9223372036854775808" on any machine with a
radix that is a power of 2.

  #include <inttypes.h>

  #define verify(name, assertion) struct name { char a[(assertion) ? 1 : -1]; }
  verify (convert_uintmax_t_to_double,
          (double) (uintmax_t) 9223372036854775808 == 9223372036854775808.0);
  verify (convert_double_to_uintmax_t,
          (uintmax_t) 9223372036854775808.0 == 9223372036854775808);

  #include <stdio.h>

  uintmax_t u = 9223372036854775808;

  int main (void)
  {
    double d = u;
    printf ("(double) 9223372036854775808 == %.20g\n", d);
    return 0;
  }

> 2. getdate.[cy] needs to declare mktime if the
>    replacement function is used.  extern
>    declaration should be harmless even if the
>    system mktime is used.

I worry that some hosts do funky things with mktime defines in <time.h>.
How about this patch instead of the one you supplied?

--- getdate.y   2002/01/16 22:43:41     1.28
+++ getdate.y   2002/07/14 03:25:09
@@ -27,9 +27,14 @@
 
 #ifdef HAVE_CONFIG_H
 # include <config.h>
-# ifdef HAVE_ALLOCA_H
-#  include <alloca.h>
-# endif
+#endif
+
+#ifdef mktime
+# define MKTIME_IS_REPLACED 1
+#endif
+
+#if HAVE_ALLOCA_H
+# include <alloca.h>
 #endif
 
 /* Since the code of getdate.y is not included in the Emacs executable
@@ -455,7 +460,7 @@ struct tm *gmtime ();
 #ifndef localtime
 struct tm *localtime ();
 #endif
-#ifndef mktime
+#if ! defined mktime || MKTIME_IS_REPLACED
 time_t mktime ();
 #endif


> 3. do-nothing code in human.c #ifdef'ed out.

But it's not do-nothing code.  Here are some details:

> +    b) does nothing useful (see comments below)
> + */
>     if (inexact_style != human_round_to_even && value < (uintmax_t) -1)
>       {
> !       uintmax_t u = value;  /* N.B. u == value (unconditionally) */

u might not equal value.  value is of type double, so it might not be
an integer, and it might be out of range for uintmax_t.  So it is
possible that (u != value) == 1.  (It is also possible that u does not
equal value, but (u != value) == 0, due to the vagaries of C
arithmetic; but that is a different story.)

> +    Most of the code in this file needs substantial revision.

The revision for GiB etc. has already been done; see
<http://savannah.gnu.org/cgi-bin/viewcvs/gnulib/gnulib/lib/human.c>.
This should appear in the next version of tar.


> 4. inconsistent placement of const qualifiers
>    made more consistent.

This sort of patch shouldn't be needed.  For example, 'char const' is
equivalent to 'const char' on all Standard C compilers.  If you have a
compiler that doesn't support 'const' properly, then 'configure'
should arrange for '#define const /**/' to appear in config.h.  If
config.h is incorrect, let's fix that bug rather than munge the code.


> 5. non-portable tests which assume that size_t
>    is signed replaced by portable tests.

I don't understand this point.  size_t is guaranteed to be unsigned,
and the code assumes this.  Is size_t signed on your host?


> 6. another case of the iconv "bug" accounted for.
> *** lib/unicodeio.c.orig      Tue Sep 25 18:52:20 2001
> --- lib/unicodeio.c   Tue Jul 02 21:14:16 2002
> ***************
> *** 209,215 ****
>   
>         /* Avoid glibc-2.1 bug and Solaris 2.7 bug.  */
>   # if defined _LIBICONV_VERSION \
> !     || !((__GLIBC__ - 0 == 2 && __GLIBC_MINOR__ - 0 <= 1) || defined __sun)
>   
>         /* Get back to the initial shift state.  */
>         res = iconv (utf8_to_local, NULL, NULL, &outptr, &outbytesleft);
> --- 209,215 ----
>   
>         /* Avoid glibc-2.1 bug and Solaris 2.7 bug.  */
>   # if defined _LIBICONV_VERSION \
> !     || !((__GLIBC__ - 0 == 2 && __GLIBC_MINOR__ - 0 <= 1) || defined __sun 
> || defined _UWIN)
>   
>         /* Get back to the initial shift state.  */
>         res = iconv (utf8_to_local, NULL, NULL, &outptr, &outbytesleft);

Doesn't UWIN use glibc?  If not, which library and version does UWIN
use?  If UWIN uses a particular C library, I'd rather have the ifdef
depend on that rather than on the more-generic _UWIN symbol.
(Presumably the bug will be fixed at some point.)


> 7. test for O_ECL vs. O_TRUNC for file creation
>    during extraction corrected.
> *** src/extract.c.orig        Mon Sep 24 14:55:17 2001
> --- src/extract.c     Sat Jul 13 08:13:17 2002
> ***************
> *** 766,772 ****
>   
>       again_file:
>         openflag = (O_WRONLY | O_BINARY | O_CREAT
> !               | (old_files_option == OVERWRITE_OLD_FILES
>                    ? O_TRUNC
>                    : O_EXCL));
>         mode = current_stat.st_mode & MODE_RWX & ~ current_umask;
> --- 766,772 ----
>   
>       again_file:
>         openflag = (O_WRONLY | O_BINARY | O_CREAT
> !               | (old_files_option != KEEP_OLD_FILES
>                    ? O_TRUNC
>                    : O_EXCL));
>         mode = current_stat.st_mode & MODE_RWX & ~ current_umask;

I don't see the bug here.  For example, if old_files_option ==
DEFAULT_OLD_FILES, the code should use O_EXCL, because tar does not
want to overwrite existing files in that case.


> *** lib/exclude.h.orig        Sun Aug 26 19:07:15 2001
> --- lib/exclude.h     Tue Jul 02 14:13:28 2002
> ***************
> *** 30,43 ****
>   
>   /* Patterns must match the start of file names, instead of matching
>      anywhere after a '/'.  */
> ! #define EXCLUDE_ANCHORED (1 << 5)
>   
>   /* Include instead of exclude.  */
> ! #define EXCLUDE_INCLUDE (1 << 6)
>   
>   /* '?', '*', '[', and '\\' are special in patterns.  Without this
>      option, these characters are ordinary and fnmatch is not used.  */
> ! #define EXCLUDE_WILDCARDS (1 << 7)
>   
>   struct exclude;
>   
> --- 30,43 ----
>   
>   /* Patterns must match the start of file names, instead of matching
>      anywhere after a '/'.  */
> ! #define EXCLUDE_ANCHORED (1 << 7)
>   
>   /* Include instead of exclude.  */
> ! #define EXCLUDE_INCLUDE (1 << 8)
>   
>   /* '?', '*', '[', and '\\' are special in patterns.  Without this
>      option, these characters are ordinary and fnmatch is not used.  */
> ! #define EXCLUDE_WILDCARDS (1 << 9)
>   
>   struct exclude;
>   

Why was the above patch needed?  It doesn't seem to match any of your
comments.


> *** src/create.c.orig Wed Aug 29 17:21:02 2001
> --- src/create.c      Tue Jul 02 20:25:17 2002
> ***************
> *** 1108,1114 ****
>          (entrylen = strlen (entry)) != 0;
>          entry += entrylen + 1)
>       {
> !       if (buflen <= len + entrylen)
>           {
>             buflen = len + entrylen;
>             namebuf = xrealloc (namebuf, buflen + 1);
> --- 1108,1114 ----
>          (entrylen = strlen (entry)) != 0;
>          entry += entrylen + 1)
>       {
> !       if (buflen < len + entrylen)
>           {
>             buflen = len + entrylen;
>             namebuf = xrealloc (namebuf, buflen + 1);

This patch seems incorrect to me.  Surely it can cause a buffer
overflow if buflen == len + entrylen, because the buffer needs to be
enlarged in that case.



> From: Bruce Lilly <address@hidden>
> Date: Sat, 13 Jul 2002 12:05:09 -0400
> 
> The following supplementary patch may be required on some systems:
> *** lib/savedir.c.orig    Wed Jul  3 01:34:55 2002
> --- lib/savedir.c       Sat Jul 13 11:56:14 2002
> ***************
> *** 60,65 ****
> --- 60,69 ----
>    # define NULL 0
>    #endif
> 
> + #if HAVE_LIMITS_H || _LIBC
> + # include <limits.h>
> + #endif
> +
>    #include "savedir.h"
>    #include "xalloc.h"
> 

Hmm, why?  savedir.c doesn't use anything that limits.h provides.
The "|| _LIBC" shouldn't be needed in any case, as the file
isn't an internal glibc file.