[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389
From: |
Eli Zaretskii |
Subject: |
Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389 |
Date: |
Fri, 03 Mar 2017 16:54:43 +0200 |
> 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.
- [Bug-wget] Fwd: PATCH: bugs 20369 and 20389, Vijo Cherian, 2017/03/03
- Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389,
Eli Zaretskii <=
- Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389, Vijo Cherian, 2017/03/03
- Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389, Vijo Cherian, 2017/03/03
- Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389, Eli Zaretskii, 2017/03/04
- Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389, Vijo Cherian, 2017/03/04
- Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389, Darshit Shah, 2017/03/05
- Re: [Bug-wget] Fwd: PATCH: bugs 20369 and 20389, Vijo Cherian, 2017/03/06