> From: Vijo Cherian <coderv...@gmail.com> > 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.