From a1293ef7c3690829a6ac47fc45f3f26b96b5c9f5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 May 2015 03:54:00 -0400 Subject: [PATCH 1/5] read-cache.c: drop PROT_WRITE from mmap of index 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. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- read-cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/read-cache.c b/read-cache.c index 9cff715d6b..cc67dd1d5d 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1525,7 +1525,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); From 3a1b3126ed5a0b51d5b1fdba827c92bf2acf5fc6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 May 2015 03:54:43 -0400 Subject: [PATCH 2/5] config.c: fix mmap leak when writing config 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. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/config.c b/config.c index 752e2e227f..dc7f8b2abc 100644 --- a/config.c +++ b/config.c @@ -1934,6 +1934,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); @@ -1983,8 +1985,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) @@ -2103,8 +2104,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) { @@ -2130,6 +2129,8 @@ out_free: if (lock) rollback_lock_file(lock); free(filename_buf); + if (contents) + munmap(contents, contents_sz); return ret; write_err_out: From 1570856b510e3722a3620063e7ba209106b75857 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 May 2015 03:56:15 -0400 Subject: [PATCH 3/5] config.c: avoid xmmap error messages 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 11 +++++++++-- git-compat-util.h | 1 + sha1_file.c | 15 +++++++++++---- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/config.c b/config.c index dc7f8b2abc..d348d792aa 100644 --- a/config.c +++ b/config.c @@ -2048,8 +2048,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) { diff --git a/git-compat-util.h b/git-compat-util.h index 400e921086..dc1948aab0 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -623,6 +623,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); diff --git a/sha1_file.c b/sha1_file.c index d7f1838c13..ca491ab8bb 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -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) { From 0e8771f1984c468b0f41a8c779c034ffc48530e5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 28 May 2015 04:03:01 -0400 Subject: [PATCH 4/5] config.c: rewrite ENODEV into EISDIR when mmap fails 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. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config.c b/config.c index d348d792aa..6551de30db 100644 --- a/config.c +++ b/config.c @@ -2051,6 +2051,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; From 9ca0aaf6de357d46916d81ca40c47886092fe610 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 27 May 2015 13:30:29 -0700 Subject: [PATCH 5/5] xmmap(): drop "Out of memory?" 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. Signed-off-by: Junio C Hamano --- sha1_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sha1_file.c b/sha1_file.c index ca491ab8bb..1457069a1a 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -728,7 +728,7 @@ void *xmmap(void *start, size_t length, { void *ret = xmmap_gently(start, length, prot, flags, fd, offset); if (ret == MAP_FAILED) - die_errno("Out of memory? mmap failed"); + die_errno("mmap failed"); return ret; }