Bug: .gitconfig folder

If you have a folder named ~/.gitconfig instead of a file with that name, when you try to run some global config editing command it will fail with a wrong error message:

"fatal: Out of memory? mmap failed: No such device"

You can reproduce it:

$rm ~/.gitconfig
$mkdir ~/.gitconfig

$ls -la ~

    ...
    drwxr-xr-x 24 hit  hit       4096 may  4 12:30 .gimp-2.8
    drwxr-xr-x  2 hit  hit       4096 may 27 15:26 .gitconfig
    drwxr-xr-x  6 hit  hit       4096 may 27 14:01 github
    ...

$git config --global user.name foo
fatal: Out of memory? mmap failed: No such device

$git config --global core.editor "vim"
fatal: Out of memory? mmap failed: No such device

That indeed is a funny error message.

How about this patch?

-- >8 --
We show that message with die_errno(), but the OS is ought to know why mmap(2) failed much better than we do. There is no reason for us to say "Out of memory?" here.

Note that mmap(2) fails with ENODEV when the file you specify is not something that can be mmap'ed, so you still need to know that "No such device" can include cases like having a directory when a regular file is expected, but we can expect that a user who creates a directory to a location where a regular file is expected to be would know what s/he is doing, hopefully ;-)

 sha1_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ccc6dac..551a9e9 100644
@@ -720,7 +720,7 @@ void *xmmap(void *start, size_t length,
 		release_pack_memory(length);
 		ret = mmap(start, length, prot, flags, fd, offset);
 		if (ret == MAP_FAILED)
-			die_errno("Out of memory? mmap failed");
+			die_errno("mmap failed");
 	}
 	return ret;
 }

What is our thinking for after-fact recovery attempts?
Like we try the mmap first, if that fails we just use open to get the contents of
the file. And when open fails, we can still print a nice error message?

For config, I think we could just open and read the file in the first place. The data is not typically very big (and if you have a 3G config file and git barfs with "out of memory", I can live with that).

-Peff

The patch was meant to be a tongue-in-cheek tangent that is a vast improvement for cases where we absolutely need to use mmap but does not help the OP at all ;-) I do not think there is any need for the config reader to read the existing file via mmap interface; just open it, strbuf_read() the whole thing (and complain when it cannot) and we should be ok.

Or do we write back through the mmaped region or something?

No, I think we must never do that in our code because our compat mmap implementation uses pread(). So all maps must be MAP_PRIVATE (and our compat mmap barfs if it is not).

I started to go the strbuf_read() route, but it just felt so dirty to change the way the code works only to try to get a better error message. So here's my attempt at making it better while still using mmap. The end result is:

  $ mkdir foo
  $ git config --file=foo some.key value
  error: unable to mmap 'foo': Is a directory

Having looked through the code, I think the _ideal_ way to implement it would actually be with read() and seek(). We read through the config once (with the normal parser, which wraps stdio) and mark the offsets of chunks we want to copy to the output. Then we mmap the original (under lock, at least, so it shouldn't be racy) and output the existing chunks and any new content in the appropriate order.

So ideally writing each chunk would just be seek() and copy_fd(). But our offsets aren't quite perfect. In some cases we read backwards in our mmap to find the right cutoff point. I'm sure this is fixable given sufficient refactoring, but the config-writing code is such a tangled mess that I don't want to spend the time or risk the regressions.

[1/4]: read-cache.c: drop PROT_WRITE from mmap of index [2/4]: config.c: fix mmap leak when writing config [3/4]: config.c: avoid xmmap error messages [4/4]: config.c: rewrite ENODEV into EISDIR when mmap fails

-Peff

Once upon a time, git's in-memory representation of a cache entry actually pointed to the mmap'd on-disk data. So in 520fc24 (Allow writing to the private index file mapping., 2005-04-26), we specified PROT_WRITE so that we could tweak the entries while we run (in our own MAP_PRIVATE copy-on-write version, of course).

Later, 7a51ed6 (Make on-disk index representation separate from in-core one, 2008-01-14) stopped doing this; we copy the data into our in-core representation, and then drop the mmap immediately. We can therefore drop the PROT_WRITE flag. It's probably not hurting anything as it is, but it's potentially confusing.

Note that we could also mark the mapping as "const" to verify that we never write to it. However, we don't typically do that for our other maps, as it then requires casting to munmap() it.

---
This one obviously is not necessary for the rest of it, but just something I noticed while writing my response to you. But read to the end of the series; there might be a twist ending that brings it back!

 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 723d48d..5dee4e2 100644
@@ -1562,7 +1562,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
 	if (mmap_size < sizeof(struct cache_header) + 20)
 		die("index file smaller than expected");
 
-	mmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
+	mmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	if (mmap == MAP_FAILED)
 		die_errno("unable to map index file");
 	close(fd);

2.4.2.668.gc3b1ade.dirty

We mmap the existing config file, but fail to unmap it if we hit an error. The function already has a shared exit path, so we can fix this by moving the mmap pointer to the function scope and clearing it in the shared exit.

---
 config.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

index ab46462..6917100 100644
@@ -1939,6 +1939,8 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	int ret;
 	struct lock_file *lock = NULL;
 	char *filename_buf = NULL;
+	char *contents = NULL;
+	size_t contents_sz;
 
 	/* parse-key returns negative; flip the sign to feed exit(3) */
 	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);

@@ -1988,8 +1990,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 			goto write_err_out;
 	} else {
 		struct stat st;
-		char *contents;
-		size_t contents_sz, copy_begin, copy_end;
+		size_t copy_begin, copy_end;
 		int i, new_line = 0;
 
 		if (value_regex == NULL)

@@ -2108,8 +2109,6 @@ int git_config_set_multivar_in_file(const char *config_filename,
 					  contents_sz - copy_begin) <
 			    contents_sz - copy_begin)
 				goto write_err_out;
-
-		munmap(contents, contents_sz);
 	}
 
 	if (commit_lock_file(lock) < 0) {

@@ -2135,6 +2134,8 @@ out_free:
 	if (lock)
 		rollback_lock_file(lock);
 	free(filename_buf);
+	if (contents)
+		munmap(contents, contents_sz);
 	return ret;
 
 write_err_out:

2.4.2.668.gc3b1ade.dirty

Renaming to an existing file doesn't work on Windows network shares if the target file is open.

munmap() the old config file before commit_lock_file.

---

See https://github.com/git-for-windows/git/issues/226

Strangely, renaming to an open file works fine on local disks...

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

index 07133ef..3a23c11 100644
@@ -2153,6 +2153,9 @@ int git_config_set_multivar_in_file(const char *config_filename,
 					  contents_sz - copy_begin) <
 			    contents_sz - copy_begin)
 				goto write_err_out;
+
+		munmap(contents, contents_sz);
+		contents = NULL;
 	}
 
 	if (commit_lock_file(lock) < 0) {

2.4.3.windows.1.1.g87477f9

Hi,

ACK.

Thanks,
Dscho

Thanks for fixing this.

-Peff

The config-writing code uses xmmap to map the existing config file, which will die if the map fails. This has two downsides:

1. The error message is not very helpful, as it lacks any context about the file we are mapping:

$ mkdir foo
$ git config --file=foo some.key value
fatal: Out of memory? mmap failed: No such device

2. We normally do not die in this code path; instead, we'd rather report the error and return an appropriate exit status (which is part of the public interface documented in git-config.1).

This patch introduces a "gentle" form of xmmap which lets us produce our own error message. We do not want to use mmap directly, because we would like to use the other compatibility elements of xmmap (e.g., handling 0-length maps portably).

The end result is:

    $ git.compile config --file=foo some.key value
    error: unable to mmap 'foo': No such device
    $ echo $?
    3

---
 config.c          | 11 +++++++++--
 git-compat-util.h |  1 +
 sha1_file.c       | 15 +++++++++++----
 3 files changed, 21 insertions(+), 6 deletions(-)

index 6917100..e7dc155 100644
@@ -2053,8 +2053,15 @@ int git_config_set_multivar_in_file(const char *config_filename,
 
 		fstat(in_fd, &st);
 		contents_sz = xsize_t(st.st_size);
-		contents = xmmap(NULL, contents_sz, PROT_READ,
-			MAP_PRIVATE, in_fd, 0);
+		contents = xmmap_gently(NULL, contents_sz, PROT_READ,
+					MAP_PRIVATE, in_fd, 0);
+		if (contents == MAP_FAILED) {
+			error("unable to mmap '%s': %s",
+			      config_filename, strerror(errno));
+			ret = CONFIG_INVALID_FILE;
+			contents = NULL;
+			goto out_free;
+		}
 		close(in_fd);
 
 		if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) {

index 17584ad..0cc7ae8 100644
@@ -718,6 +718,7 @@ extern char *xstrndup(const char *str, size_t len);
 extern void *xrealloc(void *ptr, size_t size);
 extern void *xcalloc(size_t nmemb, size_t size);
 extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
+extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);
 extern ssize_t xread(int fd, void *buf, size_t len);
 extern ssize_t xwrite(int fd, const void *buf, size_t len);
 extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);

index ccc6dac..73e0bc0 100644
@@ -707,8 +707,8 @@ static void mmap_limit_check(size_t length)
 		    (uintmax_t)length, (uintmax_t)limit);
 }
 
-void *xmmap(void *start, size_t length,
-	int prot, int flags, int fd, off_t offset)
+void *xmmap_gently(void *start, size_t length,
+		  int prot, int flags, int fd, off_t offset)
 {
 	void *ret;
 

@@ -719,12 +719,19 @@ void *xmmap(void *start, size_t length,
 			return NULL;
 		release_pack_memory(length);
 		ret = mmap(start, length, prot, flags, fd, offset);
-		if (ret == MAP_FAILED)
-			die_errno("Out of memory? mmap failed");
 	}
 	return ret;
 }
 
+void *xmmap(void *start, size_t length,
+	int prot, int flags, int fd, off_t offset)
+{
+	void *ret = xmmap_gently(start, length, prot, flags, fd, offset);
+	if (ret == MAP_FAILED)
+			die_errno("Out of memory? mmap failed");
+	return ret;
+}
+
 void close_pack_windows(struct packed_git *p)
 {
 	while (p->windows) {

2.4.2.668.gc3b1ade.dirty

If we try to mmap a directory, we'll get ENODEV. This translates to "no such device" for the user, which is not very helpful. Since we've just fstat()'d the file, we can easily check whether the problem was a directory to give a better message.

---
It feels a bit wrong to put this magic conversion here, and not in xmmap. But of course xmmap does not have the stat information.

Which makes me wonder if we should provide an interface that will take the whole "struct stat" rather than just the size. That's less flexible, but in most cases, we're mapping the whole file (the packfiles are the big exception, where we use a window).

We could also potentially drop some of the useless options. As of patch 1, all of our calls are PROT_READ. They must all be MAP_PRIVATE, or our pread compatibility wrapper will fail, and we never use other flags. We never request a specific address. And in a whole-file remap, the offset will always be 0. So something like:

  void *xmmap_file(int fd, struct stat *st);

would probably work. We could even do the fstat() on behalf of the caller, though they need to know the length themselves. Maybe:

  void *xmmap_file(int fd, size_t *len);

I dunno if it is worth it or not.

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

index e7dc155..29fa012 100644
@@ -2056,6 +2056,8 @@ int git_config_set_multivar_in_file(const char *config_filename,
 		contents = xmmap_gently(NULL, contents_sz, PROT_READ,
 					MAP_PRIVATE, in_fd, 0);
 		if (contents == MAP_FAILED) {
+			if (errno == ENODEV && S_ISDIR(st.st_mode))
+				errno = EISDIR;
 			error("unable to mmap '%s': %s",
 			      config_filename, strerror(errno));
 			ret = CONFIG_INVALID_FILE;

2.4.2.668.gc3b1ade.dirty

I am OK if you want to switch it. Certainly EISDIR produces good output on my system, but I don't know if that is universal.

We also know S_ISDIR(st.st_mode) _before_ we actually mmap. So I was tempted to simply check it beforehand, under the assumption that the mmap cannot possibly work if we have a directory. But by doing it in the error code path, then we _know_ we are not affecting the outcome, only the error message. :)

-Peff

Well, even if your mmap() worked on a directory, having ~/.gitconfig as a directory is wrong in the first place, so...

I think what you sent is good as-is, so let's go with them.

Hmm. I actually thought that we long time ago updated the system to read small loose object files via read(2) instead of mmap(2) purely as an optimization, as mmap(2) is a bad match if you are going to read the whole thing from the beginning to the end anyway, and the "why not strbuf_read() the whole configuration file" was a suggestion along that line.

But apparently we do not have such an optimization in read_object() codepath, perhaps I was hallucinating X-<.

That part I agree with. I was kinda hoping that the previous GSoC would clean it up, but that did not happen.