Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389

[Vijo Cherian]

Thank you for the review, Eli Zaretskii.

Will a function like below be more acceptable ?
I've no way to test VMS and no ready access to development on windows :

bool
file_exists_p (const char *filename, file_stats_t *fstats)
{
  struct stat buf;

#if defined(WINDOWS) || defined(__VMS)
  errno = 0;
  if (stat (filename, &buf) == 0 && S_ISREG(buf.st_mode) &&
              (((S_IRUSR & buf.st_mode) && (getuid() == buf.st_uid))  ||
               ((S_IRGRP & buf.st_mode) && group_member(buf.st_gid))  ||
                (S_IROTH & buf.st_mode))) {
    if (fstats != NULL)
    {
      fstats->access_err = 0;
      fstats->st_ino = buf.st_ino;
      fstats->st_dev = buf.st_dev;
    }
    return true;
  }
  else
  {
    if (fstats != NULL)
      fstats->access_err = (errno == 0 ? EACCES : errno);
    errno = 0;
    return false;
  }
  __builtin_unreachable();
  /* NOTREACHED */
#else
  return stat (filename, &buf) >= 0;
#endif
}

Best regards,
Vijo

On Fri, Mar 3, 2017 at 6:54 AM, Eli Zaretskii <address@hidden> wrote:

> > From: Vijo Cherian <address@hidden>
> > Date: Thu, 2 Mar 2017 18:47:11 -0800
> >
> > Changes
> >   - Bug #20369 - Safeguards against TOCTTOU
> >     Added safe_fopen() and safe_open() that checks to makes sure the file
> >     didn't change underneath us.
> >   - Bug #20389 - Return error from file_exists_p()
> >     Added a way to return error from this file without major surgery to
> >     the callers.
>
> Allow me a few comments to your patch.
>
> > +  errno = 0;
> > +  if (stat (filename, &buf) >= 0 && S_ISREG(buf.st_mode) &&
>
> 'stat' is documented to return 0 upon success, so I don't think a
> positive return value should be considered a success.
>
> > +              (((S_IRUSR & buf.st_mode) && (getuid() == buf.st_uid))  ||
> > +               ((S_IRGRP & buf.st_mode) && group_member(buf.st_gid))  ||
> > +                (S_IROTH & buf.st_mode))) {
>
> These tests assume Posix semantics, and will be too restrictive on
> MS-Windows, for example.
>
> > +    if (fstats != NULL) {
> > +      logprintf (LOG_VERBOSE, _("File %s exists, but NULL for
> fstats\n"), filename);
>
> The log message says fstats is NULL, but it isn't.
>
> > +      fstats->access_err = 0;
> > +      fstats->st_ino = buf.st_ino;
> > +      fstats->st_dev = buf.st_dev;
> > +    }
> > +    logprintf (LOG_VERBOSE, _("%s exists!!\n"), filename);
> > +    return true;
> > +  } else {
> > +    if (fstats != NULL) {
> > +      fstats->access_err = (errno == 0 ? EACCES : errno);
> > +      logprintf (LOG_VERBOSE, _("File %s is not accessible\n"),
> filename);
> > +    }
> > +    logprintf (LOG_VERBOSE, _("File %s doesn't exist\n"), filename);
> > +    errno = 0;
> > +    return false;
>
> Do we really need such detailed log messages for such a trivial check?
>
> Also, the name of the function and its commentary seem to no longer
> describe what it actually does.  The commentary should also describe
> the return value.
>
> > +/* Safe_fopen assumes that file_exists_p() was called earlier.
>
> The name of the function doesn't describe what it does.
>
> Also, instead of "assumes that file_exists_p() was called earlier",
> I'd suggest to state that the FSTATS argument should be available,
> e.g. by calling file_exists_p.
>
> > +  if (fstats != NULL &&
> > +      (fdstats.st_dev != fstats->st_dev ||
> > +       fdstats.st_ino != fstats->st_ino)) {
>
> These are Posix assumptions; on Windows you will get meaningless
> results from such a test.  I suggest to have a function for this, with
> different implementations on Posix and non-Posix platforms.
>
> Same comments for safe_open.
>
> Thanks for working on this.
>