[PATCH 0/2] Improve error messages when opening a directory as file

[PATCH 0/2] Improve error messages when opening a directory as file

[Nguyễn Thái Ngọc Duy]

The topic nd/conditional-config-include hit a problem on Windows [1].
The test basically does this (much simplified)

    echo '[include]path=foo' >~/.gitconfig
    cd ~ && git init foo

At some point in 'git init' after 'foo' directory has been created, we
request to include ~/foo as an extra config file. But it's a directory
and we get some error like this

    fatal: bad config line 2 in file ~/.gitconfig

The message gives no clue that 'foo' is a directory (and probably
wasted a good chunk of time of Johannes). This series tells the user
about that.

The other half of the problem is, the same test runs without error on
Linux because it looks like fopen(dir) returns NULL on Windows, but
non-NULL on Linux and only subsequent read() returns EISDIR.
Unfortunately the config parser conflates errors with eof, I think.
And it simply sees <dir> as an empty config file, ie. a valid config
file. So no "bad config line..."

I'm making sure even Linux now reports loud and clear that config
files should be _files_. The same treatment is done for .gitattributes.
I'm not so sure about .gitignore because it uses open(), not fopen()
and I don't know if open() behaves differently on Windows.

I briefly considered fopen() and open() wrappers that always rejects
directories (if you need to open a directory, do it without wrappers),
but discarded it because it adds an extra stat() call everywhere.

[1] https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33#commitcomment-21121210

Nguyễn Thái Ngọc Duy (2):
  config: check if config path is a file before parsing it
  attr.c: check if .gitattributes is a file before parsing it

 abspath.c              | 7 +++++++
 attr.c                 | 8 +++++++-
 cache.h                | 1 +
 config.c               | 9 +++++++++
 t/t1300-repo-config.sh | 5 +++++
 5 files changed, 29 insertions(+), 1 deletion(-)

-- 
2.11.0.157.gd943d85

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

[Nguyễn Thái Ngọc Duy]

If a directory is given as a config file by accident, we keep open it
as a file. The behavior of fopen() in this case seems to be
undefined.

On Linux, we open a directory as a file ok, then get error (which we
consider eof) on the first read. So the config parser sees this "file"
as empty (i.e. valid config). All is well and we don't complain
anything (but we should).

The situation is slighly different on Windows. I think fopen() returns
NULL. And we get a very unhelpful message:

    $ cat >abc <<EOF
    [include]
        path = /tmp/foo
    EOF
    $ mkdir /tmp/foo
    $ git config --includes --file=abc core.bare
    fatal: bad config line 3 in file abc

Opening a directory is wrong in the first place. Avoid it. If caught,
print something better. With this patch, we have

    $ git config --includes --file=abc core.bare
    error: '/tmp/foo' is not a file
    fatal: bad config line 3 in file abc

It's not perfect (line should be 2 instead of 3). But it's definitely
improving.

The new test is only relevant on linux where we blindly open the
directory and consider it an empty file. On Windows, the test should
pass even without this patch.
---
 abspath.c              | 7 +++++++
 cache.h                | 1 +
 config.c               | 9 +++++++++
 t/t1300-repo-config.sh | 5 +++++
 4 files changed, 22 insertions(+)

diff --git a/abspath.c b/abspath.c
index 2f0c26e0e2..373cc3abb2 100644
--- a/abspath.c
+++ b/abspath.c
@@ -11,6 +11,13 @@ int is_directory(const char *path)
 	return (!stat(path, &st) && S_ISDIR(st.st_mode));
 }
 
+int is_not_file(const char *path)
+{
+	struct stat st;
+
+	return !stat(path, &st) && !S_ISREG(st.st_mode);
+}
+
 /* removes the last path component from 'path' except if 'path' is root */
 static void strip_last_component(struct strbuf *path)
 {
diff --git a/cache.h b/cache.h
index 80b6372cf7..bdd1402ab9 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,6 +1149,7 @@ static inline int is_absolute_path(const char *path)
 	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
 }
 int is_directory(const char *);
+int is_not_file(const char *);
 char *strbuf_realpath(struct strbuf *resolved, const char *path,
 		      int die_on_error);
 const char *real_path(const char *path);
diff --git a/config.c b/config.c
index c6b874a7bf..c21c0ce433 100644
--- a/config.c
+++ b/config.c
@@ -13,6 +13,7 @@
 #include "hashmap.h"
 #include "string-list.h"
 #include "utf8.h"
+#include "dir.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -1212,6 +1213,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	int ret = -1;
 	FILE *f;
 
+	if (is_not_file(filename))
+		return error(_("'%s' is not a file"), filename);
+
 	f = fopen(filename, "r");
 	if (f) {
 		flockfile(f);
@@ -2451,6 +2455,11 @@ int git_config_rename_section_in_file(const char *config_filename,
 		goto out;
 	}
 
+	if (!S_ISREG(st.st_mode)) {
+		ret = error(_("'%s' is not a file"), config_filename);
+		goto out;
+	}
+
 	if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {
 		ret = error_errno("chmod on %s failed",
 				  get_lock_file_path(lock));
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 052f120216..3683fbb84e 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1477,4 +1477,9 @@ test_expect_success !MINGW '--show-origin blob ref' '
 	test_cmp expect output
 '
 
+test_expect_success 'a directory is given as a config file' '
+	mkdir config-dir &&
+	test_must_fail git config --file=config-dir --list
+'
+
 test_done
-- 
2.11.0.157.gd943d85

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

[Jeff King]

On Fri, Mar 03, 2017 at 04:42:51PM +0700, Nguyễn Thái Ngọc Duy wrote:

> If a directory is given as a config file by accident, we keep open it
> as a file. The behavior of fopen() in this case seems to be
> undefined.
> 
> On Linux, we open a directory as a file ok, then get error (which we
> consider eof) on the first read. So the config parser sees this "file"
> as empty (i.e. valid config). All is well and we don't complain
> anything (but we should).
> 
> The situation is slighly different on Windows. I think fopen() returns
> NULL. And we get a very unhelpful message:
> 
>     $ cat >abc <<EOF
>     [include]
>         path = /tmp/foo
>     EOF
>     $ mkdir /tmp/foo
>     $ git config --includes --file=abc core.bare
>     fatal: bad config line 3 in file abc
> 
> Opening a directory is wrong in the first place. Avoid it. If caught,
> print something better. With this patch, we have
> 
>     $ git config --includes --file=abc core.bare
>     error: '/tmp/foo' is not a file
>     fatal: bad config line 3 in file abc
> 
> It's not perfect (line should be 2 instead of 3). But it's definitely
> improving.
> 
> The new test is only relevant on linux where we blindly open the
> directory and consider it an empty file. On Windows, the test should
> pass even without this patch.

I'm mildly negative on this approach for two reasons:

  1. It requires doing an _extra_ check anywhere we want to care about
     this. So if we care about file/directory confusion, we're going to
     sprinkle these is_not_file() checks all over the code base.

     I think we're much better to just do the thing we want to do (like
     open the file), and deal with the error results. I'm on the fence
     on whether we want to care about the fopen behavior on Linux here
     (where reading a directory essentially behaves like an empty file,
     because the first read() gives an error and we don't distinguish
     between error and EOF).  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.

  2. It doesn't address the root problem for git_config_from_file(),
     which is that it is quiet when fopen fails, even if the reason is
     something interesting besides ENOENT. The caller can't check errno
     because it doesn't know if fopen() failed, or if the config
     callback returned an error.

     There's an attempt to protect the call to git_config_from_file() by
     checking access(), but that breaks down when access() and fopen()
     have two different results (which is exactly what happens on
     Windows in this case).

-Peff

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

[Duy Nguyen]

On Fri, Mar 3, 2017 at 4:53 PM, Jeff King <peff@peff.net> wrote:
> I'm mildly negative on this approach for two reasons:
>
>   1. It requires doing an _extra_ check anywhere we want to care about
>      this. So if we care about file/directory confusion, we're going to
>      sprinkle these is_not_file() checks all over the code base.
>
>      I think we're much better to just do the thing we want to do (like
>      open the file), and deal with the error results. I'm on the fence
>      on whether we want to care about the fopen behavior on Linux here
>      (where reading a directory essentially behaves like an empty file,
>      because the first read() gives an error and we don't distinguish
>      between error and EOF).

I can't fix problems of my series on Windows because I don't use
Windows (because I will not be able to verify it). So I'm definitely
on the side that makes behavior consistent across platforms. Then I
can at least verify some (assuming that the consistent behavior is the
right one).

I didn't go with yours because I would have to handle two separate
code paths (fopen returns NULL and read returns EISDIR). But yeah it
should be that way even if it takes more time and effort. At least
we're now back on the mailing list and I didn't have to hurry to get
something out, to get off github.

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

>   2. It doesn't address the root problem for git_config_from_file(),
>      which is that it is quiet when fopen fails, even if the reason is
>      something interesting besides ENOENT. The caller can't check errno
>      because it doesn't know if fopen() failed, or if the config
>      callback returned an error.
>
>      There's an attempt to protect the call to git_config_from_file() by
>      checking access(), but that breaks down when access() and fopen()
>      have two different results (which is exactly what happens on
>      Windows in this case).
>
> -Peff
-- 
Duy

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

[Jeff King]

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

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

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

[Duy Nguyen]

On Fri, Mar 3, 2017 at 5:15 PM, Jeff King <peff@peff.net> wrote:
> 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.

stdio error handling is a pain. Maybe we're better of with open() and
mmap() (or even read_in_full)? I/O error handling would be at the
beginning, not buried deep in the parser. Hmm.. since we already have
"fgetc' version for config blobs, this could kill some code...
-- 
Duy

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

[Jeff King]

On Fri, Mar 03, 2017 at 05:29:47PM +0700, Duy Nguyen wrote:

> On Fri, Mar 3, 2017 at 5:15 PM, Jeff King <peff@peff.net> wrote:
> > 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.
> 
> stdio error handling is a pain. Maybe we're better of with open() and
> mmap() (or even read_in_full)? I/O error handling would be at the
> beginning, not buried deep in the parser. Hmm.. since we already have
> "fgetc' version for config blobs, this could kill some code...

Yeah, I don't mind a read_in_full() version. Config isn't _supposed_ to
be big (and if it is you're in trouble anyway, because I'm pretty sure
we still parse it several times per command invocation).

I don't think that removes the issues I've mentioned with
git_config_from_file() being too quiet. But it solves the ferror()
question (though I think we pretty much return immediately from the
parser on EOF, so it's _probably_ OK to use it like in the diff I just
sent).

-Peff

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

[Jeff King]

On Fri, Mar 03, 2017 at 05:15:03AM -0500, Jeff King wrote:

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

Something like this is a big improvement, I think:

diff --git a/config.c b/config.c
index c6b874a7b..27b410dfe 100644
--- a/config.c
+++ b/config.c
@@ -156,15 +156,14 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		path = buf.buf;
 	}
 
-	if (!access_or_die(path, R_OK, 0)) {
-		if (++inc->depth > MAX_INCLUDE_DEPTH)
-			die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
-			    !cf ? "<unknown>" :
-			    cf->name ? cf->name :
-			    "the command line");
-		ret = git_config_from_file(git_config_include, path, inc);
-		inc->depth--;
-	}
+	if (++inc->depth > MAX_INCLUDE_DEPTH)
+		die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
+		    !cf ? "<unknown>" :
+		    cf->name ? cf->name :
+		    "the command line");
+	ret = git_config_from_file(git_config_include, path, inc);
+	inc->depth--;
+
 	strbuf_release(&buf);
 	free(expanded);
 	return ret;
@@ -1213,10 +1212,18 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	FILE *f;
 
 	f = fopen(filename, "r");
-	if (f) {
+	if (!f) {
+		/* a missing file is silently treated as an empty one */
+		if (errno == ENOENT || errno == EISDIR)
+			ret = 0;
+		else
+			ret = error_errno("unable to open %s", filename);
+	} else {
 		flockfile(f);
 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);
 		funlockfile(f);
+		if (!ret && ferror(f))
+			ret = error_errno("unable to read from %s", filename);
 		fclose(f);
 	}
 	return ret;

Then if you do:

  cd repo.git
  git config include.path this-is-broken

you get useful errors for a variety of situations:

  $ mkdir this-is-broken
  $ git rev-parse
  error: unable to read from this-is-broken: Is a directory
  fatal: bad config line 7 in file config

  $ rmdir this-is-broken
  $ ln -s this-is-broken this-is-broken
  $ git rev-parse
  error: unable to open this-is-broken: Too many levels of symbolic links
  fatal: bad config line 7 in file config

and so on. The two caveats are:

  1. A few of the callers treat EACCES specially, so we'd potentially
     want a flag for that (or alternatively, everybody should just fopen
     the file themselves and pass in the handle).

  2. The call in read_repository_format() does not check the return
     value at all. It measures errors only as "did the parser find a
     core.repositoryformatversion field I can look at", though arguably
     it should check for other errors, too (if we read "version=2", but
     then got a read error before we were able to read the extensions,
     that would be wrong and bad).

     But either way I suspect it probably prefers the current "quiet"
     behavior, since it is used to speculatively look for repositories.

So probably git_config_from_file() needs a flags parameter, and both
"quiet" and EACCES handling can go in there.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>        a. actually check ferror() after getting EOF and report the read
>           error. That catches EISDIR, along with any other unexpected
> 	  errors.

That is the most sensible, I would think (assuming that we really
get EISDIR instead of silent EOF).

>        b. use an fopen wrapper that checks fstat(fileno(fh)) after the
>           open, and turns fopen(some_dir) into an error.

That's already an option with FREAD_READS_DIRECTORIES, I think.

>   2. It doesn't address the root problem for git_config_from_file(),
>      which is that it is quiet when fopen fails, even if the reason is
>      something interesting besides ENOENT. The caller can't check errno
>      because it doesn't know if fopen() failed, or if the config
>      callback returned an error.

Perhaps like this one as a starting point, with FREAD_READS_DIRECTORIES?

 config.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/config.c b/config.c
index 0dac0f4cb2..af8c01c8a3 100644
--- a/config.c
+++ b/config.c
@@ -1305,6 +1305,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	FILE *f;
 
 	f = fopen(filename, "r");
+	if (!f && errno != ENOENT)
+		die_errno("fopen('%s') failed", filename);
+
 	if (f) {
 		flockfile(f);
 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);

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

[Ramsay Jones]

On 03/03/17 09:42, Nguyễn Thái Ngọc Duy wrote:
> If a directory is given as a config file by accident, we keep open it
> as a file. The behavior of fopen() in this case seems to be
> undefined.
> 
> On Linux, we open a directory as a file ok, then get error (which we
> consider eof) on the first read. So the config parser sees this "file"
> as empty (i.e. valid config). All is well and we don't complain
> anything (but we should).
> 
> The situation is slighly different on Windows. I think fopen() returns
> NULL. And we get a very unhelpful message:
> 
>     $ cat >abc <<EOF
>     [include]
>         path = /tmp/foo
>     EOF
>     $ mkdir /tmp/foo
>     $ git config --includes --file=abc core.bare
>     fatal: bad config line 3 in file abc
> 
> Opening a directory is wrong in the first place. Avoid it. If caught,
> print something better. With this patch, we have
> 
>     $ git config --includes --file=abc core.bare
>     error: '/tmp/foo' is not a file
>     fatal: bad config line 3 in file abc
> 
> It's not perfect (line should be 2 instead of 3). But it's definitely
> improving.
> 
> The new test is only relevant on linux where we blindly open the
> directory and consider it an empty file. On Windows, the test should
> pass even without this patch.
> ---
>  abspath.c              | 7 +++++++
>  cache.h                | 1 +
>  config.c               | 9 +++++++++
>  t/t1300-repo-config.sh | 5 +++++
>  4 files changed, 22 insertions(+)
> 
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2..373cc3abb2 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -11,6 +11,13 @@ int is_directory(const char *path)
>  	return (!stat(path, &st) && S_ISDIR(st.st_mode));
>  }
>  
> +int is_not_file(const char *path)
> +{
> +	struct stat st;
> +
> +	return !stat(path, &st) && !S_ISREG(st.st_mode);
> +}
> +
>  /* removes the last path component from 'path' except if 'path' is root */
>  static void strip_last_component(struct strbuf *path)
>  {
> diff --git a/cache.h b/cache.h
> index 80b6372cf7..bdd1402ab9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1149,6 +1149,7 @@ static inline int is_absolute_path(const char *path)
>  	return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
>  }
>  int is_directory(const char *);
> +int is_not_file(const char *);
>  char *strbuf_realpath(struct strbuf *resolved, const char *path,
>  		      int die_on_error);
>  const char *real_path(const char *path);
> diff --git a/config.c b/config.c
> index c6b874a7bf..c21c0ce433 100644
> --- a/config.c
> +++ b/config.c
> @@ -13,6 +13,7 @@
>  #include "hashmap.h"
>  #include "string-list.h"
>  #include "utf8.h"
> +#include "dir.h"

Something is a bit odd here - nothing in this commit (that I can
see, anyway) would require the addition of this include. Also, this
include is already there in the 'pu' branch, brought in by your
conditional include changes. So, ...

ATB,
Ramsay Jones

[PATCH 2/2] attr.c: check if .gitattributes is a file before parsing it

[Nguyễn Thái Ngọc Duy]

Similar to the previous patch, this is about better error messages
when .gitattributes happens to be a directory.

FWIW .gitignore code is also checked. There open() is used instead and
open("dir") does not fail on Linux. But the next read should fail with
EISDIR, which is a pretty good clue already. No idea how open() on
Windows behaves.
---
 attr.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/attr.c b/attr.c
index 5493bff224..34b6a6b9a8 100644
--- a/attr.c
+++ b/attr.c
@@ -703,11 +703,17 @@ void git_attr_set_direction(enum git_attr_direction new_direction,
 
 static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 {
-	FILE *fp = fopen(path, "r");
+	FILE *fp;
 	struct attr_stack *res;
 	char buf[2048];
 	int lineno = 0;
 
+	if (is_not_file(path)) {
+		warning(_("'%s' is not a file"), path);
+		return NULL;
+	}
+
+	fp = fopen(path, "r");
 	if (!fp) {
 		if (errno != ENOENT && errno != ENOTDIR)
 			warn_on_inaccessible(path);
-- 
2.11.0.157.gd943d85