* Re: [PATCH] Define XDL_FAST_HASH when building *for* (not *on*) x86_64
From: Jeff King @ 2016-12-01 3:56 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Junio C Hamano, git, Thomas Rast
In-Reply-To: <alpine.DEB.2.10.1611302202100.20145@buzzword-bingo.mit.edu>
On Wed, Nov 30, 2016 at 10:04:07PM -0500, Anders Kaseorg wrote:
> Previously, XDL_FAST_HASH was defined when ‘uname -m’ returns x86_64,
> even if we are cross-compiling for a different architecture. Check
> the __x86_64__ compiler macro instead.
>
> In addition to fixing the cross compilation bug, this is needed to
> pass the Debian build reproducibility test
> (https://tests.reproducible-builds.org/debian/index_variations.html).
I don't think this is a good approach to fix it. Right now XDL_FAST_HASH
is a Makefile knob that can be turned by the user, and can be used
either to explicitly enable or explicitly disable the feature.
With your patch, building with "make XDL_FAST_HASH=Yes" will still
explicitly enable it, but "make XDL_FAST_HASH=" would no longer disable
it (because even if unset, the compiler would turn it on when it sees
__x86_64__).
And being able to turn it off is important; more on that in a second.
So I think if we wanted to auto-detect based on __x86_64__, we'd
probably need to be able to set it to "auto" or something, and then
#if defined(XDL_FAST_HASH_AUTO) && __x86_64__
#define XDL_FAST_HASH
#endif
or something.
However, I think this might be the tip of the iceberg. There are lots of
Makefile knobs whose defaults are tweaked based on uname output. This
one caught you because you are cross-compiling across architectures, but
in theory you could cross-compile for FreeBSD from Linux, or whatever.
So I suspect a better strategy in general is to just override the
uname_* variables when cross-compiling.
All that being said, I actually think an easier fix for this particular
case might be to drop XDL_FAST_HASH entirely. It computes the hashes
slightly faster, but its collision characteristics are much worse. About
2 years ago I ran across a pathological diff that ran over 100x slower
with XDL_FAST_HASH:
http://public-inbox.org/git/20141222041944.GA441@peff.net/
The discussion veered into whether we should have a randomized hash
secured against DoS attacks. I played around with some alternatives, but
never found anything quite as fast for the "normal" case. And having
disabled XDL_FAST_HASH on GitHub's servers, it wasn't a big priority for
me.
I'd be happy if somebody wanted to investigate other hash functions
further. But barring that, I think we should drop XDL_FAST_HASH (or at
the very least stop turning it on by default) in the meantime. It's just
not a good tradeoff.
-Peff
^ permalink raw reply
* Re: [PATCH] Define XDL_FAST_HASH when building *for* (not *on*) x86_64
From: Jeff King @ 2016-12-01 3:59 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Junio C Hamano, git, Thomas Rast
In-Reply-To: <alpine.DEB.2.10.1611302202100.20145@buzzword-bingo.mit.edu>
[resend; the original had an outdated address for Thomas, and I would
definitely like his blessing before removing XDL_FAST_HASH].
On Wed, Nov 30, 2016 at 10:04:07PM -0500, Anders Kaseorg wrote:
> Previously, XDL_FAST_HASH was defined when ‘uname -m’ returns x86_64,
> even if we are cross-compiling for a different architecture. Check
> the __x86_64__ compiler macro instead.
>
> In addition to fixing the cross compilation bug, this is needed to
> pass the Debian build reproducibility test
> (https://tests.reproducible-builds.org/debian/index_variations.html).
I don't think this is a good approach to fix it. Right now XDL_FAST_HASH
is a Makefile knob that can be turned by the user, and can be used
either to explicitly enable or explicitly disable the feature.
With your patch, building with "make XDL_FAST_HASH=Yes" will still
explicitly enable it, but "make XDL_FAST_HASH=" would no longer disable
it (because even if unset, the compiler would turn it on when it sees
__x86_64__).
And being able to turn it off is important; more on that in a second.
So I think if we wanted to auto-detect based on __x86_64__, we'd
probably need to be able to set it to "auto" or something, and then
#if defined(XDL_FAST_HASH_AUTO) && __x86_64__
#define XDL_FAST_HASH
#endif
or something.
However, I think this might be the tip of the iceberg. There are lots of
Makefile knobs whose defaults are tweaked based on uname output. This
one caught you because you are cross-compiling across architectures, but
in theory you could cross-compile for FreeBSD from Linux, or whatever.
So I suspect a better strategy in general is to just override the
uname_* variables when cross-compiling.
All that being said, I actually think an easier fix for this particular
case might be to drop XDL_FAST_HASH entirely. It computes the hashes
slightly faster, but its collision characteristics are much worse. About
2 years ago I ran across a pathological diff that ran over 100x slower
with XDL_FAST_HASH:
http://public-inbox.org/git/20141222041944.GA441@peff.net/
The discussion veered into whether we should have a randomized hash
secured against DoS attacks. I played around with some alternatives, but
never found anything quite as fast for the "normal" case. And having
disabled XDL_FAST_HASH on GitHub's servers, it wasn't a big priority for
me.
I'd be happy if somebody wanted to investigate other hash functions
further. But barring that, I think we should drop XDL_FAST_HASH (or at
the very least stop turning it on by default) in the meantime. It's just
not a good tradeoff.
-Peff
^ permalink raw reply
* Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Jeff King @ 2016-12-01 4:02 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, Johannes Schindelin, GIT Mailing-list
In-Reply-To: <59da5383-16a0-b327-75a8-b4c4ad7bd479@ramsayjones.plus.com>
On Thu, Dec 01, 2016 at 01:18:35AM +0000, Ramsay Jones wrote:
> >> I forgot, we ended up reversing course later and silencing them:
> >>
> >> http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/
> >>
> >> By the rationale of that conversation, we should be doing:
> >>
> >> warning("%s", "");
> >>
> >> here.
> >
> > I forgot too. Thanks for digging up that thread.
>
> Yes, I blamed wt-status.c:227 and came up with commit 7d7d68022
> as well.
>
> So, by the same rationale, we should remove -Wno-format-zero-length
> from DEVELOPER_CFLAGS. yes?
I don't have a preference on which direction we go, but yes, right now
we are in an awkward middle ground. We should do one of:
1. Drop -Wno-format-zero-length from DEVELOPER_CFLAGS and make sure
future patches to do not violate it.
2. Declare warning("") as OK.
I still think the warning is silly, but (1) has value in that it
produces the least surprise and annoyance to various people building
Git.
-Peff
^ permalink raw reply
* Re: [PATCH v6 0/6] recursively grep across submodules
From: Jeff King @ 2016-12-01 4:22 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-1-git-send-email-bmwill@google.com>
On Wed, Nov 30, 2016 at 05:28:28PM -0800, Brandon Williams wrote:
> v6 fixes a race condition which existed in the 'is_submodule_populated'
> function. Instead of calling 'resolve_gitdir' to check for the existance of a
> .git file/directory, use 'stat'. 'resolve_gitdir' calls 'chdir' which can
> affect other running threads trying to load thier files into a buffer in
> memory.
This one passes my stress-test for t7814 (though I imagine you already
knew that).
I tried to think of things that could go wrong by using a simple stat()
instead of resolve_gitdir(). They should only differ when ".git" for
some reason does not point to a git repository. My initial thought is
that this might be more vocal about errors, because the child process
will complain. But actually, the original would already die if the
".git" file is funny, so we were pretty vocal already.
I also wondered whether the sub-process might skip a bogus ".git" file
and keep looking upward in the filesystem tree (which would confusingly
end up back in the super-project!). But it looks like we bail hard when
we see a ".git" file but it's bogus. Which is probably a good thing in
general for submodules.
I'm not sure any of that is actually even worth worrying about, as such
a setup is broken by definition. I just wanted to think it through as a
devil's advocate, and even that seems pretty reasonable.
-Peff
^ permalink raw reply
* [PATCH] xdiff: Do not enable XDL_FAST_HASH by default
From: Anders Kaseorg @ 2016-12-01 4:26 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Thomas Rast
Although XDL_FAST_HASH computes hashes slightly faster on some
architectures, its collision characteristics are much worse, resulting
in some pathological diffs running over 100x slower
(http://public-inbox.org/git/20141222041944.GA441@peff.net/).
Furthermore, it was being enabled when ‘uname -m’ returns x86_64, even
if we are cross-compiling for a different architecture. This mistake
was also causing the Debian build reproducibility test to fail
(https://tests.reproducible-builds.org/debian/index_variations.html).
Future architecture-specific definitions should be based on compiler
macros such as __x86_64__ rather than uname.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
On Wed, 30 Nov 2016, Jeff King wrote:
> However, I think this might be the tip of the iceberg. There are lots of
> Makefile knobs whose defaults are tweaked based on uname output. This
> one caught you because you are cross-compiling across architectures, but
> in theory you could cross-compile for FreeBSD from Linux, or whatever.
>
> So I suspect a better strategy in general is to just override the
> uname_* variables when cross-compiling.
The specific case of an i386 userspace on an x86_64 kernel is important
independently of the general cross compilation problem (in fact, the words
“cross compilation” may not even really apply here). And I don’t think
one should have to manually tweak the build for this setup, especially
since the compiler already has the needed information.
> All that being said, I actually think an easier fix for this particular
> case might be to drop XDL_FAST_HASH entirely.
Works for me.
Anders
Makefile | 1 -
config.mak.uname | 5 -----
2 files changed, 6 deletions(-)
diff --git a/Makefile b/Makefile
index f53fcc90d..c237d4f91 100644
--- a/Makefile
+++ b/Makefile
@@ -341,7 +341,6 @@ all::
# Define XDL_FAST_HASH to use an alternative line-hashing method in
# the diff algorithm. It gives a nice speedup if your processor has
# fast unaligned word loads. Does NOT work on big-endian systems!
-# Enabled by default on x86_64.
#
# Define GIT_USER_AGENT if you want to change how git identifies itself during
# network interactions. The default is "git/$(GIT_VERSION)".
diff --git a/config.mak.uname b/config.mak.uname
index b232908f8..2831a68c3 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -1,10 +1,8 @@
# Platform specific Makefile tweaks based on uname detection
uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
-uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')
uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not')
uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
-uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')
ifdef MSVC
@@ -17,9 +15,6 @@ endif
# because maintaining the nesting to match is a pain. If
# we had "elif" things would have been much nicer...
-ifeq ($(uname_M),x86_64)
- XDL_FAST_HASH = YesPlease
-endif
ifeq ($(uname_S),OSF1)
# Need this for u_short definitions et al
BASIC_CFLAGS += -D_OSF_SOURCE
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules
From: Jeff King @ 2016-12-01 4:29 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-2-git-send-email-bmwill@google.com>
On Wed, Nov 30, 2016 at 05:28:29PM -0800, Brandon Williams wrote:
> +/*
> + * Determine if a submodule has been populated at a given 'path'
> + */
> +int is_submodule_populated(const char *path)
> +{
> + int ret = 0;
> + struct stat st;
> + char *gitdir = xstrfmt("%s/.git", path);
> +
> + if (!stat(gitdir, &st))
> + ret = 1;
> +
> + free(gitdir);
> + return ret;
> +}
I don't know if it's worth changing or not, but this could be a bit
shorter:
int is_submodule_populated(const char *path)
{
return !access(mkpath("%s/.git", path), F_OK);
}
There is a file_exists() helper, but it uses lstat(), which I think you
don't want (because you'd prefer to bail on a broken .git symlink). But
access(F_OK) does what you want, I think.
mkpath() is generally an unsafe function because it uses a static
buffer, but it's handy and safe for handing values to syscalls like
this.
I say "I don't know if it's worth it" because what you've written is
fine, and while more lines, it's fairly obvious and safe.
-Peff
^ permalink raw reply
* [PATCH] xdiff: Do not enable XDL_FAST_HASH by default
From: Anders Kaseorg @ 2016-12-01 4:30 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Thomas Rast
In-Reply-To: <20161201035914.kftxb4vqmzcqed5r@sigill.intra.peff.net>
Although XDL_FAST_HASH computes hashes slightly faster on some
architectures, its collision characteristics are much worse, resulting
in some pathological diffs running over 100x slower
(http://public-inbox.org/git/20141222041944.GA441@peff.net/).
Furthermore, it was being enabled when ‘uname -m’ returns x86_64, even
if we are cross-compiling for a different architecture. This mistake
was also causing the Debian build reproducibility test to fail
(https://tests.reproducible-builds.org/debian/index_variations.html).
Future architecture-specific definitions should be based on compiler
macros such as __x86_64__ rather than uname.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
[Oops, also resending for Thomas’s new email address. Sorry for the
spam.]
On Wed, 30 Nov 2016, Jeff King wrote:
> However, I think this might be the tip of the iceberg. There are lots of
> Makefile knobs whose defaults are tweaked based on uname output. This
> one caught you because you are cross-compiling across architectures, but
> in theory you could cross-compile for FreeBSD from Linux, or whatever.
>
> So I suspect a better strategy in general is to just override the
> uname_* variables when cross-compiling.
The specific case of an i386 userspace on an x86_64 kernel is important
independently of the general cross compilation problem (in fact, the words
“cross compilation” may not even really apply here). And I don’t think
one should have to manually tweak the build for this setup, especially
since the compiler already has the needed information.
> All that being said, I actually think an easier fix for this particular
> case might be to drop XDL_FAST_HASH entirely.
Works for me.
Anders
Makefile | 1 -
config.mak.uname | 5 -----
2 files changed, 6 deletions(-)
diff --git a/Makefile b/Makefile
index f53fcc90d..c237d4f91 100644
--- a/Makefile
+++ b/Makefile
@@ -341,7 +341,6 @@ all::
# Define XDL_FAST_HASH to use an alternative line-hashing method in
# the diff algorithm. It gives a nice speedup if your processor has
# fast unaligned word loads. Does NOT work on big-endian systems!
-# Enabled by default on x86_64.
#
# Define GIT_USER_AGENT if you want to change how git identifies itself during
# network interactions. The default is "git/$(GIT_VERSION)".
diff --git a/config.mak.uname b/config.mak.uname
index b232908f8..2831a68c3 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -1,10 +1,8 @@
# Platform specific Makefile tweaks based on uname detection
uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
-uname_M := $(shell sh -c 'uname -m 2>/dev/null || echo not')
uname_O := $(shell sh -c 'uname -o 2>/dev/null || echo not')
uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')
-uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')
ifdef MSVC
@@ -17,9 +15,6 @@ endif
# because maintaining the nesting to match is a pain. If
# we had "elif" things would have been much nicer...
-ifeq ($(uname_M),x86_64)
- XDL_FAST_HASH = YesPlease
-endif
ifeq ($(uname_S),OSF1)
# Need this for u_short definitions et al
BASIC_CFLAGS += -D_OSF_SOURCE
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] xdiff: Do not enable XDL_FAST_HASH by default
From: Jeff King @ 2016-12-01 4:52 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Junio C Hamano, git, Thomas Rast
In-Reply-To: <alpine.DEB.2.10.1611302310240.20145@buzzword-bingo.mit.edu>
On Wed, Nov 30, 2016 at 11:26:43PM -0500, Anders Kaseorg wrote:
> > So I suspect a better strategy in general is to just override the
> > uname_* variables when cross-compiling.
>
> The specific case of an i386 userspace on an x86_64 kernel is important
> independently of the general cross compilation problem (in fact, the words
> “cross compilation” may not even really apply here). And I don’t think
> one should have to manually tweak the build for this setup, especially
> since the compiler already has the needed information.
Ah, I mistook that you were really cross-compiling x86-64 from i386, in
which case you'd generally have to set CC, etc for the cross-compile
chain. I agree this is a much more subtle case, and it's nice for it to
work out of the box.
> diff --git a/Makefile b/Makefile
> index f53fcc90d..c237d4f91 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -341,7 +341,6 @@ all::
> # Define XDL_FAST_HASH to use an alternative line-hashing method in
> # the diff algorithm. It gives a nice speedup if your processor has
> # fast unaligned word loads. Does NOT work on big-endian systems!
> -# Enabled by default on x86_64.
This is a nice incremental step in the sense that people can still
enable it if they want to in order to time it, play with it, etc. But
given what we know, I wonder if the help text here should warn people.
Or I guess we could move straight to dropping it entirely.
Here's what that patch might look like (I retimed it just be sure, and
was sad to see that it really _is_ making some cases faster. But I still
think slower-but-predictable is a better default).
I didn't drop uname_M here. If we go this route, I think it would make
sense to do that in a separate patch on top, with your commit message
explaining why it is a bad idea versus using compiler-defined macros.
-- >8 --
Subject: [PATCH] xdiff: drop XDL_FAST_HASH
The xdiff code hashes every line of both sides of a diff,
and then compares those hashes to find duplicates. The
overall performance depends both on how fast we can compute
the hashes, but also on how many hash collisions we see.
The idea of XDL_FAST_HASH is to speed up the hash
computation. But the generated hashes have worse collision
behavior. This means that in some cases it speeds diffs up
(running "git log -p" on git.git improves by ~8% with it),
but in others it can slow things down. One pathological case
saw over a 100x slowdown[1].
There may be a better hash function that covers both
properties, but in the meantime we are better off with the
original hash. It's slightly slower in the common case, but
it has fewer surprising pathological cases.
[1] http://public-inbox.org/git/20141222041944.GA441@peff.net/
Signed-off-by: Jeff King <peff@peff.net>
---
Makefile | 9 -----
config.mak.uname | 3 --
xdiff/xutils.c | 106 -------------------------------------------------------
3 files changed, 118 deletions(-)
diff --git a/Makefile b/Makefile
index f53fcc90d..f61076997 100644
--- a/Makefile
+++ b/Makefile
@@ -338,11 +338,6 @@ all::
#
# Define NATIVE_CRLF if your platform uses CRLF for line endings.
#
-# Define XDL_FAST_HASH to use an alternative line-hashing method in
-# the diff algorithm. It gives a nice speedup if your processor has
-# fast unaligned word loads. Does NOT work on big-endian systems!
-# Enabled by default on x86_64.
-#
# Define GIT_USER_AGENT if you want to change how git identifies itself during
# network interactions. The default is "git/$(GIT_VERSION)".
#
@@ -1485,10 +1480,6 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
MSGFMT += --check --statistics
endif
-ifneq (,$(XDL_FAST_HASH))
- BASIC_CFLAGS += -DXDL_FAST_HASH
-endif
-
ifdef GMTIME_UNRELIABLE_ERRORS
COMPAT_OBJS += compat/gmtime.o
BASIC_CFLAGS += -DGMTIME_UNRELIABLE_ERRORS
diff --git a/config.mak.uname b/config.mak.uname
index b232908f8..447f36ac2 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -17,9 +17,6 @@ endif
# because maintaining the nesting to match is a pain. If
# we had "elif" things would have been much nicer...
-ifeq ($(uname_M),x86_64)
- XDL_FAST_HASH = YesPlease
-endif
ifeq ($(uname_S),OSF1)
# Need this for u_short definitions et al
BASIC_CFLAGS += -D_OSF_SOURCE
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 027192a1c..04d7b32e4 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -264,110 +264,6 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
return ha;
}
-#ifdef XDL_FAST_HASH
-
-#define REPEAT_BYTE(x) ((~0ul / 0xff) * (x))
-
-#define ONEBYTES REPEAT_BYTE(0x01)
-#define NEWLINEBYTES REPEAT_BYTE(0x0a)
-#define HIGHBITS REPEAT_BYTE(0x80)
-
-/* Return the high bit set in the first byte that is a zero */
-static inline unsigned long has_zero(unsigned long a)
-{
- return ((a - ONEBYTES) & ~a) & HIGHBITS;
-}
-
-static inline long count_masked_bytes(unsigned long mask)
-{
- if (sizeof(long) == 8) {
- /*
- * Jan Achrenius on G+: microoptimized version of
- * the simpler "(mask & ONEBYTES) * ONEBYTES >> 56"
- * that works for the bytemasks without having to
- * mask them first.
- */
- /*
- * return mask * 0x0001020304050608 >> 56;
- *
- * Doing it like this avoids warnings on 32-bit machines.
- */
- long a = (REPEAT_BYTE(0x01) / 0xff + 1);
- return mask * a >> (sizeof(long) * 7);
- } else {
- /* Carl Chatfield / Jan Achrenius G+ version for 32-bit */
- /* (000000 0000ff 00ffff ffffff) -> ( 1 1 2 3 ) */
- long a = (0x0ff0001 + mask) >> 23;
- /* Fix the 1 for 00 case */
- return a & mask;
- }
-}
-
-unsigned long xdl_hash_record(char const **data, char const *top, long flags)
-{
- unsigned long hash = 5381;
- unsigned long a = 0, mask = 0;
- char const *ptr = *data;
- char const *end = top - sizeof(unsigned long) + 1;
-
- if (flags & XDF_WHITESPACE_FLAGS)
- return xdl_hash_record_with_whitespace(data, top, flags);
-
- ptr -= sizeof(unsigned long);
- do {
- hash += hash << 5;
- hash ^= a;
- ptr += sizeof(unsigned long);
- if (ptr >= end)
- break;
- a = *(unsigned long *)ptr;
- /* Do we have any '\n' bytes in this word? */
- mask = has_zero(a ^ NEWLINEBYTES);
- } while (!mask);
-
- if (ptr >= end) {
- /*
- * There is only a partial word left at the end of the
- * buffer. Because we may work with a memory mapping,
- * we have to grab the rest byte by byte instead of
- * blindly reading it.
- *
- * To avoid problems with masking in a signed value,
- * we use an unsigned char here.
- */
- const char *p;
- for (p = top - 1; p >= ptr; p--)
- a = (a << 8) + *((const unsigned char *)p);
- mask = has_zero(a ^ NEWLINEBYTES);
- if (!mask)
- /*
- * No '\n' found in the partial word. Make a
- * mask that matches what we read.
- */
- mask = 1UL << (8 * (top - ptr) + 7);
- }
-
- /* The mask *below* the first high bit set */
- mask = (mask - 1) & ~mask;
- mask >>= 7;
- hash += hash << 5;
- hash ^= a & mask;
-
- /* Advance past the last (possibly partial) word */
- ptr += count_masked_bytes(mask);
-
- if (ptr < top) {
- assert(*ptr == '\n');
- ptr++;
- }
-
- *data = ptr;
-
- return hash;
-}
-
-#else /* XDL_FAST_HASH */
-
unsigned long xdl_hash_record(char const **data, char const *top, long flags) {
unsigned long ha = 5381;
char const *ptr = *data;
@@ -384,8 +280,6 @@ unsigned long xdl_hash_record(char const **data, char const *top, long flags) {
return ha;
}
-#endif /* XDL_FAST_HASH */
-
unsigned int xdl_hashbits(unsigned int size) {
unsigned int val = 1, bits = 0;
--
2.11.0.319.gd1e73eb
^ permalink raw reply related
* Re: 'git repack' and repack.writeBitmaps=true with kept packs
From: Jeff King @ 2016-12-01 5:16 UTC (permalink / raw)
To: Steven Noonan; +Cc: git
In-Reply-To: <CAKbGBLjZ2WLVRM9f=by337xLhPgKCy10T8ra6Qz7OWA=QF-5yA@mail.gmail.com>
On Wed, Nov 30, 2016 at 04:15:33PM -0800, Steven Noonan wrote:
> It seems like it's behaving as though I've provided
> --pack-kept-objects. In order to ensure the .bitmap is created, it
> repacks everything, including everything in existing .pack files (not
> respecting .keep). But then it's not deleting the old .pack file
> (oddly, respecting .keep).
Right, that's exactly what's happening.
The bitmaps require a completely reachable set inside the pack, so if
you omit some objects that are in .keep packs, we cannot generate the
bitmap. So we have to either disable bitmaps, or pack the kept objects.
By default, we do the latter (and I'll explain why in a minute).
We can't delete the .keep packfiles because we don't know for sure that
we've included all of their contents in the new pack (not to mention
that somebody asked to keep them, and we don't know why; we should
respect that).
> What I'd expect it to do here is ignore the 'repack.writeBitmaps =
> true' value if there's a .keep that needs to be respected. Is this not
> a correct assumption?
In practice, I think that ends up worse. The .keep files are used by
receive-pack as lockfiles for incoming pushes. So imagine you kick off a
full repack just as somebody is pushing, and repack sees the temporary
.keep file. Your options are:
1. Disable bitmaps, leaving the repository with no bitmaps at all
until the next repack (because you're deleting the old bitmaps
along with the old, non-kept pack).
2. Duplicate the newly pushed objects in the pack (if they're even
reachable; you're also racing to see the ref updates). Now you have
bitmaps, but you're wasting a bit of space to store the racy push
twice (and it goes away next time you repack).
If you're running a Git server which depends on bitmaps for good
performance, then (2) is much better. And that's the default.
If you want to override it, you can pass --no-pack-kept-objects, or set
repack.packKeptObjects to false.
I think the documentation for --pack-kept-objects could be a bit more
clear for this case. It doesn't mention the default value, nor that what
you really want with "-b" is probably "--no-pack-kept-objects".
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)
From: Johannes Sixt @ 2016-12-01 7:09 UTC (permalink / raw)
To: Jeff King, Brandon Williams; +Cc: Junio C Hamano, git
In-Reply-To: <20161130234056.iltitkszvccbjivp@sigill.intra.peff.net>
Am 01.12.2016 um 00:40 schrieb Jeff King:
> 20813 access("su:b/../.git/modules/su:b/refs", X_OK) = 0
Side note: where does this weirdness "su:b" come from? (I haven't looked
at any of the patches, yet, let alone tested.) Colons in file or
directory names won't work on Windows (in the way one would expect).
-- Hannes
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2016, #06; Mon, 28)
From: Jeff King @ 2016-12-01 7:19 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Brandon Williams, Junio C Hamano, git
In-Reply-To: <fa54bb08-4206-ebd5-6808-f7de6cf4b9a2@kdbg.org>
On Thu, Dec 01, 2016 at 08:09:16AM +0100, Johannes Sixt wrote:
> Am 01.12.2016 um 00:40 schrieb Jeff King:
> > 20813 access("su:b/../.git/modules/su:b/refs", X_OK) = 0
>
> Side note: where does this weirdness "su:b" come from? (I haven't looked at
> any of the patches, yet, let alone tested.) Colons in file or directory
> names won't work on Windows (in the way one would expect).
It's explicitly used in the test, I assume to check that the recursive
grep is not confused into treating the name as a tree-ish.
I think it would have to either be marked with a prereq on Windows, or
modified to do the whole thing in-index (and use "grep --cached").
-Peff
^ permalink raw reply
* Re: [PATCH v6 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Johannes Sixt @ 2016-12-01 7:25 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, peff, sbeller, jonathantanmy, gitster
In-Reply-To: <1480555714-186183-6-git-send-email-bmwill@google.com>
Am 01.12.2016 um 02:28 schrieb Brandon Williams:
> + git init "su:b" &&
Don't do that. Colons in file names won't work on Windows.
-- Hannes
^ permalink raw reply
* bw/transport-protocol-policy
From: Jeff King @ 2016-12-01 8:30 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git
In-Reply-To: <xmqqk2bngn03.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 28, 2016 at 04:15:08PM -0800, Junio C Hamano wrote:
> * bw/transport-protocol-policy (2016-11-09) 2 commits
> (merged to 'next' on 2016-11-16 at 1391d3eeed)
> + transport: add protocol policy config option
> + lib-proto-disable: variable name fix
>
> Finer-grained control of what protocols are allowed for transports
> during clone/fetch/push have been enabled via a new configuration
> mechanism.
>
> Will cook in 'next'.
I was looking at the way the http code feeds protocol restrictions to
CURLOPT_REDIR_PROTOCOLS, and I think this topic is missing two elements:
1. The new policy config lets you say "only allow this protocol when
the user specifies it". But when http.c calls is_transport_allowed(),
the latter has no idea that we are asking it about potential
redirects (which obviously do _not_ come from the user), and would
erroneously allow them.
I think this needs fixed before the topic is merged. It's not a
regression, as it only comes into play if you use the new policy
config. But it is a minor security hole in the new feature.
2. If your curl is too old to support CURLOPT_REDIR_PROTOCOLS, we will
warn if there is a protocol whitelist in effect. But that check
only covers the environment whitelist, and we do not warn if you
restrict other protocols.
I actually think this should probably just warn indiscriminately.
Even without a Git protocol whitelist specified, the code serves to
prevent curl from redirecting to bizarre protocols like smtp. The
affected curl versions are from 2009 and prior, so I kind of doubt
it matters much either way (I'm actually tempted to suggest we bump
the minimum curl version there; there's a ton of #ifdef cruft going
back to 2002-era versions of libcurl).
-Peff
^ permalink raw reply
* [PATCH 3/6] remote-curl: rename shadowed options variable
From: Jeff King @ 2016-12-01 9:04 UTC (permalink / raw)
To: git; +Cc: Jann Horn
In-Reply-To: <20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net>
The discover_refs() function has a local "options" variable
to hold the http_get_options we pass to http_get_strbuf().
But this shadows the global "struct options" that holds our
program-level options, which cannot be accessed from this
function.
Let's give the local one a more descriptive name so we can
tell the two apart.
Signed-off-by: Jeff King <peff@peff.net>
---
remote-curl.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index f14c41f4c..f4145fd5c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -274,7 +274,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
struct strbuf effective_url = STRBUF_INIT;
struct discovery *last = last_discovery;
int http_ret, maybe_smart = 0;
- struct http_get_options options;
+ struct http_get_options http_options;
if (last && !strcmp(service, last->service))
return last;
@@ -291,15 +291,15 @@ static struct discovery *discover_refs(const char *service, int for_push)
strbuf_addf(&refs_url, "service=%s", service);
}
- memset(&options, 0, sizeof(options));
- options.content_type = &type;
- options.charset = &charset;
- options.effective_url = &effective_url;
- options.base_url = &url;
- options.no_cache = 1;
- options.keep_error = 1;
+ memset(&http_options, 0, sizeof(http_options));
+ http_options.content_type = &type;
+ http_options.charset = &charset;
+ http_options.effective_url = &effective_url;
+ http_options.base_url = &url;
+ http_options.no_cache = 1;
+ http_options.keep_error = 1;
- http_ret = http_get_strbuf(refs_url.buf, &buffer, &options);
+ http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
switch (http_ret) {
case HTTP_OK:
break;
--
2.11.0.319.g1f4e1e0
^ permalink raw reply related
* [PATCH 1/6] http: simplify update_url_from_redirect
From: Jeff King @ 2016-12-01 9:03 UTC (permalink / raw)
To: git; +Cc: Jann Horn
In-Reply-To: <20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net>
This function looks for a common tail between what we asked
for and where we were redirected to, but it open-codes the
comparison. We can avoid some confusing subtractions by
using strip_suffix_mem().
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/http.c b/http.c
index 4c4a812fc..840dbd1c7 100644
--- a/http.c
+++ b/http.c
@@ -1664,7 +1664,7 @@ static int update_url_from_redirect(struct strbuf *base,
const struct strbuf *got)
{
const char *tail;
- size_t tail_len;
+ size_t new_len;
if (!strcmp(asked, got->buf))
return 0;
@@ -1673,14 +1673,12 @@ static int update_url_from_redirect(struct strbuf *base,
die("BUG: update_url_from_redirect: %s is not a superset of %s",
asked, base->buf);
- tail_len = strlen(tail);
-
- if (got->len < tail_len ||
- strcmp(tail, got->buf + got->len - tail_len))
+ new_len = got->len;
+ if (!strip_suffix_mem(got->buf, &new_len, tail))
return 0; /* insane redirect scheme */
strbuf_reset(base);
- strbuf_add(base, got->buf, got->len - tail_len);
+ strbuf_add(base, got->buf, new_len);
return 1;
}
--
2.11.0.319.g1f4e1e0
^ permalink raw reply related
* [PATCH 2/6] http: always update the base URL for redirects
From: Jeff King @ 2016-12-01 9:04 UTC (permalink / raw)
To: git; +Cc: Jann Horn
In-Reply-To: <20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net>
If a malicious server redirects the initial ref
advertisement, it may be able to leak sha1s from other,
unrelated servers that the client has access to. For
example, imagine that Alice is a git user, she has access to
a private repository on a server hosted by Bob, and Mallory
runs a malicious server and wants to find out about Bob's
private repository.
Mallory asks Alice to clone an unrelated repository from her
over HTTP. When Alice's client contacts Mallory's server for
the initial ref advertisement, the server issues an HTTP
redirect for Bob's server. Alice contacts Bob's server and
gets the ref advertisement for the private repository. If
there is anything to fetch, she then follows up by asking
the server for one or more sha1 objects. But who is the
server?
If it is still Mallory's server, then Alice will leak the
existence of those sha1s to her.
Since commit c93c92f30 (http: update base URLs when we see
redirects, 2013-09-28), the client usually rewrites the base
URL such that all further requests will go to Bob's server.
But this is done by textually matching the URL. If we were
originally looking for "http://mallory/repo.git/info/refs",
and we got pointed at "http://bob/other.git/info/refs", then
we know that the right root is "http://bob/other.git".
If the redirect appears to change more than just the root,
we punt and continue to use the original server. E.g.,
imagine the redirect adds a URL component that Bob's server
will ignore, like "http://bob/other.git/info/refs?dummy=1".
We can solve this by aborting in this case rather than
silently continuing to use Mallory's server. In addition to
protecting from sha1 leakage, it's arguably safer and more
sane to refuse a confusing redirect like that in general.
For example, part of the motivation in c93c92f30 is
avoiding accidentally sending credentials over clear http,
just to get a response that says "try again over https". So
even in a non-malicious case, we'd prefer to err on the side
of caution.
The downside is that it's possible this will break a
legitimate but complicated server-side redirection scheme.
The setup given in the newly added test does work, but it's
convoluted enough that we don't need to care about it. A
more plausible case would be a server which redirects a
request for "info/refs?service=git-upload-pack" to just
"info/refs" (because it does not do smart HTTP, and for some
reason really dislikes query parameters). Right now we
would transparently downgrade to dumb-http, but with this
patch, we'd complain (and the user would have to set
GIT_SMART_HTTP=0 to fetch).
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 12 ++++++++----
t/lib-httpd/apache.conf | 8 ++++++++
t/t5551-http-fetch-smart.sh | 4 ++++
t/t5812-proto-disable-http.sh | 1 +
4 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/http.c b/http.c
index 840dbd1c7..ba03beead 100644
--- a/http.c
+++ b/http.c
@@ -1655,9 +1655,9 @@ static int http_request(const char *url,
*
* Note that this assumes a sane redirect scheme. It's entirely possible
* in the example above to end up at a URL that does not even end in
- * "info/refs". In such a case we simply punt, as there is not much we can
- * do (and such a scheme is unlikely to represent a real git repository,
- * which means we are likely about to abort anyway).
+ * "info/refs". In such a case we die. There's not much we can do, such a
+ * scheme is unlikely to represent a real git repository, and failing to
+ * rewrite the base opens options for malicious redirects to do funny things.
*/
static int update_url_from_redirect(struct strbuf *base,
const char *asked,
@@ -1675,10 +1675,14 @@ static int update_url_from_redirect(struct strbuf *base,
new_len = got->len;
if (!strip_suffix_mem(got->buf, &new_len, tail))
- return 0; /* insane redirect scheme */
+ die(_("unable to update url base from redirection:\n"
+ " asked for: %s\n"
+ " redirect: %s"),
+ asked, got->buf);
strbuf_reset(base);
strbuf_add(base, got->buf, new_len);
+
return 1;
}
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index c3e631394..f98b23a3c 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -132,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
+# The first rule issues a client-side redirect to something
+# that _doesn't_ look like a git repo. The second rule is a
+# server-side rewrite, so that it turns out the odd-looking
+# thing _is_ a git repo. The "[PT]" tells Apache to match
+# the usual ScriptAlias rules for /smart.
+RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]
+RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]
+
# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
# And as RewriteCond does not allow testing for non-matches, we match
# the desired case first (one has abra, two has cadabra), and let it
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 1ec5b2747..6e5b9e42f 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -119,6 +119,10 @@ test_expect_success 'redirects re-root further requests' '
git clone $HTTPD_URL/smart-redir-limited/repo.git repo-redir-limited
'
+test_expect_success 're-rooting dies on insane schemes' '
+ test_must_fail git clone $HTTPD_URL/insane-redir/repo.git insane
+'
+
test_expect_success 'clone from password-protected repository' '
echo two >expect &&
set_askpass user@host pass@host &&
diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index 0d105d541..044cc152f 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -18,6 +18,7 @@ test_proto "smart http" http "$HTTPD_URL/smart/repo.git"
test_expect_success 'curl redirects respect whitelist' '
test_must_fail env GIT_ALLOW_PROTOCOL=http:https \
+ GIT_SMART_HTTP=0 \
git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr &&
{
test_i18ngrep "ftp.*disabled" stderr ||
--
2.11.0.319.g1f4e1e0
^ permalink raw reply related
* [PATCH 0/6] restricting http redirects
From: Jeff King @ 2016-12-01 9:03 UTC (permalink / raw)
To: git; +Cc: Jann Horn
Jann Horn brought up on the git-security list some interesting
social-engineering attacks around the way Git handles HTTP redirects.
These patches are my attempt to harden our redirect handling against
these attacks.
Out of the box, they should make it more obvious to the user when we are
redirecting, and avoid intermingling objects between multiple dumb-http
repositories. There's also a config flag (not on by default) to disable
redirects entirely if you're operating in a more paranoid environment.
The individual commits have more details on the attack scenarios.
I gave some thought to how this might interact with the
bw/transport-protocol-policy topic, which lets you distinguish between
"from the user" and "from some other system" when allowing protocols. I
think that topic is missing some bits when it comes to HTTP, which I
outlined elsewhere:
http://public-inbox.org/git/20161201083005.dui572o4jxsqacas@sigill.intra.peff.net/
I also wondered if the new http.followRedirects option in this series
could be replaced by just setting protocol.allow to "user". But it's
not quite the same:
1. That only covers setting http.followRedirects to "false". There is
a special value "initial", which allows redirects on the initial
ref advertisement (see patch 4 for details).
2. The http.* options can be applied on a per-server basis. So you
might allow a trusted server to redirect you, but not others. The
protocol config is less flexible in that regard (it's less about
"who are you contacting" and more about "what situation are you
in").
So I think it's fine for the two to co-exist. There's some small
overlap, but which is appropriate depends on what problem you're trying
to solve.
Thanks Jann for the initial report and for good discussion on the
security list.
[1/6]: http: simplify update_url_from_redirect
[2/6]: http: always update the base URL for redirects
[3/6]: remote-curl: rename shadowed options variable
[4/6]: http: make redirects more obvious
[5/6]: http: treat http-alternates like redirects
[6/6]: http-walker: complain about non-404 loose object errors
Documentation/config.txt | 10 +++++++
http-walker.c | 15 +++++++----
http.c | 56 ++++++++++++++++++++++++++++++---------
http.h | 10 ++++++-
remote-curl.c | 22 +++++++++-------
t/lib-httpd/apache.conf | 14 ++++++++++
t/t5550-http-fetch-dumb.sh | 61 +++++++++++++++++++++++++++++++++++++++++++
t/t5551-http-fetch-smart.sh | 4 +++
t/t5812-proto-disable-http.sh | 1 +
9 files changed, 165 insertions(+), 28 deletions(-)
^ permalink raw reply
* [PATCH 6/6] http-walker: complain about non-404 loose object errors
From: Jeff King @ 2016-12-01 9:04 UTC (permalink / raw)
To: git; +Cc: Jann Horn
In-Reply-To: <20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net>
Since commit 17966c0a6 (http: avoid disconnecting on 404s
for loose objects, 2016-07-11), we turn off curl's
FAILONERROR option and instead manually deal with failing
HTTP codes.
However, the logic to do so only recognizes HTTP 404 as a
failure. This is probably the most common result, but if we
were to get another code, the curl result remains CURLE_OK,
and we treat it as success. We still end up detecting the
failure when we try to zlib-inflate the object (which will
fail), but instead of reporting the HTTP error, we just
claim that the object is corrupt.
Instead, let's catch anything in the 300's or above as an
error (300's are redirects which are not an error at the
HTTP level, but are an indication that we've explicitly
disabled redirects, so we should treat them as such; we
certainly don't have the resulting object content).
Note that we also fill in req->errorstr, which we didn't do
before. Without FAILONERROR, curl will not have filled this
in, and it will remain a blank string. This never mattered
for the 404 case, because in the logic below we hit the
"missing_target()" branch and print nothing. But for other
errors, we'd want to say _something_, if only to fill in the
blank slot in the error message.
Signed-off-by: Jeff King <peff@peff.net>
---
http-walker.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index 25a8b1ad4..c2f81cd6a 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -482,10 +482,13 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)
* we turned off CURLOPT_FAILONERROR to avoid losing a
* persistent connection and got CURLE_OK.
*/
- if (req->http_code == 404 && req->curl_result == CURLE_OK &&
+ if (req->http_code >= 300 && req->curl_result == CURLE_OK &&
(starts_with(req->url, "http://") ||
- starts_with(req->url, "https://")))
+ starts_with(req->url, "https://"))) {
req->curl_result = CURLE_HTTP_RETURNED_ERROR;
+ xsnprintf(req->errorstr, sizeof(req->errorstr),
+ "HTTP request failed");
+ }
if (obj_req->state == ABORTED) {
ret = error("Request for %s aborted", hex);
--
2.11.0.319.g1f4e1e0
^ permalink raw reply related
* [PATCH 4/6] http: make redirects more obvious
From: Jeff King @ 2016-12-01 9:04 UTC (permalink / raw)
To: git; +Cc: Jann Horn
In-Reply-To: <20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net>
We instruct curl to always follow HTTP redirects. This is
convenient, but it creates opportunities for malicious
servers to create confusing situations. For instance,
imagine Alice is a git user with access to a private
repository on Bob's server. Mallory runs her own server and
wants to access objects from Bob's repository.
Mallory may try a few tricks that involve asking Alice to
clone from her, build on top, and then push the result:
1. Mallory may simply redirect all fetch requests to Bob's
server. Git will transparently follow those redirects
and fetch Bob's history, which Alice may believe she
got from Mallory. The subsequent push seems like it is
just feeding Mallory back her own objects, but is
actually leaking Bob's objects. There is nothing in
git's output to indicate that Bob's repository was
involved at all.
The downside (for Mallory) of this attack is that Alice
will have received Bob's entire repository, and is
likely to notice that when building on top of it.
2. If Mallory happens to know the sha1 of some object X in
Bob's repository, she can instead build her own history
that references that object. She then runs a dumb http
server, and Alice's client will fetch each object
individually. When it asks for X, Mallory redirects her
to Bob's server. The end result is that Alice obtains
objects from Bob, but they may be buried deep in
history. Alice is less likely to notice.
Both of these attacks are fairly hard to pull off. There's a
social component in getting Mallory to convince Alice to
work with her. Alice may be prompted for credentials in
accessing Bob's repository (but not always, if she is using
a credential helper that caches). Attack (1) requires a
certain amount of obliviousness on Alice's part while making
a new commit. Attack (2) requires that Mallory knows a sha1
in Bob's repository, that Bob's server supports dumb http,
and that the object in question is loose on Bob's server.
But we can probably make things a bit more obvious without
any loss of functionality. This patch does two things to
that end.
First, when we encounter a whole-repo redirect during the
initial ref discovery, we now inform the user on stderr,
making attack (1) much more obvious.
Second, the decision to follow redirects is now
configurable. The truly paranoid can set the new
http.followRedirects to false to avoid any redirection
entirely. But for a more practical default, we will disallow
redirects only after the initial ref discovery. This is
enough to thwart attacks similar to (2), while still
allowing the common use of redirects at the repository
level. Since c93c92f30 (http: update base URLs when we see
redirects, 2013-09-28) we re-root all further requests from
the redirect destination, which should generally mean that
no further redirection is necessary.
As an escape hatch, in case there really is a server that
needs to redirect individual requests, the user can set
http.followRedirects to "true" (and this can be done on a
per-server basis via http.*.followRedirects config).
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 10 ++++++++++
http.c | 33 ++++++++++++++++++++++++++++++---
http.h | 10 +++++++++-
remote-curl.c | 4 ++++
t/lib-httpd/apache.conf | 6 ++++++
t/t5550-http-fetch-dumb.sh | 23 +++++++++++++++++++++++
6 files changed, 82 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a0ab66aae..d51182a06 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1891,6 +1891,16 @@ http.userAgent::
of common USER_AGENT strings (but not including those like git/1.7.1).
Can be overridden by the `GIT_HTTP_USER_AGENT` environment variable.
+http.followRedirects::
+ Whether git should follow HTTP redirects. If set to `true`, git
+ will transparently follow any redirect issued by a server it
+ encounters. If set to `false`, git will treat all redirects as
+ errors. If set to `initial`, git will follow redirects only for
+ the initial request to a remote, but not for subsequent
+ follow-up HTTP requests. Since git uses the redirected URL as
+ the base for the follow-up requests, this is generally
+ sufficient. The default is `initial`.
+
http.<url>.*::
Any of the http.* options above can be applied selectively to some URLs.
For a config key to match a URL, each element of the config key is
diff --git a/http.c b/http.c
index ba03beead..825118481 100644
--- a/http.c
+++ b/http.c
@@ -111,6 +111,8 @@ static int http_proactive_auth;
static const char *user_agent;
static int curl_empty_auth;
+enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
+
#if LIBCURL_VERSION_NUM >= 0x071700
/* Use CURLOPT_KEYPASSWD as is */
#elif LIBCURL_VERSION_NUM >= 0x070903
@@ -366,6 +368,16 @@ static int http_options(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp("http.followredirects", var)) {
+ if (value && !strcmp(value, "initial"))
+ http_follow_config = HTTP_FOLLOW_INITIAL;
+ else if (git_config_bool(var, value))
+ http_follow_config = HTTP_FOLLOW_ALWAYS;
+ else
+ http_follow_config = HTTP_FOLLOW_NONE;
+ return 0;
+ }
+
/* Fall back on the default ones */
return git_default_config(var, value, cb);
}
@@ -717,7 +729,6 @@ static CURL *get_curl_handle(void)
curl_low_speed_time);
}
- curl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
#if LIBCURL_VERSION_NUM >= 0x071301
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
@@ -1044,6 +1055,16 @@ struct active_request_slot *get_active_slot(void)
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
+ /*
+ * Default following to off unless "ALWAYS" is configured; this gives
+ * callers a sane starting point, and they can tweak for individual
+ * HTTP_FOLLOW_* cases themselves.
+ */
+ if (http_follow_config == HTTP_FOLLOW_ALWAYS)
+ curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
+ else
+ curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 0);
+
#if LIBCURL_VERSION_NUM >= 0x070a08
curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
#endif
@@ -1286,9 +1307,12 @@ static int handle_curl_result(struct slot_results *results)
* If we see a failing http code with CURLE_OK, we have turned off
* FAILONERROR (to keep the server's custom error response), and should
* translate the code into failure here.
+ *
+ * Likewise, if we see a redirect (30x code), that means we turned off
+ * redirect-following, and we should treat the result as an error.
*/
if (results->curl_result == CURLE_OK &&
- results->http_code >= 400) {
+ results->http_code >= 300) {
results->curl_result = CURLE_HTTP_RETURNED_ERROR;
/*
* Normally curl will already have put the "reason phrase"
@@ -1607,6 +1631,9 @@ static int http_request(const char *url,
strbuf_addstr(&buf, " no-cache");
if (options && options->keep_error)
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
+ if (options && options->initial_request &&
+ http_follow_config == HTTP_FOLLOW_INITIAL)
+ curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
headers = curl_slist_append(headers, buf.buf);
@@ -2030,7 +2057,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
if (c != CURLE_OK)
die("BUG: curl_easy_getinfo for HTTP code failed: %s",
curl_easy_strerror(c));
- if (slot->http_code >= 400)
+ if (slot->http_code >= 300)
return size;
}
diff --git a/http.h b/http.h
index 5ab9d9c32..02bccb7b0 100644
--- a/http.h
+++ b/http.h
@@ -116,6 +116,13 @@ extern struct credential http_auth;
extern char curl_errorstr[CURL_ERROR_SIZE];
+enum http_follow_config {
+ HTTP_FOLLOW_NONE,
+ HTTP_FOLLOW_ALWAYS,
+ HTTP_FOLLOW_INITIAL
+};
+extern enum http_follow_config http_follow_config;
+
static inline int missing__target(int code, int result)
{
return /* file:// URL -- do we ever use one??? */
@@ -139,7 +146,8 @@ extern char *get_remote_object_url(const char *url, const char *hex,
/* Options for http_get_*() */
struct http_get_options {
unsigned no_cache:1,
- keep_error:1;
+ keep_error:1,
+ initial_request:1;
/* If non-NULL, returns the content-type of the response. */
struct strbuf *content_type;
diff --git a/remote-curl.c b/remote-curl.c
index f4145fd5c..28d9d1063 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -296,6 +296,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
http_options.charset = &charset;
http_options.effective_url = &effective_url;
http_options.base_url = &url;
+ http_options.initial_request = 1;
http_options.no_cache = 1;
http_options.keep_error = 1;
@@ -314,6 +315,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
die("unable to access '%s': %s", url.buf, curl_errorstr);
}
+ if (options.verbosity && !starts_with(refs_url.buf, url.buf))
+ warning(_("redirecting to %s"), url.buf);
+
last= xcalloc(1, sizeof(*last_discovery));
last->service = service;
last->buf_alloc = strbuf_detach(&buffer, &last->len);
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index f98b23a3c..69174c6e3 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -123,6 +123,7 @@ ScriptAlias /error/ error.sh/
</Files>
RewriteEngine on
+RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]
RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]
RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]
RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]
@@ -140,6 +141,11 @@ RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]
RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]
+# Serve info/refs internally without redirecting, but
+# issue a redirect for any object requests.
+RewriteRule ^/redir-objects/(.*/info/refs)$ /dumb/$1 [PT]
+RewriteRule ^/redir-objects/(.*/objects/.*)$ /dumb/$1 [R=301]
+
# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
# And as RewriteCond does not allow testing for non-matches, we match
# the desired case first (one has abra, two has cadabra), and let it
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 7641417b4..532507b7c 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -307,5 +307,28 @@ test_expect_success 'remote-http complains cleanly about malformed urls' '
test_must_fail git remote-http http::/example.com/repo.git
'
+test_expect_success 'redirects can be forbidden/allowed' '
+ test_must_fail git -c http.followRedirects=false \
+ clone $HTTPD_URL/dumb-redir/repo.git dumb-redir &&
+ git -c http.followRedirects=true \
+ clone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr
+'
+
+test_expect_success 'redirects are reported to stderr' '
+ # just look for a snippet of the redirected-to URL
+ test_i18ngrep /dumb/ stderr
+'
+
+test_expect_success 'non-initial redirects can be forbidden' '
+ test_must_fail git -c http.followRedirects=initial \
+ clone $HTTPD_URL/redir-objects/repo.git redir-objects &&
+ git -c http.followRedirects=true \
+ clone $HTTPD_URL/redir-objects/repo.git redir-objects
+'
+
+test_expect_success 'http.followRedirects defaults to "initial"' '
+ test_must_fail git clone $HTTPD_URL/redir-objects/repo.git default
+'
+
stop_httpd
test_done
--
2.11.0.319.g1f4e1e0
^ permalink raw reply related
* [PATCH 5/6] http: treat http-alternates like redirects
From: Jeff King @ 2016-12-01 9:04 UTC (permalink / raw)
To: git; +Cc: Jann Horn
In-Reply-To: <20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net>
The previous commit made HTTP redirects more obvious and
tightened up the default behavior. However, there's another
way for a server to ask a git client to fetch arbitrary
content: by having an http-alternates file (or a regular
alternates file, which is used as a backup).
Similar to the HTTP redirect case, a malicious server can
claim to have refs pointing at object X, return a 404 when
the client asks for X, but point to some other URL via
http-alternates, which the client will transparently fetch.
The end result is that it looks from the user's perspective
like the objects came from the malicious server, as the
other URL is not mentioned at all.
Worse, because we feed the new URL to curl ourselves, the
usual protocol restrictions do not kick in (neither curl's
default of disallowing file://, nor the protocol
whitelisting in f4113cac0 (http: limit redirection to
protocol-whitelist, 2015-09-22).
Let's apply the same rules here as we do for HTTP redirects.
Namely:
- unless http.followRedirects is set to "always", we will
not follow remote redirects from http-alternates (or
alternates) at all
- set CURLOPT_PROTOCOLS alongside CURLOPT_REDIR_PROTOCOLS
restrict ourselves to a known-safe set and respect any
user-provided whitelist.
- mention alternate object stores on stderr so that the
user is aware another source of objects may be involved
The first item may prove to be too restrictive. The most
common use of alternates is to point to another path on the
same server. While it's possible for a single-server
redirect to be an attack, it takes a fairly obscure setup
(victim and evil repository on the same host, host speaks
dumb http, and evil repository has access to edit its own
http-alternates file).
So we could make the checks more specific, and only cover
cross-server redirects. But that means parsing the URLs
ourselves, rather than letting curl handle them. This patch
goes for the simpler approach. Given that they are only used
with dumb http, http-alternates are probably pretty rare.
And there's an escape hatch: the user can allow redirects on
a specific server by setting http.<url>.followRedirects to
"always".
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
http-walker.c | 8 +++++---
http.c | 1 +
t/t5550-http-fetch-dumb.sh | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index 0b2425531..25a8b1ad4 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -274,9 +274,8 @@ static void process_alternates_response(void *callback_data)
struct strbuf target = STRBUF_INIT;
strbuf_add(&target, base, serverlen);
strbuf_add(&target, data + i, posn - i - 7);
- if (walker->get_verbosely)
- fprintf(stderr, "Also look at %s\n",
- target.buf);
+ warning("adding alternate object store: %s",
+ target.buf);
newalt = xmalloc(sizeof(*newalt));
newalt->next = NULL;
newalt->base = strbuf_detach(&target, NULL);
@@ -302,6 +301,9 @@ static void fetch_alternates(struct walker *walker, const char *base)
struct alternates_request alt_req;
struct walker_data *cdata = walker->data;
+ if (http_follow_config != HTTP_FOLLOW_ALWAYS)
+ return;
+
/*
* If another request has already started fetching alternates,
* wait for them to arrive and return to processing this request's
diff --git a/http.c b/http.c
index 825118481..051fe6e5a 100644
--- a/http.c
+++ b/http.c
@@ -745,6 +745,7 @@ static CURL *get_curl_handle(void)
if (is_transport_allowed("ftps"))
allowed_protocols |= CURLPROTO_FTPS;
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
+ curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
#else
if (transport_restrict_protocols())
warning("protocol restrictions not applied to curl redirects because\n"
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 532507b7c..264a1ab8b 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -330,5 +330,43 @@ test_expect_success 'http.followRedirects defaults to "initial"' '
test_must_fail git clone $HTTPD_URL/redir-objects/repo.git default
'
+# The goal is for a clone of the "evil" repository, which has no objects
+# itself, to cause the client to fetch objects from the "victim" repository.
+test_expect_success 'set up evil alternates scheme' '
+ victim=$HTTPD_DOCUMENT_ROOT_PATH/victim.git &&
+ git init --bare "$victim" &&
+ git -C "$victim" --work-tree=. commit --allow-empty -m secret &&
+ git -C "$victim" repack -ad &&
+ git -C "$victim" update-server-info &&
+ sha1=$(git -C "$victim" rev-parse HEAD) &&
+
+ evil=$HTTPD_DOCUMENT_ROOT_PATH/evil.git &&
+ git init --bare "$evil" &&
+ # do this by hand to avoid object existence check
+ printf "%s\\t%s\\n" $sha1 refs/heads/master >"$evil/info/refs"
+'
+
+# Here we'll just redirect via HTTP. In a real-world attack these would be on
+# different servers, but we should reject it either way.
+test_expect_success 'http-alternates is a non-initial redirect' '
+ echo "$HTTPD_URL/dumb/victim.git/objects" \
+ >"$evil/objects/info/http-alternates" &&
+ test_must_fail git -c http.followRedirects=initial \
+ clone $HTTPD_URL/dumb/evil.git evil-initial &&
+ git -c http.followRedirects=true \
+ clone $HTTPD_URL/dumb/evil.git evil-initial
+'
+
+# Curl supports a lot of protocols that we'd prefer not to allow
+# http-alternates to use, but it's hard to test whether curl has
+# accessed, say, the SMTP protocol, because we are not running an SMTP server.
+# But we can check that it does not allow access to file://, which would
+# otherwise allow this clone to complete.
+test_expect_success 'http-alternates cannot point at funny protocols' '
+ echo "file://$victim/objects" >"$evil/objects/info/http-alternates" &&
+ test_must_fail git -c http.followRedirects=true \
+ clone "$HTTPD_URL/dumb/evil.git" evil-file
+'
+
stop_httpd
test_done
--
2.11.0.319.g1f4e1e0
^ permalink raw reply related
* Re: CVSImport - spaces in CVS path
From: David Aguilar @ 2016-12-01 9:59 UTC (permalink / raw)
To: Yojoa; +Cc: git
In-Reply-To: <1480539395581-7657459.post@n2.nabble.com>
On Wed, Nov 30, 2016 at 01:56:35PM -0700, Yojoa wrote:
> I'm in the process of moving an entire collection of cvs modules into git.
> I'm working in Mac Yosemite. Everything is working fine except for one
> thing. A couple of the CVS modules have spaces in the paths. Below is what
> my command line looks like. When the path has spaces I've tried putting it
> in single and double quotes and using escape characters. None of that
> matters. I always get a message that it can't read dir/dir/path and then
> messages that it can't find the modules "with" or "spaces".
>
> git cvsimport -v -d :pserver:MYLOGIN:/usr/local/cvsroot/
> dir/dir/PathWithNoSpaces/dir
>
> git cvsimport -v -d :pserver:MYLOGIN:/usr/local/cvsroot/ dir/dir/path with
> spaces/dir
Try cvs-fast-export + cvsconvert.
http://www.catb.org/~esr/cvs-fast-export/
http://www.catb.org/~esr/cvs-fast-export/cvsconvert.html
--
David
^ permalink raw reply
* [ANNOUNCE] Git for Windows 2.11.0
From: Johannes Schindelin @ 2016-12-01 12:31 UTC (permalink / raw)
To: git-for-windows, git; +Cc: Johannes Schindelin
MIME-Version: 1.0
Fcc: Sent
Dear Git users,
It is my pleasure to announce that Git for Windows 2.11.0 is available from:
https://git-for-windows.github.io/
Changes since Git for Windows v2.10.2 (November 2nd 2016)
New Features
* Comes with Git v2.11.0.
* Performance of git add in large worktrees was improved.
* A new, experimental, builtin version of the difftool is available
as an opt-in feature.
* Support has been added to generate project files for Visual Studio
2010 and later.
Bug Fixes
* The preload-index feature now behaves much better in conjunction
with sparse checkouts.
* When encountering a symbolic link, Git now always tries to read it,
not only when core.symlinks = true.
* The regression where Git would not interpret non-ASCII characters
passed from a CMD window correctly has been fixed.
* Performance of the cache of case-insensitive file names has been
improved.
* When building with MS Visual C, release builds are now properly
optimized.
* git cvsexportcommit now also works with CVSNT.
* Git's Perl no longer gets confused by externally-set PERL5LIB.
* The uninstaller no longer leaves an empty Git\mingw64 folder behind
.
* The installer now actually records whether the user chose to enable
or disable the Git Credential Manager.
* A certain scenario that could cause a crash in cherry-pick no
longer causes that.
Filename | SHA-256
-------- | -------
Git-2.11.0-64-bit.exe | fd1937ea8468461d35d9cabfcdd2daa3a74509dc9213c43c2b9615e8f0b85086
Git-2.11.0-32-bit.exe | 2a6083479538c4fe454336660fce96346ee3cf46f99ce08a666d4635539239d7
PortableGit-2.11.0-64-bit.7z.exe | a86cc4babfe204cc91408053b517827b4a93e6c659b85ab30910eda0e38bfc19
PortableGit-2.11.0-32-bit.7z.exe | fccec9350c1cb58a5e6d84d307d4f9f43ab9d58d93c8de67056416539d199002
MinGit-2.11.0-64-bit.zip | f31b0135e11e425555fb34779da3345ce8d32490fdd0a33b6f5ae8d74bae20b6
MinGit-2.11.0-32-bit.zip | 48e363cb6ce867a8004056a95da57f7c330ab79dbd26e4895c2aaeb4aec3b3a0
Git-2.11.0-64-bit.tar.bz2 | e62263ff734dbb071819dfcc33e9f443f9a445cde54d0102f6d164d5df5ece6c
Git-2.11.0-32-bit.tar.bz2 | 43dda49d4c7305d18b9f0612f66ba096ca2881a8ebf7b41cfb358d9a05fedef9
Ciao,
Johannes
^ permalink raw reply
* Re: [ANNOUNCE] Git for Windows 2.11.0
From: stefan.naewe @ 2016-12-01 14:12 UTC (permalink / raw)
To: johannes.schindelin, git-for-windows, git
In-Reply-To: <20161201123130.7608-1-johannes.schindelin@gmx.de>
Am 01.12.2016 um 13:31 schrieb Johannes Schindelin:
> MIME-Version: 1.0
>
> Fcc: Sent
>
> Dear Git users,
>
> It is my pleasure to announce that Git for Windows 2.11.0 is available from:
>
> https://git-for-windows.github.io/
>
> Changes since Git for Windows v2.10.2 (November 2nd 2016)
>
> New Features
>
> * Comes with Git v2.11.0.
> * Performance of git add in large worktrees was improved.
> * A new, experimental, builtin version of the difftool is available
> as an opt-in feature.
> * Support has been added to generate project files for Visual Studio
> 2010 and later.
That's not really a new feature of Git-for-Windows, is it ?
Just wondering...
Stefan
--
----------------------------------------------------------------
/dev/random says: Money is the root of all evil; everyone needs roots!
python -c "print '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9 9666 829B 49C5 9221 27AF
^ permalink raw reply
* Re: [PATCH 4/6] http: make redirects more obvious
From: Ramsay Jones @ 2016-12-01 16:06 UTC (permalink / raw)
To: Jeff King, git; +Cc: Jann Horn
In-Reply-To: <20161201090428.pweq7slwujbne5hg@sigill.intra.peff.net>
On 01/12/16 09:04, Jeff King wrote:
> We instruct curl to always follow HTTP redirects. This is
> convenient, but it creates opportunities for malicious
> servers to create confusing situations. For instance,
> imagine Alice is a git user with access to a private
> repository on Bob's server. Mallory runs her own server and
Ahem, so Mallory is female? (-blush-) :(
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 2/6] http: always update the base URL for redirects
From: Ramsay Jones @ 2016-12-01 16:02 UTC (permalink / raw)
To: Jeff King, git; +Cc: Jann Horn
In-Reply-To: <20161201090414.zgz7pimgpctghbwu@sigill.intra.peff.net>
On 01/12/16 09:04, Jeff King wrote:
> If a malicious server redirects the initial ref
> advertisement, it may be able to leak sha1s from other,
> unrelated servers that the client has access to. For
> example, imagine that Alice is a git user, she has access to
> a private repository on a server hosted by Bob, and Mallory
> runs a malicious server and wants to find out about Bob's
> private repository.
>
> Mallory asks Alice to clone an unrelated repository from her
-----------------------------------------------------------^^^
... from _him_ ? (ie Mallory)
> over HTTP. When Alice's client contacts Mallory's server for
> the initial ref advertisement, the server issues an HTTP
> redirect for Bob's server. Alice contacts Bob's server and
> gets the ref advertisement for the private repository. If
> there is anything to fetch, she then follows up by asking
> the server for one or more sha1 objects. But who is the
> server?
>
> If it is still Mallory's server, then Alice will leak the
> existence of those sha1s to her.
------------------------------^^^
... to _him_ ? (again Mallory)
ATB,
Ramsay Jones
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox