Re: Patches for GNU tar 1.13.25

[Paul Eggert]

> Date: Sun, 14 Jul 2002 15:41:26 -0400
> From: Bruce Lilly <address@hidden>

> > I don't understand this point, as unary minus has well-defined
> > behavior on unsigned quantities in C.
> The compiler emits warnings such as:
> 
> create.c(184) : warning C4146: unary minus operator applied to unsigned type, 
> result still unsigned
> create.c(195) : warning C4146: unary minus operator applied to unsigned type, 
> result still unsigned

Those are just warnings.  The warnings are bogus, so we can ignore
them.

> Then there are a bunch of potential problems with conversion
> between double and uintmax_t (both directions).  How about
> long double (ANSI C 1990 6.1.2.5)?

I'm afraid that doesn't solve the problem either, since long double
might be equivalent to double.

> human.c(190) : error C2520: conversion from unsigned __int64 to double not 
> implemented, use signed __int64
> In this case, defining uintmax_t to (presumably unsigned) long would
> seem a rather extreme measure given that there *is* 64-bit integer
> support

There is some support for 64-bit ints, but there isn't enough support
to conform to ISO C99 or to be compatible with GCC.

We need to be pragmatic here.  I would rather not have to maintain
random bits of mainline code to support a non-GNU compiler with
limitations that will be fixed one of these days anyway.  There is a
temporarly workaround (that is, limit ourselves to 32-bit ints) that
will work OK until the compiler gets fixed.

I need to make some other changes to human.c anyway (to support "df
--block-size=1TB" and greater on 32-bit int hosts) and will see if I
can easily recode it to avoid this glitch of Microsoft's.  If not,
then I think we can just live with the temporary workaround until
Microsoft comes out with a C99 compiler.

> >  ...
> >  time_t mktime ();
> >  ...
> 
> That ought to work OK, but please include "extern".

The "extern" is just a style thing, right?  The C standard says that
it is not needed, and I'd rather omit it.

> > and it might be out of range for uintmax_t.
> 
> Not if I understand the intent of "value < (uintmax_t) -1" in
> the enclosing if condition.

True; I missed that.

> there probably ought to be a macro defined for the maximum value of
> uintmax_t (a la INT_MAX) rather than the (uintmax_t) -1 cast.

The macro's name is UINTMAX_MAX, and it's defined by C99.  That will
be easy enough to add in the rewrite I mentioned above.

> is that there ought to be some consistency within the package;

I'm attempting to rewrite everything using "T const" rather than
"const T", as that is more consistent in the C language (e.g. when "T"
is "U*").  I haven't gotten around to converting everything, though.

> I'm uncomfortable with the "used + entry_size < used"
> and "2 * allocated < allocated" conditionals, which
> depend on overfow handling (not signed vs. unsigned).

Since all the quantities are unsigned, they depend on unsigned
overflow handling, which is well defined in C to be modulo arithmetic.
(Actually, there is a theoretical problem if size_t is narrower than
int, but that doesn't happen on any host so I would rather not worry
about it.)

> Try
> ... || (defined (_AST_VERSION) && (_AST_VERSION <= 20020214L))
> which should work when AST is used on non-UWIN systems also.

OK.  Is the following patch what you had in mind?  This is academic
for tar, since I'm going to remove unicodeio.c from it; but that
source file is used by other packages so I'd like to propose a fix to
the author.

--- unicodeio.c 2002/02/11 14:28:09     1.7
+++ unicodeio.c 2002/07/15 06:51:19
@@ -194,9 +194,11 @@ unicode_to_mb (unsigned int code,
          )
        return failure (code, NULL, callback_arg);
 
-      /* Avoid glibc-2.1 bug and Solaris 2.7 bug.  */
-# if defined _LIBICONV_VERSION \
-    || !((__GLIBC__ - 0 == 2 && __GLIBC_MINOR__ - 0 <= 1) || defined __sun)
+      /* Avoid bug in glibc 2.1, Solaris 2.7, and AST 20020214.  */
+# if (defined _LIBICONV_VERSION \
+      || !((__GLIBC__ - 0 == 2 && __GLIBC_MINOR__ - 0 <= 1) \
+          || defined __sun \
+          || (defined _AST_VERSION && _AST_VERSION <= 20020214L)))
 
       /* Get back to the initial shift state.  */
       res = iconv (utf8_to_local, NULL, NULL, &outptr, &outbytesleft);


> Alternatively, errno could be checked for EEXIST (legitimate
> cause for an aborted extraction with O_CREAT | O_EXCL) but without
> aborting the extraction otherwise.

That's what the code does, I thought.

> This also might be a UWIN bug in the open() implementation.

>From your later message, this appears to be the case.

> AST fnmatch.h provides extensions which use bits through 0x0040
> (a.k.a. 1<<6).

Does AST fnmatch.h support FNM_LEADING_DIR and FNM_CASEFOLD correctly?
If not, we should be using the fnmatch.h substitute anyway.

> > 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.
> 
> How so? The buffer is allocated (and reallocated) to
> bufen + 1.

Good point.  OK, I'll put in that patch.

> This relates to the change to use INT_MAX rather than relying
> on overflow.

OK, that change shouldn't be needed so we can omit it.

Thanks for your detailed comments; they're quite helpful.