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

[Vijo Cherian]

Sorry about the mistake in having logic around #if reversed.
Here is what I meant :

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

#if defined(WINDOWS) || defined(__VMS)
    return stat (filename, &buf) >= 0;
#else
  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 */
#endif
}

On Fri, Mar 3, 2017 at 10:30 AM, Vijo Cherian <address@hidden> wrote:

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