Re: [PATCH 1/2] config: check if config path is a file before parsing it

From: Jeff King <peff@peff.net>
Date: Mar 3, 2017, 10:15 UTC
Message-ID: <20170303101503.lf2ub2c7i6w7kg3t@sigill.intra.peff.net>
In-Reply-To: <CACsJy8DU7-o06mfuw1L02CFFR2wmoNa0MQJ8KqsV79ULzjRaRQ@mail.gmail.com>

On Fri, Mar 03, 2017 at 05:06:57PM +0700, Duy Nguyen wrote:

Show 11 quoted lines

> > But if we do, I think we'd either want to:
> >
> >        a. actually check ferror() after getting EOF and report the read
> >           error. That catches EISDIR, along with any other unexpected
> >           errors.
> >
> >        b. use an fopen wrapper that checks fstat(fileno(fh)) after the
> >           open, and turns fopen(some_dir) into an error.
> 
> If you don't like extra check, I guess you're negative on b as well
> since it is an extra check on Windows. That leaves us with option a.

I don't mind _doing_ the extra check that much. I don't think we fopen so many files that an extra fstat on each would kill us. I mostly just don't like having to sprinkle the explicit call to it everywhere. I'd be OK with:

  FILE *xfopen(const char *path, const char *mode)
  {
	FILE *ret = fopen(path, mode);
  #ifdef FOPEN_OPENS_DIRECTORIES
	if (ret) {
		struct stat st;
		if (!fstat(fileno(ret), &st) && S_ISDIR(st.st_mode)) {
			fclose(ret);
			ret = NULL;
		}
	}
  #endif
	return ret;
  }

But I do think option (a) is cleaner. The only trick is that for errno to be valid, we need to make sure we check ferror() soon after seeing the EOF return value. I suspect it would work OK in practice for the git_config_from_file() case.

-Peff