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

[Eli Zaretskii]

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