RE: bug#10305: coreutils-8.14, "rm -r" fails with EBADF

[Joachim Schmitz]

> From: Paul Eggert [mailto:address@hidden
> Sent: Wednesday, June 27, 2012 2:49 AM
> To: Joachim Schmitz
> Cc: address@hidden; address@hidden; 'Eric Blake'; 'Jim Meyering'
> Subject: Re: bug#10305: coreutils-8.14, "rm -r" fails with EBADF
> 
> On 06/26/2012 09:38 AM, Joachim Schmitz wrote:
> 
> > Let me know what you think and where/how you'd do it differently.
> 
> The changes mostly look good.  The trivial ones we've incorporated already.  I
> have some comments on the nontrivial ones (please see below).
> But before we get into it too much further, are you and your company willing 
> to
> assign copyright to your nontrival changes to the FSF?
> If so, I can send you more info about how to do that.

I'd need to check, will pursue this in a different email.
 
> Here are some comments about that patch:
> 
> 
> > --- ./configure.orig        2011-03-12 03:50:18.000000000 -0600
> > +++ ./configure     2012-06-26 06:49:17.000000000 -0500
> For future versions of this patch there's no need to show differences in
> automatically-generated files like 'configure'.

That change then needs to go into m4/dirfd.m4, right?

> > --- ./gnu/argp-help.c.orig  2011-03-12 03:14:26.000000000 -0600
> > +++ ./gnu/argp-help.c       2011-06-16 02:01:23.000000000 -0500
> 
> This one Bruno just now fixed in a different way:
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=2457d7ca685
> 6f84502b09fa4690f6f4187de050f

Fine by me
 
> > --- ./gnu/regex.c.orig      2011-03-12 03:14:32.000000000 -0600
> > +++ ./gnu/regex.c   2011-06-17 04:07:16.000000000 -0500
> 
> This one we also fixed in a different way:
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=d4903bb0efa
> c5e399b785c71367d8cda3fb539ab

Fine too.
 
> > --- ./gnu/dirfd.c.orig      2011-03-12 03:14:28.000000000 -0600
> > +++ ./gnu/dirfd.c   2012-06-25 02:55:09.000000000 -0500
> > ...
> > +#ifdef __TANDEM
> > +# include <unistd.h> /* for _gl_fnum2dt(), needed in C99 mode */
> > +#endif
> 
> Shouldn't that be "_gl_fnum2fd"?

Yes, of course, stupid typo.
 
> More important, doesn't the declaration of _gl_fnum2fd belong better in
> dirent.h, not unistd.h?  Among other things, that would mean the above change
> can be omitted.

My attempts to integrate this into coreutils-8.17 seem to indicate that this 
function and a couple more are used elsewhere too
Looks like there closedir.o needs _gl_ungerister_fnum(), dirfd.c needs 
_gl_fnum2ds() and opendir.c needs _gl_register_fnum().

> >  int
> >  dirfd (DIR *dir_p)
> >  {
> >    int fd = DIR_TO_FD (dir_p);
> >    if (fd == -1)
> >      errno = ENOTSUP;
> > +#ifdef __TANDEM
> > +  fd = _gl_fnum2fd(fd);
> > +#endif
> 
> This might be cleaner if DIR_TO_FD invoked _gl_fnum2fd directly.
> That way, dirfd.c could be left alone.  (Or perhaps not; I don't understand 
> the
> code that well.)

Won't that make that macro too complicated?
 
> > +# define NOFNUM      -1
> 
> What's this for?  I don't see it used anywhere.

Indeed, scratch it.
 
> > +# define NOFD        -1
> 
> Body needs to be parenthesized.

Point taken.

> > +  char fnum;        /* 'y' or 'n', actually a bool */
> 
> Why not use 1 and 0?  That's far more typical for boolean values, and
> generates better code.

True. But the corresponding member in dir_info_t should remain a char (or 
signed char?), to save space, shouldn't it?

Maybe we could also make it a short and place fnum right there? That would 
change the implementation quite a bit though, but might make it leaner too. And 
the comment
/* FIXME - add a DIR* member to make dirfd possible on mingw?  */
Indicates that there is a change pending for a similar purpose on a different 
platform...

> > +#ifdef __TANDEM
> > +      || (stat (filename, &statbuf) == 0 && S_ISDIR
> > +(statbuf.st_mode))) 
>> +#else
> >        || (fstat (fd, &statbuf) == 0 && S_ISDIR (statbuf.st_mode)))
> > +#endif
>
> fstat doesn't work on Tandem?  I must be missing something here.

This is a special case for those "dummy directories" from the patch chunk below.
And yes, that warrants a comment in the above patch chunk ;-)
 
> >    fd = orig_open (filename, flags, mode);
> > +#ifdef __TANDEM
> > +   /* On NonStop open(2) can open an OSS directory, but not /G & /E
> > +    * directory, hence we do a dummy open here and override fstat() in
> > +    * fchdir.c to hide the fact that we have a dummy.
> > +    */
> > +  if (fd < 0 && errno == EISDIR)
> > +     fd = open ("/dev/null", flags, mode); 
> > +#endif
> 
> If 'flags' contains O_WRONLY or O_RDWR, this will misbehave on an OSS
> directory, since open will fail with errno == EISDIR but we don't want to 
> replace
> it with /dev/null.  So the fallback needs to be conditioned on the file being
> opened read-only.

These cases are handeld furter above in the (existing) code:

  if (flags & (O_CREAT | O_WRONLY | O_RDWR))
    {
      size_t len = strlen (filename);
      if (len > 0 && filename[len - 1] == '/')
        {
          errno = EISDIR;
          return -1;
        }
    }
#endif

  fd = orig_open (filename, flags, mode);
#ifdef __TANDEM
        /* On NonStop open(2) can open an OSS directory, but not /G & /E
         * directory, hence we do a dummy open here and override fstat() in
         * fchdir.c to hide the fact that we have a dummy.
         */
  if (fd < 0 && errno == EISDIR)
     fd = open ("/dev/null", flags, mode);
#endif

> > --- ./gnu/unlinkdir.c.orig  2011-03-12 03:14:34.000000000 -0600
> > +++ ./gnu/unlinkdir.c       2012-06-26 08:46:41.000000000 -0500
> > ...
> > --- ./src/extract.c.orig    2010-11-27 04:33:22.000000000 -0600
> > +++ ./src/extract.c 2011-06-16 01:55:46.000000000 -0500
> 
> I just now fixed this in a different way in gnulib and tar master.

Good. Where does that ROOT_UID get set?

> > /usr/local/bin/diff -EBbu ./tests/genfile.c.orig ./tests/genfile.c
> > --- ./tests/genfile.c.orig  2010-10-24 13:06:45.000000000 -0500
> > +++ ./tests/genfile.c       2011-06-16 01:55:46.000000000 -0500
> > @@ -610,9 +610,17 @@
> >        else if (strcmp (p, "size") == 0)
> >     printf ("%s", umaxtostr (st.st_size, buf));
> >        else if (strcmp (p, "blksize") == 0)
> > +#ifdef __TANDEM
> > +   printf ("*****Nothing to print on NonStop for st_blksize****\n");
> > +#else
> >     printf ("%s", umaxtostr (st.st_blksize, buf));
> > +#endif
> >        else if (strcmp (p, "blocks") == 0)
> > +#ifdef __TANDEM
> > +   printf ("*****Nothing to print on NonStop for st_blocks****\n");
> > +#else
> >     printf ("%s", umaxtostr (st.st_blocks, buf));
> > +#endif
> >        else if (strcmp (p, "atime") == 0)
> >     printf ("%lu", (unsigned long) st.st_atime);
> >        else if (strcmp (p, "atimeH") == 0)
> 
> I assume st_blksize and st_blocks are garbage on Tandem?
> Is there any harm in printing the garbage?

No, we don't have those struct members at all. Instead of #ifdef __TANDEM it 
should probably better be #if HAVE_STAT_ST_BLOCKS and #if 
HAVE_STRUCT_STAT_ST_BLKSIZE or some such and remain silent if not, maybe like 
this:

      else if (strcmp (p, "blksize") == 0)
#if HAVE_STRUCT_STAT_ST_BLKSIZE
        printf ("%s", umaxtostr (st.st_blksize, buf));
#endif
      else if (strcmp (p, "blocks") == 0)
#if HAVE_STRUCT_STAT_ST_BLOCKS
        printf ("%s", umaxtostr (st.st_blocks, buf));
#endif

Bye, Jojo