From 3446a59b3950d57960e27f8a2c7e41462bd2bcf4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Apr 2015 04:48:23 -0400 Subject: [PATCH 1/9] strbuf_getwholeline: use getc macro strbuf_getwholeline calls fgetc in a tight loop. Using the getc form, which can be implemented as a macro, should be faster (and we do not care about it evaluating our argument twice, as we just have a plain variable). On my glibc system, running "git rev-parse refs/heads/does-not-exist" on a file with an extremely large (1.6GB) packed-refs file went from (best of 3 runs): real 0m19.383s user 0m18.876s sys 0m0.528s to: real 0m18.900s user 0m18.472s sys 0m0.448s for a wall-clock speedup of 2.5%. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- strbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/strbuf.c b/strbuf.c index 88cafd4a7..14f337db1 100644 --- a/strbuf.c +++ b/strbuf.c @@ -443,7 +443,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) return EOF; strbuf_reset(sb); - while ((ch = fgetc(fp)) != EOF) { + while ((ch = getc(fp)) != EOF) { strbuf_grow(sb, 1); sb->buf[sb->len++] = ch; if (ch == term) From f43cce23adde58d44081e46279c7fd655d4246ec Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Apr 2015 04:48:45 -0400 Subject: [PATCH 2/9] git-compat-util: add fallbacks for unlocked stdio POSIX.1-2001 specifies some functions for optimizing the locking out of tight getc() loops. Not all systems are POSIX, though, and even not all POSIX systems are required to implement these functions. We can check for the feature-test macro to see if they are available, and if not, provide a noop implementation. There's no Makefile knob here, because we should just detect this automatically. If there are very bizarre systems, we may need to add one, but it's not clear yet in which direction: 1. If a system defines _POSIX_THREAD_SAFE_FUNCTIONS but these functions are missing or broken, we would want a knob to manually turn them off. 2. If a system has these functions but does not define _POSIX_THREAD_SAFE_FUNCTIONS, we would want a knob to manually turn them on. We can add such a knob when we find a real-world system that matches this. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- git-compat-util.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index bc8fc8cf8..685a0a406 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -883,4 +883,10 @@ struct tm *git_gmtime_r(const time_t *, struct tm *); # define SHELL_PATH "/bin/sh" #endif +#ifndef _POSIX_THREAD_SAFE_FUNCTIONS +#define flockfile(fh) +#define funlockfile(fh) +#define getc_unlocked(fh) getc(fh) +#endif + #endif From 82912d1de86ae3d7f247bc3b16a81afd01aa31c7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Apr 2015 04:49:06 -0400 Subject: [PATCH 3/9] strbuf_getwholeline: use getc_unlocked strbuf_getwholeline calls getc in a tight loop. On modern libc implementations, the stdio code locks the handle for every operation, which means we are paying a significant overhead. We can get around this by locking the handle for the whole loop and using the unlocked variant. Running "git rev-parse refs/heads/does-not-exist" on a repo with an extremely large (1.6GB) packed-refs file went from: real 0m18.900s user 0m18.472s sys 0m0.448s to: real 0m10.953s user 0m10.384s sys 0m0.580s for a wall-clock speedup of 42%. All times are best-of-3, and done on a glibc 2.19 system. Note that we call into strbuf_grow while holding the lock. It's possible for that function to call other stdio functions (e.g., printing to stderr when dying due to malloc error); however, the POSIX.1-2001 definition of flockfile makes it clear that the locks are per-handle, so we are fine unless somebody else tries to read from our same handle. This doesn't ever happen in the current code, and is unlikely to be added in the future (we would have to do something exotic like add a die_routine that tried to read from stdin). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- strbuf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/strbuf.c b/strbuf.c index 14f337db1..af2bad4b5 100644 --- a/strbuf.c +++ b/strbuf.c @@ -443,12 +443,14 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) return EOF; strbuf_reset(sb); - while ((ch = getc(fp)) != EOF) { + flockfile(fp); + while ((ch = getc_unlocked(fp)) != EOF) { strbuf_grow(sb, 1); sb->buf[sb->len++] = ch; if (ch == term) break; } + funlockfile(fp); if (ch == EOF && sb->len == 0) return EOF; From 260d408e32363a9e76d0fea3056563d4fb51f29e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Apr 2015 04:51:18 -0400 Subject: [PATCH 4/9] config: use getc_unlocked when reading from file We read config files character-by-character from a stdio handle using fgetc(). This incurs significant locking overhead, even though we know that only one thread can possibly access the handle. We can speed this up by taking the lock ourselves, and then using getc_unlocked to read each character. On a silly pathological case: perl -le ' print "[core]"; print "key$_ = value$_" for (1..1000000) ' >input git config -f input core.key1 this dropped the time to run git-config from: real 0m0.263s user 0m0.260s sys 0m0.000s to: real 0m0.159s user 0m0.152s sys 0m0.004s for a savings of 39%. Most config files are not this big, but the savings should be proportional to the size of the file (i.e., we always save 39%, just of a much smaller number). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 66c0a51bc..8b297fc4e 100644 --- a/config.c +++ b/config.c @@ -49,7 +49,7 @@ static struct config_set the_config_set; static int config_file_fgetc(struct config_source *conf) { - return fgetc(conf->u.file); + return getc_unlocked(conf->u.file); } static int config_file_ungetc(int c, struct config_source *conf) @@ -1088,7 +1088,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data) f = fopen(filename, "r"); if (f) { + flockfile(f); ret = do_config_from_file(fn, filename, filename, f, data); + funlockfile(f); fclose(f); } return ret; From fec501dae8bf6c8fdcd4124c94f37c4cbc26ba29 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Apr 2015 04:53:56 -0400 Subject: [PATCH 5/9] strbuf_addch: avoid calling strbuf_grow We mark strbuf_addch as inline, because we expect it may be called from a tight loop. However, the first thing it does is call the non-inline strbuf_grow(), which can handle arbitrary-sized growth. Since we know that we only need a single character, we can use the inline strbuf_avail() to quickly check whether we need to grow at all. Our check is redundant when we do call strbuf_grow(), but that's OK. The common case is that we avoid calling it at all, and we have made that case faster. On a silly pathological case: perl -le ' print "[core]"; print "key$_ = value$_" for (1..1000000) ' >input git config -f input core.key1 this dropped the time to run git-config from: real 0m0.159s user 0m0.152s sys 0m0.004s to: real 0m0.140s user 0m0.136s sys 0m0.004s for a savings of 12%. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- strbuf.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/strbuf.h b/strbuf.h index 1883494ca..01c5c6371 100644 --- a/strbuf.h +++ b/strbuf.h @@ -205,7 +205,8 @@ extern int strbuf_cmp(const struct strbuf *, const struct strbuf *); */ static inline void strbuf_addch(struct strbuf *sb, int c) { - strbuf_grow(sb, 1); + if (!strbuf_avail(sb)) + strbuf_grow(sb, 1); sb->buf[sb->len++] = c; sb->buf[sb->len] = '\0'; } From f80c153bea4e0ea86f5b6d32e77df0b69501ee18 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Apr 2015 04:58:54 -0400 Subject: [PATCH 6/9] strbuf_getwholeline: avoid calling strbuf_grow As with the recent speedup to strbuf_addch, we can avoid calling strbuf_grow() in a tight loop of single-character adds by instead checking strbuf_avail. Note that we would instead call strbuf_addch directly here, but it does more work than necessary: it will NUL-terminate the result for each character read. Instead, in this loop we read the characters one by one and then add the terminator manually at the end. Running "git rev-parse refs/heads/does-not-exist" on a repo with an extremely large (1.6GB) packed-refs file went from (best-of-5): real 0m10.948s user 0m10.548s sys 0m0.412s to: real 0m8.601s user 0m8.084s sys 0m0.524s for a wall-clock speedup of 21%. Helped-by: Eric Sunshine Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- strbuf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/strbuf.c b/strbuf.c index af2bad4b5..921619ea6 100644 --- a/strbuf.c +++ b/strbuf.c @@ -445,7 +445,8 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) strbuf_reset(sb); flockfile(fp); while ((ch = getc_unlocked(fp)) != EOF) { - strbuf_grow(sb, 1); + if (!strbuf_avail(sb)) + strbuf_grow(sb, 1); sb->buf[sb->len++] = ch; if (ch == term) break; From 0cc30e0e842a25846e76e09f62a1d425a25ee556 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Apr 2015 05:01:38 -0400 Subject: [PATCH 7/9] strbuf_getwholeline: use getdelim if it is available We spend a lot of time in strbuf_getwholeline in a tight loop reading characters from a stdio handle into a buffer. The libc getdelim() function can do this for us with less overhead. It's in POSIX.1-2008, and was a GNU extension before that. Therefore we can't rely on it, but can fall back to the existing getc loop when it is not available. The HAVE_GETDELIM knob is turned on automatically for Linux, where we have glibc. We don't need to set any new feature-test macros, because we already define _GNU_SOURCE. Other systems that implement getdelim may need to other macros (probably _POSIX_C_SOURCE >= 200809L), but we can address that along with setting the Makefile knob after testing the feature on those systems. Running "git rev-parse refs/heads/does-not-exist" on a repo with an extremely large (1.6GB) packed-refs file went from (best-of-5): real 0m8.601s user 0m8.084s sys 0m0.524s to: real 0m6.768s user 0m6.340s sys 0m0.432s for a wall-clock speedup of 21%. Based on a patch from Rasmus Villemoes . Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Makefile | 6 ++++++ config.mak.uname | 1 + strbuf.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/Makefile b/Makefile index 5f3987fe3..36655d5a1 100644 --- a/Makefile +++ b/Makefile @@ -359,6 +359,8 @@ all:: # compiler is detected to support it. # # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function. +# +# Define HAVE_GETDELIM if your system has the getdelim() function. GIT-VERSION-FILE: FORCE @$(SHELL_PATH) ./GIT-VERSION-GEN @@ -1437,6 +1439,10 @@ ifdef HAVE_BSD_SYSCTL BASIC_CFLAGS += -DHAVE_BSD_SYSCTL endif +ifdef HAVE_GETDELIM + BASIC_CFLAGS += -DHAVE_GETDELIM +endif + ifeq ($(TCLTK_PATH),) NO_TCLTK = NoThanks endif diff --git a/config.mak.uname b/config.mak.uname index f4e77cb9e..d26665fa5 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -36,6 +36,7 @@ ifeq ($(uname_S),Linux) HAVE_DEV_TTY = YesPlease HAVE_CLOCK_GETTIME = YesPlease HAVE_CLOCK_MONOTONIC = YesPlease + HAVE_GETDELIM = YesPlease endif ifeq ($(uname_S),GNU/kFreeBSD) HAVE_ALLOCA_H = YesPlease diff --git a/strbuf.c b/strbuf.c index 921619ea6..0d4f4e54e 100644 --- a/strbuf.c +++ b/strbuf.c @@ -435,6 +435,47 @@ int strbuf_getcwd(struct strbuf *sb) return -1; } +#ifdef HAVE_GETDELIM +int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) +{ + ssize_t r; + + if (feof(fp)) + return EOF; + + strbuf_reset(sb); + + /* Translate slopbuf to NULL, as we cannot call realloc on it */ + if (!sb->alloc) + sb->buf = NULL; + r = getdelim(&sb->buf, &sb->alloc, term, fp); + + if (r > 0) { + sb->len = r; + return 0; + } + assert(r == -1); + + /* + * Normally we would have called xrealloc, which will try to free + * memory and recover. But we have no way to tell getdelim() to do so. + * Worse, we cannot try to recover ENOMEM ourselves, because we have + * no idea how many bytes were read by getdelim. + * + * Dying here is reasonable. It mirrors what xrealloc would do on + * catastrophic memory failure. We skip the opportunity to free pack + * memory and retry, but that's unlikely to help for a malloc small + * enough to hold a single line of input, anyway. + */ + if (errno == ENOMEM) + die("Out of memory, getdelim failed"); + + /* Restore slopbuf that we moved out of the way before */ + if (!sb->buf) + strbuf_init(sb, 0); + return EOF; +} +#else int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) { int ch; @@ -458,6 +499,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) sb->buf[sb->len] = '\0'; return 0; } +#endif int strbuf_getline(struct strbuf *sb, FILE *fp, int term) { From 03afcbee9b9246f5ec9b013f2a699b151f0ba7ab Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Apr 2015 05:03:26 -0400 Subject: [PATCH 8/9] read_packed_refs: avoid double-checking sane refs Prior to d0f810f (refs.c: allow listing and deleting badly named refs, 2014-09-03), read_packed_refs would barf on any malformed refnames by virtue of calling create_ref_entry with the "check" parameter set to 1. That commit loosened our reading so that we call check_refname_format ourselves and just set a REF_BAD_NAME flag. We then call create_ref_entry with the check parameter set to 0. That function learned to do an extra safety check even when the check parameter is 0, so that we don't load any dangerous refnames (like "../../../etc/passwd"). This is implemented by calling refname_is_safe() in create_ref_entry(). However, we can observe that refname_is_safe() can only be true if check_refname_format() also failed. So in the common case of a sanely named ref, we perform _both_ checks, even though we know that the latter will never trigger. This has a noticeable performance impact when the packed-refs file is large. Let's drop the refname_is_safe check from create_ref_entry(), and make it the responsibility of the caller. Of the three callers that pass a check parameter of "0", two will have just called check_refname_format(), and can check the refname-safety only when it fails. The third case, pack_if_possible_fn, is copying from an existing ref entry, which must have previously passed our safety check. With this patch, running "git rev-parse refs/heads/does-not-exist" on a repo with a large (1.6GB) packed-refs file went from: real 0m6.768s user 0m6.340s sys 0m0.432s to: real 0m5.703s user 0m5.276s sys 0m0.432s for a wall-clock speedup of 15%. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- refs.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index 47e4e5380..f36ea75c8 100644 --- a/refs.c +++ b/refs.c @@ -344,8 +344,6 @@ static struct ref_entry *create_ref_entry(const char *refname, if (check_name && check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) die("Reference has invalid format: '%s'", refname); - if (!check_name && !refname_is_safe(refname)) - die("Reference has invalid name: '%s'", refname); len = strlen(refname) + 1; ref = xmalloc(sizeof(struct ref_entry) + len); hashcpy(ref->u.value.sha1, sha1); @@ -1178,6 +1176,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) int flag = REF_ISPACKED; if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { + if (!refname_is_safe(refname)) + die("packed refname is dangerous: %s", refname); hashclr(sha1); flag |= REF_BAD_NAME | REF_ISBROKEN; } @@ -1323,6 +1323,8 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir) } if (check_refname_format(refname.buf, REFNAME_ALLOW_ONELEVEL)) { + if (!refname_is_safe(refname.buf)) + die("loose refname is dangerous: %s", refname.buf); hashclr(sha1); flag |= REF_BAD_NAME | REF_ISBROKEN; } From a33729267504c7b1a63a1f05b47faea6f9e1642c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Apr 2015 05:04:44 -0400 Subject: [PATCH 9/9] t1430: add another refs-escape test In t1430, we check whether deleting the branch "../../foo" will delete ".git/foo". However, this is not that interesting a test; the precious file ".git/foo" does not look like a ref, so even if we did not notice the "escape" from the "refs/" hierarchy, we would fail for that reason (i.e., if you turned refname_is_safe into a noop, the test still passes). Let's add an additional test for the same thing, but with a file that actually looks like a ref. That will make sure we are exercising the refname_is_safe code. While we're at it, let's also make the code work a little harder by adding some extra paths and some empty path components. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1430-bad-ref-name.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh index 468e85621..16d0b8bd1 100755 --- a/t/t1430-bad-ref-name.sh +++ b/t/t1430-bad-ref-name.sh @@ -68,6 +68,14 @@ test_expect_success 'branch -D cannot delete non-ref in .git dir' ' test_cmp expect .git/my-private-file ' +test_expect_success 'branch -D cannot delete ref in .git dir' ' + git rev-parse HEAD >.git/my-private-file && + git rev-parse HEAD >expect && + git branch foo/legit && + test_must_fail git branch -D foo////./././../../../my-private-file && + test_cmp expect .git/my-private-file +' + test_expect_success 'branch -D cannot delete absolute path' ' git branch -f extra && test_must_fail git branch -D "$(pwd)/.git/refs/heads/extra" &&