Re: Patches for GNU tar 1.13.25

[Bruce Lilly]

Paul Eggert wrote:
> > Date: Sun, 14 Jul 2002 15:41:26 -0400
> > From: Bruce Lilly <address@hidden>
[...]
> > 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.

Hmmm. 2 points:

1. I have little confidence that MS will fix problems.  I reported bugs
   in their strtod() and atof() years ago (they're not compliant with
   ANSI C 1990 and later; briefly "3d3" becomes 3000.0 instead of 3.0),
   and the bug hasn't been fixed to date and is unlikely to ever be
   fixed, AFAICT.

2. MS either already has dropped support for VC++ 6.0 or will do so
   soon.  I'm unlikely to get MS ".NET", so any "temporary workaround"
   will pretty much have to be permanent.  I know of others who are in
   the same boat as we are fed up with MS's shoddy support (see point
   #1 above).

[is that 1TB or 1TiB? :-)]


> > > ...
> > > 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.

Yes, it seems to be OK w/o extern.

[...]
> > 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.

There's currently no UINTMAX_MAX in any of the UWIN or MS headers,
so configure ought to detect that and probably supply a suitable
definition.

> 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.

You might want to check whether cscope, cflow, lclint, etc. all
work with that before proceeding.

[...]
> 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);

That should work.

> > 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.

Under the specific conditions where this shows up (accessing a
networked drive hosted on Linux via SAMBA from UWIN, where the
Linux directories have sgid and sticky bits set), the UWIN
open() sets errno to EACCES and returns -1 (but after creating
a zero-length file).  maybe_recoverable() in src/extract.c does
not handle EACCES (i.e. it doesn't do anything if errno is EACCES
other than return 0).  Even if it did, the code would simply go
back to the open() with the same flags, which would then fail
with EEXIST (since the file *was* created the first time).  Other
than maybe_recoverable, there's nothing between the open() and
open_error().  The net result w/o the patch under the conditions
mentioned is that during extraction GNU tar creates empty files
(after removing existing files) in place of files in the archive.

I don't yet know if the UWIN open() can be modified to handle the
specific conditions differently, or if so, when that might happen.
I don't know if/how similar products (Cygwin, etc.) would handle
the situation.  For that matter, I don't know if there are similar
issues with Linux-Linux using SAMBA or NFS, or Linux-Windows using
SAMBA. And there may be issues with links (I know that UWIN supports
symlinks on local drives and hard links on NTFS filesystems, and
that it simulates hard links on FAT filesystems, but it's unclear
what would happen via SAMBA; I haven't checked).

Anyway, I think this probably falls into the "FIXME" w.r.t.
permissions that's in src/extract.c.  There are a bunch of
possible values for errno after a failed open that aren't
handled.

> > 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.

"correctly" is difficult to answer due to the memory leak.
It does define those macros, and the header file mentions
"gnu compatibility".

[...]

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

My pleasure.

Best regards,
  Bruce Lilly