* 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
* [PATCH 0/3] string-list: make string_list_sort() reentrant
From: René Scharfe @ 2016-12-01 16:24 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Johannes Schindelin
Use qsort_s() from C11 Annex K to make string_list_sort() safer, in
particular when called from parallel threads.
compat: add qsort_s()
add QSORT_S
string-list: use QSORT_S in string_list_sort()
Makefile | 10 ++++++++
compat/qsort_s.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
git-compat-util.h | 11 +++++++++
string-list.c | 13 ++++-------
4 files changed, 95 insertions(+), 8 deletions(-)
create mode 100644 compat/qsort_s.c
--
2.11.0
^ permalink raw reply
* [PATCH 1/3] compat: add qsort_s()
From: René Scharfe @ 2016-12-01 16:26 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <3083fbf7-d67e-77e4-e05f-94a7e7e15eba@web.de>
The function qsort_s() was introduced with C11 Annex K; it provides the
ability to pass a context pointer to the comparison function, supports
the convention of using a NULL pointer for an empty array and performs a
few safety checks.
Add an implementation based on compat/qsort.c for platforms that lack a
native standards-compliant qsort_s() (i.e. basically everyone). It
doesn't perform the full range of possible checks: It uses size_t
instead of rsize_t and doesn't check nmemb and size against RSIZE_MAX
because we probably don't have the restricted size type defined. For
the same reason it returns int instead of errno_t.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Makefile | 10 ++++++++
compat/qsort_s.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
git-compat-util.h | 6 +++++
3 files changed, 85 insertions(+)
create mode 100644 compat/qsort_s.c
diff --git a/Makefile b/Makefile
index f53fcc90d..2245fd95d 100644
--- a/Makefile
+++ b/Makefile
@@ -279,6 +279,9 @@ all::
# is a simplified version of the merge sort used in glibc. This is
# recommended if Git triggers O(n^2) behavior in your platform's qsort().
#
+# Define HAVE_QSORT_S if your platform provides a qsort_s() that's
+# compatible with the one described in C11 Annex K.
+#
# Define UNRELIABLE_FSTAT if your system's fstat does not return the same
# information on a not yet closed file that lstat would return for the same
# file after it was closed.
@@ -1423,6 +1426,13 @@ ifdef INTERNAL_QSORT
COMPAT_CFLAGS += -DINTERNAL_QSORT
COMPAT_OBJS += compat/qsort.o
endif
+
+ifdef HAVE_QSORT_S
+ COMPAT_CFLAGS += -DHAVE_QSORT_S
+else
+ COMPAT_OBJS += compat/qsort_s.o
+endif
+
ifdef RUNTIME_PREFIX
COMPAT_CFLAGS += -DRUNTIME_PREFIX
endif
diff --git a/compat/qsort_s.c b/compat/qsort_s.c
new file mode 100644
index 000000000..52d1f0a73
--- /dev/null
+++ b/compat/qsort_s.c
@@ -0,0 +1,69 @@
+#include "../git-compat-util.h"
+
+/*
+ * A merge sort implementation, simplified from the qsort implementation
+ * by Mike Haertel, which is a part of the GNU C Library.
+ * Added context pointer, safety checks and return value.
+ */
+
+static void msort_with_tmp(void *b, size_t n, size_t s,
+ int (*cmp)(const void *, const void *, void *),
+ char *t, void *ctx)
+{
+ char *tmp;
+ char *b1, *b2;
+ size_t n1, n2;
+
+ if (n <= 1)
+ return;
+
+ n1 = n / 2;
+ n2 = n - n1;
+ b1 = b;
+ b2 = (char *)b + (n1 * s);
+
+ msort_with_tmp(b1, n1, s, cmp, t, ctx);
+ msort_with_tmp(b2, n2, s, cmp, t, ctx);
+
+ tmp = t;
+
+ while (n1 > 0 && n2 > 0) {
+ if (cmp(b1, b2, ctx) <= 0) {
+ memcpy(tmp, b1, s);
+ tmp += s;
+ b1 += s;
+ --n1;
+ } else {
+ memcpy(tmp, b2, s);
+ tmp += s;
+ b2 += s;
+ --n2;
+ }
+ }
+ if (n1 > 0)
+ memcpy(tmp, b1, n1 * s);
+ memcpy(b, t, (n - n2) * s);
+}
+
+int git_qsort_s(void *b, size_t n, size_t s,
+ int (*cmp)(const void *, const void *, void *), void *ctx)
+{
+ const size_t size = st_mult(n, s);
+ char buf[1024];
+
+ if (!n)
+ return 0;
+ if (!b || !cmp)
+ return -1;
+
+ if (size < sizeof(buf)) {
+ /* The temporary array fits on the small on-stack buffer. */
+ msort_with_tmp(b, n, s, cmp, buf, ctx);
+ } else {
+ /* It's somewhat large, so malloc it. */
+ char *tmp = xmalloc(size);
+ msort_with_tmp(b, n, s, cmp, tmp, ctx);
+ free(tmp);
+ }
+ return 0;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092..d25f0bd4c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -988,6 +988,12 @@ static inline void sane_qsort(void *base, size_t nmemb, size_t size,
qsort(base, nmemb, size, compar);
}
+#ifndef HAVE_QSORT_S
+int git_qsort_s(void *base, size_t nmemb, size_t size,
+ int (*compar)(const void *, const void *, void *), void *ctx);
+#define qsort_s git_qsort_s
+#endif
+
#ifndef REG_STARTEND
#error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd"
#endif
--
2.11.0
^ permalink raw reply related
* [PATCH 2/3] add QSORT_S
From: René Scharfe @ 2016-12-01 16:28 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <3083fbf7-d67e-77e4-e05f-94a7e7e15eba@web.de>
Add the macro QSORT_S, a convenient wrapper for qsort_s() that infers
the size of the array elements and dies on error.
Basically all possible errors are programming mistakes (passing NULL as
base of a non-empty array, passing NULL as comparison function,
out-of-bounds accesses), so terminating the program should be acceptable
for most callers.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
git-compat-util.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index d25f0bd4c..b707dd880 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -994,6 +994,11 @@ int git_qsort_s(void *base, size_t nmemb, size_t size,
#define qsort_s git_qsort_s
#endif
+#define QSORT_S(base, n, compar, ctx) do { \
+ if (qsort_s((base), (n), sizeof(*(base)), compar, ctx)) \
+ die("BUG: qsort_s() failed"); \
+} while (0)
+
#ifndef REG_STARTEND
#error "Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd"
#endif
--
2.11.0
^ permalink raw reply related
* [PATCH 3/3] string-list: use QSORT_S in string_list_sort()
From: René Scharfe @ 2016-12-01 16:29 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <3083fbf7-d67e-77e4-e05f-94a7e7e15eba@web.de>
Pass the comparison function to cmp_items() via the context parameter of
qsort_s() instead of using a global variable. That allows calling
string_list_sort() from multiple parallel threads.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
string-list.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/string-list.c b/string-list.c
index 8c83cac18..45016ad86 100644
--- a/string-list.c
+++ b/string-list.c
@@ -211,21 +211,18 @@ struct string_list_item *string_list_append(struct string_list *list,
list->strdup_strings ? xstrdup(string) : (char *)string);
}
-/* Yuck */
-static compare_strings_fn compare_for_qsort;
-
-/* Only call this from inside string_list_sort! */
-static int cmp_items(const void *a, const void *b)
+static int cmp_items(const void *a, const void *b, void *ctx)
{
+ compare_strings_fn cmp = ctx;
const struct string_list_item *one = a;
const struct string_list_item *two = b;
- return compare_for_qsort(one->string, two->string);
+ return cmp(one->string, two->string);
}
void string_list_sort(struct string_list *list)
{
- compare_for_qsort = list->cmp ? list->cmp : strcmp;
- QSORT(list->items, list->nr, cmp_items);
+ QSORT_S(list->items, list->nr, cmp_items,
+ list->cmp ? list->cmp : strcmp);
}
struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 1/3] compat: add qsort_s()
From: René Scharfe @ 2016-12-01 16:31 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <fc602a66-a06c-203e-b50b-55fd7b258b54@web.de>
Am 01.12.2016 um 17:26 schrieb René Scharfe:
> The function qsort_s() was introduced with C11 Annex K; it provides the
> ability to pass a context pointer to the comparison function, supports
> the convention of using a NULL pointer for an empty array and performs a
> few safety checks.
>
> Add an implementation based on compat/qsort.c for platforms that lack a
> native standards-compliant qsort_s() (i.e. basically everyone). It
> doesn't perform the full range of possible checks: It uses size_t
> instead of rsize_t and doesn't check nmemb and size against RSIZE_MAX
> because we probably don't have the restricted size type defined. For
> the same reason it returns int instead of errno_t.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> Makefile | 10 ++++++++
> compat/qsort_s.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> git-compat-util.h | 6 +++++
> 3 files changed, 85 insertions(+)
> create mode 100644 compat/qsort_s.c
And here's the diff for compat/qsort_s.c with copy detection (-C -C):
---
compat/{qsort.c => qsort_s.c} | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/compat/qsort.c b/compat/qsort_s.c
similarity index 62%
copy from compat/qsort.c
copy to compat/qsort_s.c
index 7d071afb7..52d1f0a73 100644
--- a/compat/qsort.c
+++ b/compat/qsort_s.c
@@ -3,11 +3,12 @@
/*
* A merge sort implementation, simplified from the qsort implementation
* by Mike Haertel, which is a part of the GNU C Library.
+ * Added context pointer, safety checks and return value.
*/
static void msort_with_tmp(void *b, size_t n, size_t s,
- int (*cmp)(const void *, const void *),
- char *t)
+ int (*cmp)(const void *, const void *, void *),
+ char *t, void *ctx)
{
char *tmp;
char *b1, *b2;
@@ -21,13 +22,13 @@ static void msort_with_tmp(void *b, size_t n, size_t s,
b1 = b;
b2 = (char *)b + (n1 * s);
- msort_with_tmp(b1, n1, s, cmp, t);
- msort_with_tmp(b2, n2, s, cmp, t);
+ msort_with_tmp(b1, n1, s, cmp, t, ctx);
+ msort_with_tmp(b2, n2, s, cmp, t, ctx);
tmp = t;
while (n1 > 0 && n2 > 0) {
- if (cmp(b1, b2) <= 0) {
+ if (cmp(b1, b2, ctx) <= 0) {
memcpy(tmp, b1, s);
tmp += s;
b1 += s;
@@ -44,19 +45,25 @@ static void msort_with_tmp(void *b, size_t n, size_t s,
memcpy(b, t, (n - n2) * s);
}
-void git_qsort(void *b, size_t n, size_t s,
- int (*cmp)(const void *, const void *))
+int git_qsort_s(void *b, size_t n, size_t s,
+ int (*cmp)(const void *, const void *, void *), void *ctx)
{
const size_t size = st_mult(n, s);
char buf[1024];
+ if (!n)
+ return 0;
+ if (!b || !cmp)
+ return -1;
+
if (size < sizeof(buf)) {
/* The temporary array fits on the small on-stack buffer. */
- msort_with_tmp(b, n, s, cmp, buf);
+ msort_with_tmp(b, n, s, cmp, buf, ctx);
} else {
/* It's somewhat large, so malloc it. */
char *tmp = xmalloc(size);
- msort_with_tmp(b, n, s, cmp, tmp);
+ msort_with_tmp(b, n, s, cmp, tmp, ctx);
free(tmp);
}
+ return 0;
}
^ permalink raw reply related
* Re: [ANNOUNCE] Git for Windows 2.11.0
From: Johannes Schindelin @ 2016-12-01 17:05 UTC (permalink / raw)
To: stefan.naewe; +Cc: git-for-windows, git
In-Reply-To: <77d1fa5d-869c-546e-357b-cd1e6ffee48d@atlas-elektronik.com>
Hi Stefan,
On Thu, 1 Dec 2016, stefan.naewe@atlas-elektronik.com wrote:
> Am 01.12.2016 um 13:31 schrieb Johannes Schindelin:
>
> > * 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 ?
Yes it is. We had a script to generate project files for Visual Studio <
2008, but they were broken.
This script has been partially fixed, and support has been added to
generate the .vcxproj used by Visual Studio 2010 and later.
Further, compile errors have been addressed.
After that, I worked on being able to run the tests in a regular Git Bash
(i.e. *without* installing Git for Windows' SDK, just a regular Git for
Windows will do).
Took a lot of effort and time, too,
Johannes
^ permalink raw reply
* Re: [PATCH v6 0/6] recursively grep across submodules
From: Brandon Williams @ 2016-12-01 17:45 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, jonathantanmy, gitster
In-Reply-To: <20161201042228.ynug33mcsqkdbuoe@sigill.intra.peff.net>
On 11/30, Jeff King wrote:
> 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
Yeah I was trying to think through these scenarios myself last night.
And like you found it seemed alright to let the child process deal with
the .git file/dir as long as once actually exists at that path. If one
didn't then there would be the possibility that we ended up back at the
superproject, which would result in an infinite loop. And yeah if the
.git file doesn't resolve to anything sensible then the user probably
mangled their repository somehow anyways.
Thanks again for all the help!
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v6 5/6] grep: enable recurse-submodules to work on <tree> objects
From: Brandon Williams @ 2016-12-01 17:51 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, peff, sbeller, jonathantanmy, gitster
In-Reply-To: <c6b2ddad-ac09-3457-8289-88a3f52b7e4b@kdbg.org>
On 12/01, Johannes Sixt wrote:
> 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
>
This test is needed to see if the code still works with filenames that
contain colons. Is there a way to mark the test to not run on windows?
--
Brandon Williams
^ permalink raw reply
* Re* git pull --rebase should use fast forward merge if possible
From: Junio C Hamano @ 2016-12-01 17:59 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: neuling, Stefan Beller
In-Reply-To: <CAPc5daURyXO6-yaOWPhvvdS8Dr5psEEc8MVP4wQJ_AuxyZraRg@mail.gmail.com>
Junio C Hamano <gitster@pobox.com> writes:
> die_no_merge_candidates() would have triggered, I would imagine.
>
> Note that I won't be applying this without test updates and proper log message,
> so no need to worry about the style yet ;-)
This time with a bit of explanation in the log and a trivial test.
I am no longer sure if this is a good idea myself, though. The
trivial case the new test covers is not interesting, even though it
may be worth protecting the behaviour with the test. What's more
interesting to think about is what should happen in various corner
cases. E.g. what should happen when there are local changes that
would conflict with the fast-forwarding? what should happen if the
user has rebase.autostash set in such a case? etc.
-- >8 --
Subject: [PATCH] pull: fast-forward "pull --rebase=true"
"git pull --rebase" always runs "git rebase" after fetching the
commit to serve as the new base, even when the new base is a
descendant of the current HEAD, i.e. we haven't done any work.
In such a case, we can instead fast-forward to the new base without
invoking the rebase process.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/pull.c | 22 ++++++++++++++++++----
t/t5520-pull.sh | 17 +++++++++++++++++
2 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index bf3fd3f9c8..2a41d415b2 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -878,10 +878,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (merge_heads.nr > 1)
die(_("Cannot merge multiple branches into empty head."));
return pull_into_void(*merge_heads.sha1, curr_head);
- } else if (opt_rebase) {
- if (merge_heads.nr > 1)
- die(_("Cannot rebase onto multiple branches."));
+ }
+ if (opt_rebase && merge_heads.nr > 1)
+ die(_("Cannot rebase onto multiple branches."));
+
+ if (opt_rebase) {
+ struct commit_list *list = NULL;
+ struct commit *merge_head, *head;
+
+ head = lookup_commit_reference(orig_head);
+ commit_list_insert(head, &list);
+ merge_head = lookup_commit_reference(merge_heads.sha1[0]);
+ if (is_descendant_of(merge_head, list)) {
+ /* we can fast-forward this without invoking rebase */
+ opt_ff = "--ff-only";
+ return run_merge();
+ }
return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
- } else
+ } else {
return run_merge();
+ }
}
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index a0013ee32f..7887b6d97b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -237,6 +237,23 @@ test_expect_success '--rebase' '
test new = "$(git show HEAD:file2)"
'
+test_expect_success '--rebase fast forward' '
+ git reset --hard before-rebase &&
+ git checkout -b ff &&
+ echo another modification >file &&
+ git commit -m third file &&
+
+ git checkout to-rebase &&
+ git pull --rebase . ff &&
+ test "$(git rev-parse HEAD)" = "$(git rev-parse ff)" &&
+
+ # The above only validates the result. Did we actually bypass rebase?
+ git reflog -1 >reflog.actual &&
+ sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
+ echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
+ test_cmp reflog.expected reflog.fuzzy
+'
+
test_expect_success '--rebase fails with multiple branches' '
git reset --hard before-rebase &&
test_must_fail git pull --rebase . copy master 2>err &&
--
2.11.0-192-gbadfaabe38
^ permalink raw reply related
* Re: bw/transport-protocol-policy
From: Brandon Williams @ 2016-12-01 18:14 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20161201083005.dui572o4jxsqacas@sigill.intra.peff.net>
On 12/01, Jeff King wrote:
> 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.
I agree and it should be an easy fix. We can just add a parameter like
so:
diff --git a/transport.c b/transport.c
index 2c0ec76..d38d50f 100644
--- a/transport.c
+++ b/transport.c
@@ -723,7 +723,7 @@ static enum protocol_allow_config get_protocol_config(const char *type)
return PROTOCOL_ALLOW_USER_ONLY;
}
-int is_transport_allowed(const char *type)
+int is_transport_allowed(const char *type, int redirect)
{
const struct string_list *whitelist = protocol_whitelist();
if (whitelist)
@@ -735,7 +735,7 @@ int is_transport_allowed(const char *type)
case PROTOCOL_ALLOW_NEVER:
return 0;
case PROTOCOL_ALLOW_USER_ONLY:
- return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ return git_env_bool("GIT_PROTOCOL_FROM_USER", !redirect);
}
die("BUG: invalid protocol_allow_config type");
That way the libcurl code can say it is asking if it is ok to redirect
to that protocol.
>
> 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).
We should switch to warning all the time since this series adds in
default whitelisted/blacklisted protocols anyways.
--
Brandon Williams
^ permalink raw reply related
* Re: [PATCH] difftool.c: mark a file-local symbol with static
From: Junio C Hamano @ 2016-12-01 18:20 UTC (permalink / raw)
To: Jeff King; +Cc: Ramsay Jones, Johannes Schindelin, GIT Mailing-list
In-Reply-To: <20161201040234.3rnuttitneweedn5@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 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.
What is most awkward is that "make", with no customization out of
box, suggests to use -Wall in CFLAGS and triggers the misguided
warning, and DEVELOPER_CFLAGS, partly because it adds -Werror to
turn warnings (including this misguided one) into errors, disables
it with -Wno-format-zero-length. As a result:
- Casual builders that follow our suggestion get warnings.
- Developers do not notice their new code may make the "casual
builder" experience worse.
That is totally backwards. That is probably what you meant by "the
least surprise and annoyance"?
I also still think that any_printf_like_function("%s", "") looks
silly. I know that we've already started moving in that direction
and we stopped at a place we do not want to be in, but perhaps it
was a mistake to move in that direction in the first place. I am
tempted to say we should do something like the attached, but that
may not fly well, as I suspect that -Wno-format-zero-length may be a
lot more exotic than the -Wall compiler option. An obvious second
best option would be to drop -Wall from the "everybody" CFLAGS and
move it to DEVELOPER_CFLAGS instead.
diff --git a/Makefile b/Makefile
index a379738db2..137c10e257 100644
--- a/Makefile
+++ b/Makefile
@@ -391,10 +391,9 @@ GIT-VERSION-FILE: FORCE
# CFLAGS and LDFLAGS are for the users to override from the command line.
-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -O2 -Wall -Wno-format-zero-length
DEVELOPER_CFLAGS = -Werror \
-Wdeclaration-after-statement \
- -Wno-format-zero-length \
-Wold-style-definition \
-Woverflow \
-Wpointer-arith \
^ permalink raw reply related
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