* git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0
[not found] <CAJa+X0OkzAX9E2SnDmU=on0yzzVZ9OMa2dJZgKMK=gQu2Rhf_Q@mail.gmail.com>
@ 2012-10-12 4:58 ` Brad Hein
2012-10-12 6:22 ` [PATCH] http: fix segfault in handle_curl_result Jeff King
2012-10-12 16:29 ` git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0 Erik Faye-Lund
0 siblings, 2 replies; 10+ messages in thread
From: Brad Hein @ 2012-10-12 4:58 UTC (permalink / raw)
To: git
In Fedora 17
With git-1.7.11.7-1.fc17.x86_64 (rpm)
I try to clone a particular repository but git just returns, having
not cloned the repo. Seems like a bug. Details follow:
$ git clone http://gnuradio.org/git/gnuradio.git
While the command fails a message is logged to syslog. Repeated
attempts to clone the repo yield the same result:
Oct 11 21:38:25 localhost kernel: [662703.442645]
git-remote-http[25796]: segfault at 0 ip 00000000004076d5 sp
00007fff7806ebc0 error 4 in git-remote-http[400000+96000]
Oct 11 21:39:00 localhost kernel: [662737.899829]
git-remote-http[25837]: segfault at 0 ip 00000000004076d5 sp
00007fff37c5ef20 error 4 in git-remote-http[400000+96000]
Oct 11 21:39:25 localhost kernel: [662763.341248]
git-remote-http[25873]: segfault at 0 ip 00000000004076d5 sp
00007fff6310d470 error 4 in git-remote-http[400000+96000]
A tcpdump reveals that the last thing the client does is requests a
file that doesn't exist on the server (404). Details are in my post on
FedoraForums: http://forums.fedoraforum.org/showthread.php?p=1607891&posted=1#post1607891
Problem mitigated by downgrade to "git-1.7.10.1-1.fc17.x86_64" or
"git-1.7.11.4-3.fc17.x86_64" or try to clone a different repository.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] http: fix segfault in handle_curl_result
2012-10-12 4:58 ` git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0 Brad Hein
@ 2012-10-12 6:22 ` Jeff King
2012-10-12 7:30 ` Jeff King
2012-10-12 16:46 ` [PATCH] http: fix segfault in handle_curl_result Junio C Hamano
2012-10-12 16:29 ` git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0 Erik Faye-Lund
1 sibling, 2 replies; 10+ messages in thread
From: Jeff King @ 2012-10-12 6:22 UTC (permalink / raw)
To: Brad Hein; +Cc: Junio C Hamano, git
On Fri, Oct 12, 2012 at 12:58:21AM -0400, Brad Hein wrote:
> In Fedora 17
> With git-1.7.11.7-1.fc17.x86_64 (rpm)
>
> I try to clone a particular repository but git just returns, having
> not cloned the repo. Seems like a bug. Details follow:
> $ git clone http://gnuradio.org/git/gnuradio.git
Thanks for a thorough bug report. I can reproduce here, and it was easy
to bisect. The culprit is 8809703 (http: factor out http error code
handling, 2012-08-27). The patch below should fix it.
-- >8 --
When we create an http active_request_slot, we can set its
"results" pointer back to local storage. The http code will
fill in the details of how the request went, and we can
access those details even after the slot has been cleaned
up.
Commit 8809703 (http: factor out http error code handling)
switched us from accessing our local results struct directly
to accessing it via the "results" pointer of the slot. That
means we're accessing the slot after it has been marked as
finished, defeating the whole purpose of keeping the results
storage separate.
Most of the time this doesn't matter, as finishing the slot
does not actually clean up the pointer. However, when using
curl's multi interface with the dumb-http revision walker,
we might actually start a new request before handing control
back to the original caller. In that case, we may reuse the
slot, zeroing its results pointer, and leading the original
caller to segfault while looking for its results inside the
slot.
Instead, we need to pass a pointer to our local results
storage to the handle_curl_result function, rather than
relying on the pointer in the slot struct. This matches what
the original code did before the refactoring (which did not
use a separate function, and therefore just accessed the
results struct directly).
Signed-off-by: Jeff King <peff@peff.net>
---
Junio, this goes on top of the jk/maint-http-half-auth-push topic.
The bug is released in v1.7.11.7 and in v1.7.12.1. No clue how difficult
it is to trigger in practice (it's very repeatable with this repo, but
it does not trigger when our test scripts do dumb-http fetches, so there
may be something with the distribution of loose objects, packs, and so
forth to trigger the right sequence of requests).
We should probably not be passing the slot to handle_curl_results at
all, since it may have already been reused and is not safe to read. The
only thing we do with it is to set up any new auth information in the
curl handle. This doesn't suffer the same problem because a reused slot
will always have a curl handle. However, it means we may be setting the
auth information for a random handle. Which is OK, since all handles use
the same auth information anyway. But it should also be pointless,
because since dfa1725 (fix http auth with multiple curl handles) we
always refresh the curl handle's auth information whenever we get an
active slot.
However, I'm leaving that out of this patch. Commit 8809703 was
supposed to be a refactor with zero behavior changes, but it regressed.
This fixes the regression by behaving exactly as we did beforehand. We
can build the other thing on top.
http.c | 7 +++----
http.h | 3 ++-
remote-curl.c | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/http.c b/http.c
index 7c4a407..9334386 100644
--- a/http.c
+++ b/http.c
@@ -744,10 +744,9 @@ int handle_curl_result(struct active_request_slot *slot)
return strbuf_detach(&buf, NULL);
}
-int handle_curl_result(struct active_request_slot *slot)
+int handle_curl_result(struct active_request_slot *slot,
+ struct slot_results *results)
{
- struct slot_results *results = slot->results;
-
if (results->curl_result == CURLE_OK) {
credential_approve(&http_auth);
return HTTP_OK;
@@ -818,7 +817,7 @@ static int http_request(const char *url, void *result, int target, int options)
if (start_active_slot(slot)) {
run_active_slot(slot);
- ret = handle_curl_result(slot);
+ ret = handle_curl_result(slot, &results);
} else {
error("Unable to start HTTP request for %s", url);
ret = HTTP_START_FAILED;
diff --git a/http.h b/http.h
index 12de255..0bd1e84 100644
--- a/http.h
+++ b/http.h
@@ -78,7 +78,8 @@ extern void finish_all_active_slots(void);
extern void run_active_slot(struct active_request_slot *slot);
extern void finish_active_slot(struct active_request_slot *slot);
extern void finish_all_active_slots(void);
-extern int handle_curl_result(struct active_request_slot *slot);
+extern int handle_curl_result(struct active_request_slot *slot,
+ struct slot_results *results);
#ifdef USE_CURL_MULTI
extern void fill_active_slots(void);
diff --git a/remote-curl.c b/remote-curl.c
index 3ec474f..6054e47 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -369,7 +369,7 @@ static int run_slot(struct active_request_slot *slot)
slot->curl_result = curl_easy_perform(slot->curl);
finish_active_slot(slot);
- err = handle_curl_result(slot);
+ err = handle_curl_result(slot, &results);
if (err != HTTP_OK && err != HTTP_REAUTH) {
error("RPC failed; result=%d, HTTP code = %ld",
results.curl_result, results.http_code);
--
1.8.0.rc2.3.g303f317
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] http: fix segfault in handle_curl_result
2012-10-12 6:22 ` [PATCH] http: fix segfault in handle_curl_result Jeff King
@ 2012-10-12 7:30 ` Jeff King
2012-10-12 7:35 ` [PATCH 1/2] remote-curl: do not call run_slot repeatedly Jeff King
2012-10-12 7:35 ` [PATCH 2/2] http: do not set up curl auth after a 401 Jeff King
2012-10-12 16:46 ` [PATCH] http: fix segfault in handle_curl_result Junio C Hamano
1 sibling, 2 replies; 10+ messages in thread
From: Jeff King @ 2012-10-12 7:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brad Hein, git
On Fri, Oct 12, 2012 at 02:22:49AM -0400, Jeff King wrote:
> We should probably not be passing the slot to handle_curl_results at
> all, since it may have already been reused and is not safe to read. The
> only thing we do with it is to set up any new auth information in the
> curl handle. This doesn't suffer the same problem because a reused slot
> will always have a curl handle. However, it means we may be setting the
> auth information for a random handle. Which is OK, since all handles use
> the same auth information anyway. But it should also be pointless,
> because since dfa1725 (fix http auth with multiple curl handles) we
> always refresh the curl handle's auth information whenever we get an
> active slot.
>
> However, I'm leaving that out of this patch. Commit 8809703 was
> supposed to be a refactor with zero behavior changes, but it regressed.
> This fixes the regression by behaving exactly as we did beforehand. We
> can build the other thing on top.
I took a look at this, and indeed, it breaks existing code. But the
broken code is wrong and is easy to fix. So here is a series to do this,
on top of the one I am responding to.
These ones shouldn't have any functional impact, but do clean up the
code. I'd recommend the regression fix I already posted for maint and
1.8.0, and leave these to cook for post-1.8.0.
[1/2]: remote-curl: do not call run_slot repeatedly
[2/2]: http: do not set up curl auth after a 401
-Peff
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] remote-curl: do not call run_slot repeatedly
2012-10-12 7:30 ` Jeff King
@ 2012-10-12 7:35 ` Jeff King
2012-10-12 7:35 ` [PATCH 2/2] http: do not set up curl auth after a 401 Jeff King
1 sibling, 0 replies; 10+ messages in thread
From: Jeff King @ 2012-10-12 7:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brad Hein, git
Commit b81401c (http: prompt for credentials on failed POST)
taught post_rpc to call run_slot in a loop in order to retry
a request after asking the user for credentials. However,
after a call to run_slot we will have called
finish_active_slot. This means we have released the slot,
and we should no longer look at it.
As it happens, this does not cause any bugs in the current
code, since we know that we are not using curl_multi in this
code path, and therefore nobody will have taken over our
slot in the meantime. However, it is good form to actually
call get_active_slot again. It also future proofs us against
changes in the http code.
We can do this by jumping back to a retry label at the top
of our function. We just need to reorder a few setup lines
that should not be repeated; everything else within the loop
is either idempotent, needs to be repeated, or in a path we
do not follow (e.g., we do not even try when large_request
is set, because we don't know how much data we might have
streamed from our helper program).
Signed-off-by: Jeff King <peff@peff.net>
---
Without this future-proofing, the next patch causes test
failures.
If the goto is too ugly, we can pull it out into a loop (it
just makes the patch harder to read, because the meat of the
function gets indented further).
remote-curl.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 6054e47..4281262 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -444,6 +444,11 @@ static int post_rpc(struct rpc_state *rpc)
return -1;
}
+ headers = curl_slist_append(headers, rpc->hdr_content_type);
+ headers = curl_slist_append(headers, rpc->hdr_accept);
+ headers = curl_slist_append(headers, "Expect:");
+
+retry:
slot = get_active_slot();
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
@@ -451,10 +456,6 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
- headers = curl_slist_append(headers, rpc->hdr_content_type);
- headers = curl_slist_append(headers, rpc->hdr_accept);
- headers = curl_slist_append(headers, "Expect:");
-
if (large_request) {
/* The request body is large and the size cannot be predicted.
* We must use chunked encoding to send it.
@@ -528,9 +529,9 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
- do {
- err = run_slot(slot);
- } while (err == HTTP_REAUTH && !large_request && !use_gzip);
+ err = run_slot(slot);
+ if (err == HTTP_REAUTH && !large_request && !use_gzip)
+ goto retry;
if (err != HTTP_OK)
err = -1;
--
1.8.0.rc2.3.g303f317
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] http: do not set up curl auth after a 401
2012-10-12 7:30 ` Jeff King
2012-10-12 7:35 ` [PATCH 1/2] remote-curl: do not call run_slot repeatedly Jeff King
@ 2012-10-12 7:35 ` Jeff King
2012-10-12 13:39 ` Brad Hein
1 sibling, 1 reply; 10+ messages in thread
From: Jeff King @ 2012-10-12 7:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brad Hein, git
When we get an http 401, we prompt for credentials and put
them in our global credential struct. We also feed them to
the curl handle that produced the 401, with the intent that
they will be used on a retry.
When the code was originally introduced in commit 42653c0,
this was a necessary step. However, since dfa1725, we always
feed our global credential into every curl handle when we
initialize the slot with get_active_slot. So every further
request already feeds the credential to curl.
Moreover, accessing the slot here is somewhat dubious. After
the slot has produced a response, we don't actually control
it any more. If we are using curl_multi, it may even have
been re-initialized to handle a different request.
It just so happens that we will reuse the curl handle within
the slot in such a case, and that because we only keep one
global credential, it will be the one we want. So the
current code is not buggy, but it is misleading.
By cleaning it up, we can remove the slot argument entirely
from handle_curl_result, making it much more obvious that
slots should not be accessed after they are marked as
finished.
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 6 ++----
http.h | 3 +--
remote-curl.c | 2 +-
3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/http.c b/http.c
index 9334386..0a74345 100644
--- a/http.c
+++ b/http.c
@@ -744,8 +744,7 @@ char *get_remote_object_url(const char *url, const char *hex,
return strbuf_detach(&buf, NULL);
}
-int handle_curl_result(struct active_request_slot *slot,
- struct slot_results *results)
+int handle_curl_result(struct slot_results *results)
{
if (results->curl_result == CURLE_OK) {
credential_approve(&http_auth);
@@ -758,7 +757,6 @@ int handle_curl_result(struct active_request_slot *slot,
return HTTP_NOAUTH;
} else {
credential_fill(&http_auth);
- init_curl_http_auth(slot->curl);
return HTTP_REAUTH;
}
} else {
@@ -817,7 +815,7 @@ static int http_request(const char *url, void *result, int target, int options)
if (start_active_slot(slot)) {
run_active_slot(slot);
- ret = handle_curl_result(slot, &results);
+ ret = handle_curl_result(&results);
} else {
error("Unable to start HTTP request for %s", url);
ret = HTTP_START_FAILED;
diff --git a/http.h b/http.h
index 0bd1e84..0a80d30 100644
--- a/http.h
+++ b/http.h
@@ -78,8 +78,7 @@ extern void finish_all_active_slots(void);
extern void run_active_slot(struct active_request_slot *slot);
extern void finish_active_slot(struct active_request_slot *slot);
extern void finish_all_active_slots(void);
-extern int handle_curl_result(struct active_request_slot *slot,
- struct slot_results *results);
+extern int handle_curl_result(struct slot_results *results);
#ifdef USE_CURL_MULTI
extern void fill_active_slots(void);
diff --git a/remote-curl.c b/remote-curl.c
index 4281262..aefafd3 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -369,7 +369,7 @@ static int run_slot(struct active_request_slot *slot)
slot->curl_result = curl_easy_perform(slot->curl);
finish_active_slot(slot);
- err = handle_curl_result(slot, &results);
+ err = handle_curl_result(&results);
if (err != HTTP_OK && err != HTTP_REAUTH) {
error("RPC failed; result=%d, HTTP code = %ld",
results.curl_result, results.http_code);
--
1.8.0.rc2.3.g303f317
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] http: do not set up curl auth after a 401
2012-10-12 7:35 ` [PATCH 2/2] http: do not set up curl auth after a 401 Jeff King
@ 2012-10-12 13:39 ` Brad Hein
0 siblings, 0 replies; 10+ messages in thread
From: Brad Hein @ 2012-10-12 13:39 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
Nice work sorting this out. I don't see the commit on github
(https://github.com/git/git/commits/master) yet but once the code is
available I'll be happy to re-test if needed.
On Fri, Oct 12, 2012 at 3:35 AM, Jeff King <peff@peff.net> wrote:
> When we get an http 401, we prompt for credentials and put
> them in our global credential struct. We also feed them to
> the curl handle that produced the 401, with the intent that
> they will be used on a retry.
>
> When the code was originally introduced in commit 42653c0,
> this was a necessary step. However, since dfa1725, we always
> feed our global credential into every curl handle when we
> initialize the slot with get_active_slot. So every further
> request already feeds the credential to curl.
>
> Moreover, accessing the slot here is somewhat dubious. After
> the slot has produced a response, we don't actually control
> it any more. If we are using curl_multi, it may even have
> been re-initialized to handle a different request.
>
> It just so happens that we will reuse the curl handle within
> the slot in such a case, and that because we only keep one
> global credential, it will be the one we want. So the
> current code is not buggy, but it is misleading.
>
> By cleaning it up, we can remove the slot argument entirely
> from handle_curl_result, making it much more obvious that
> slots should not be accessed after they are marked as
> finished.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> http.c | 6 ++----
> http.h | 3 +--
> remote-curl.c | 2 +-
> 3 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/http.c b/http.c
> index 9334386..0a74345 100644
> --- a/http.c
> +++ b/http.c
> @@ -744,8 +744,7 @@ char *get_remote_object_url(const char *url, const char *hex,
> return strbuf_detach(&buf, NULL);
> }
>
> -int handle_curl_result(struct active_request_slot *slot,
> - struct slot_results *results)
> +int handle_curl_result(struct slot_results *results)
> {
> if (results->curl_result == CURLE_OK) {
> credential_approve(&http_auth);
> @@ -758,7 +757,6 @@ int handle_curl_result(struct active_request_slot *slot,
> return HTTP_NOAUTH;
> } else {
> credential_fill(&http_auth);
> - init_curl_http_auth(slot->curl);
> return HTTP_REAUTH;
> }
> } else {
> @@ -817,7 +815,7 @@ static int http_request(const char *url, void *result, int target, int options)
>
> if (start_active_slot(slot)) {
> run_active_slot(slot);
> - ret = handle_curl_result(slot, &results);
> + ret = handle_curl_result(&results);
> } else {
> error("Unable to start HTTP request for %s", url);
> ret = HTTP_START_FAILED;
> diff --git a/http.h b/http.h
> index 0bd1e84..0a80d30 100644
> --- a/http.h
> +++ b/http.h
> @@ -78,8 +78,7 @@ extern void finish_all_active_slots(void);
> extern void run_active_slot(struct active_request_slot *slot);
> extern void finish_active_slot(struct active_request_slot *slot);
> extern void finish_all_active_slots(void);
> -extern int handle_curl_result(struct active_request_slot *slot,
> - struct slot_results *results);
> +extern int handle_curl_result(struct slot_results *results);
>
> #ifdef USE_CURL_MULTI
> extern void fill_active_slots(void);
> diff --git a/remote-curl.c b/remote-curl.c
> index 4281262..aefafd3 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -369,7 +369,7 @@ static int run_slot(struct active_request_slot *slot)
> slot->curl_result = curl_easy_perform(slot->curl);
> finish_active_slot(slot);
>
> - err = handle_curl_result(slot, &results);
> + err = handle_curl_result(&results);
> if (err != HTTP_OK && err != HTTP_REAUTH) {
> error("RPC failed; result=%d, HTTP code = %ld",
> results.curl_result, results.http_code);
> --
> 1.8.0.rc2.3.g303f317
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0
2012-10-12 4:58 ` git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0 Brad Hein
2012-10-12 6:22 ` [PATCH] http: fix segfault in handle_curl_result Jeff King
@ 2012-10-12 16:29 ` Erik Faye-Lund
2012-10-12 16:34 ` Erik Faye-Lund
1 sibling, 1 reply; 10+ messages in thread
From: Erik Faye-Lund @ 2012-10-12 16:29 UTC (permalink / raw)
To: Brad Hein; +Cc: git, Jeff King
On Fri, Oct 12, 2012 at 6:58 AM, Brad Hein <linuxbrad@gmail.com> wrote:
> In Fedora 17
> With git-1.7.11.7-1.fc17.x86_64 (rpm)
>
> I try to clone a particular repository but git just returns, having
> not cloned the repo. Seems like a bug. Details follow:
> $ git clone http://gnuradio.org/git/gnuradio.git
>
> While the command fails a message is logged to syslog. Repeated
> attempts to clone the repo yield the same result:
> Oct 11 21:38:25 localhost kernel: [662703.442645]
> git-remote-http[25796]: segfault at 0 ip 00000000004076d5 sp
> 00007fff7806ebc0 error 4 in git-remote-http[400000+96000]
> Oct 11 21:39:00 localhost kernel: [662737.899829]
> git-remote-http[25837]: segfault at 0 ip 00000000004076d5 sp
> 00007fff37c5ef20 error 4 in git-remote-http[400000+96000]
> Oct 11 21:39:25 localhost kernel: [662763.341248]
> git-remote-http[25873]: segfault at 0 ip 00000000004076d5 sp
> 00007fff6310d470 error 4 in git-remote-http[400000+96000]
>
> A tcpdump reveals that the last thing the client does is requests a
> file that doesn't exist on the server (404). Details are in my post on
> FedoraForums: http://forums.fedoraforum.org/showthread.php?p=1607891&posted=1#post1607891
>
> Problem mitigated by downgrade to "git-1.7.10.1-1.fc17.x86_64" or
> "git-1.7.11.4-3.fc17.x86_64" or try to clone a different repository.
Thanks for reporting. I gave it a quick go, and the issue seems to
also be present in the current 'master'.
The problem is a NULL-pointer dereferencing introduced in 8809703
("http: factor out http error code handling"), where the code assume
that slot->results still points to http_request::results. This
assumption seems to be wrong.
This seems to step around the issue, but I don't know if
http_request::results should be set to NULL in the first place. Jeff?
diff --git a/http.c b/http.c
index 345c171..ac3ab16 100644
--- a/http.c
+++ b/http.c
@@ -745,10 +745,8 @@ char *get_remote_object_url(const char *url,
const char *hex,
return strbuf_detach(&buf, NULL);
}
-int handle_curl_result(struct active_request_slot *slot)
+int handle_curl_result(struct active_request_slot *slot, struct
slot_results *results)
{
- struct slot_results *results = slot->results;
-
if (results->curl_result == CURLE_OK) {
credential_approve(&http_auth);
return HTTP_OK;
@@ -822,7 +820,7 @@ static int http_request(const char *url, void
*result, int target, int options)
if (start_active_slot(slot)) {
run_active_slot(slot);
- ret = handle_curl_result(slot);
+ ret = handle_curl_result(slot, &results);
} else {
error("Unable to start HTTP request for %s", url);
ret = HTTP_START_FAILED;
diff --git a/http.h b/http.h
index 12de255..12c27fa 100644
--- a/http.h
+++ b/http.h
@@ -78,7 +78,7 @@ extern int start_active_slot(struct
active_request_slot *slot);
extern void run_active_slot(struct active_request_slot *slot);
extern void finish_active_slot(struct active_request_slot *slot);
extern void finish_all_active_slots(void);
-extern int handle_curl_result(struct active_request_slot *slot);
+extern int handle_curl_result(struct active_request_slot *slot,
struct slot_results *results);
#ifdef USE_CURL_MULTI
extern void fill_active_slots(void);
diff --git a/remote-curl.c b/remote-curl.c
index 10fa8f1..42716c5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -356,7 +356,7 @@ static int run_slot(struct active_request_slot *slot)
slot->curl_result = curl_easy_perform(slot->curl);
finish_active_slot(slot);
- err = handle_curl_result(slot);
+ err = handle_curl_result(slot, &results);
if (err != HTTP_OK && err != HTTP_REAUTH) {
error("RPC failed; result=%d, HTTP code = %ld",
results.curl_result, results.http_code);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0
2012-10-12 16:29 ` git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0 Erik Faye-Lund
@ 2012-10-12 16:34 ` Erik Faye-Lund
2012-10-12 17:12 ` Jeff King
0 siblings, 1 reply; 10+ messages in thread
From: Erik Faye-Lund @ 2012-10-12 16:34 UTC (permalink / raw)
To: Brad Hein; +Cc: git, Jeff King
On Fri, Oct 12, 2012 at 6:29 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Fri, Oct 12, 2012 at 6:58 AM, Brad Hein <linuxbrad@gmail.com> wrote:
>> In Fedora 17
>> With git-1.7.11.7-1.fc17.x86_64 (rpm)
>>
>> I try to clone a particular repository but git just returns, having
>> not cloned the repo. Seems like a bug. Details follow:
>> $ git clone http://gnuradio.org/git/gnuradio.git
>>
>> While the command fails a message is logged to syslog. Repeated
>> attempts to clone the repo yield the same result:
>> Oct 11 21:38:25 localhost kernel: [662703.442645]
>> git-remote-http[25796]: segfault at 0 ip 00000000004076d5 sp
>> 00007fff7806ebc0 error 4 in git-remote-http[400000+96000]
>> Oct 11 21:39:00 localhost kernel: [662737.899829]
>> git-remote-http[25837]: segfault at 0 ip 00000000004076d5 sp
>> 00007fff37c5ef20 error 4 in git-remote-http[400000+96000]
>> Oct 11 21:39:25 localhost kernel: [662763.341248]
>> git-remote-http[25873]: segfault at 0 ip 00000000004076d5 sp
>> 00007fff6310d470 error 4 in git-remote-http[400000+96000]
>>
>> A tcpdump reveals that the last thing the client does is requests a
>> file that doesn't exist on the server (404). Details are in my post on
>> FedoraForums: http://forums.fedoraforum.org/showthread.php?p=1607891&posted=1#post1607891
>>
>> Problem mitigated by downgrade to "git-1.7.10.1-1.fc17.x86_64" or
>> "git-1.7.11.4-3.fc17.x86_64" or try to clone a different repository.
>
> Thanks for reporting. I gave it a quick go, and the issue seems to
> also be present in the current 'master'.
>
> The problem is a NULL-pointer dereferencing introduced in 8809703
> ("http: factor out http error code handling"), where the code assume
> that slot->results still points to http_request::results. This
> assumption seems to be wrong.
>
> This seems to step around the issue, but I don't know if
> http_request::results should be set to NULL in the first place. Jeff?
OK, it seems I jumped the gun, and Jeff already sent out a patch for
it. Nevermind me, then :)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] http: fix segfault in handle_curl_result
2012-10-12 6:22 ` [PATCH] http: fix segfault in handle_curl_result Jeff King
2012-10-12 7:30 ` Jeff King
@ 2012-10-12 16:46 ` Junio C Hamano
1 sibling, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2012-10-12 16:46 UTC (permalink / raw)
To: Jeff King; +Cc: Brad Hein, git
Jeff King <peff@peff.net> writes:
> When we create an http active_request_slot, we can set its
> "results" pointer back to local storage. The http code will
> fill in the details of how the request went, and we can
> access those details even after the slot has been cleaned
> up.
> ...
> However, I'm leaving that out of this patch. Commit 8809703 was
> supposed to be a refactor with zero behavior changes, but it regressed.
> This fixes the regression by behaving exactly as we did beforehand. We
> can build the other thing on top.
Thanks for a good write-up.
I agree with the fix, and I also agree that it does not make sense
to pass slot to handle-curl-RESULT when we know the slot may not
have anything to do with the result we are dealing with. If
anything, we should be passing result (which this patch already
does) and curl handle (the sole reason why slot is passed, after
this patch, becomes to let the function access it via slot->curl),
but as your analysis shows, with dfa1725 (post 1.7.10.2) it should
not be even needed to call init_curl_http_auth() here, which is the
[2/2] of your follow-up.
Happy I am. Thanks.
> http.c | 7 +++----
> http.h | 3 ++-
> remote-curl.c | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/http.c b/http.c
> index 7c4a407..9334386 100644
> --- a/http.c
> +++ b/http.c
> @@ -744,10 +744,9 @@ int handle_curl_result(struct active_request_slot *slot)
> return strbuf_detach(&buf, NULL);
> }
>
> -int handle_curl_result(struct active_request_slot *slot)
> +int handle_curl_result(struct active_request_slot *slot,
> + struct slot_results *results)
> {
> - struct slot_results *results = slot->results;
> -
> if (results->curl_result == CURLE_OK) {
> credential_approve(&http_auth);
> return HTTP_OK;
> @@ -818,7 +817,7 @@ static int http_request(const char *url, void *result, int target, int options)
>
> if (start_active_slot(slot)) {
> run_active_slot(slot);
> - ret = handle_curl_result(slot);
> + ret = handle_curl_result(slot, &results);
> } else {
> error("Unable to start HTTP request for %s", url);
> ret = HTTP_START_FAILED;
> diff --git a/http.h b/http.h
> index 12de255..0bd1e84 100644
> --- a/http.h
> +++ b/http.h
> @@ -78,7 +78,8 @@ extern void finish_all_active_slots(void);
> extern void run_active_slot(struct active_request_slot *slot);
> extern void finish_active_slot(struct active_request_slot *slot);
> extern void finish_all_active_slots(void);
> -extern int handle_curl_result(struct active_request_slot *slot);
> +extern int handle_curl_result(struct active_request_slot *slot,
> + struct slot_results *results);
>
> #ifdef USE_CURL_MULTI
> extern void fill_active_slots(void);
> diff --git a/remote-curl.c b/remote-curl.c
> index 3ec474f..6054e47 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -369,7 +369,7 @@ static int run_slot(struct active_request_slot *slot)
> slot->curl_result = curl_easy_perform(slot->curl);
> finish_active_slot(slot);
>
> - err = handle_curl_result(slot);
> + err = handle_curl_result(slot, &results);
> if (err != HTTP_OK && err != HTTP_REAUTH) {
> error("RPC failed; result=%d, HTTP code = %ld",
> results.curl_result, results.http_code);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0
2012-10-12 16:34 ` Erik Faye-Lund
@ 2012-10-12 17:12 ` Jeff King
0 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2012-10-12 17:12 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: Brad Hein, git
On Fri, Oct 12, 2012 at 06:34:30PM +0200, Erik Faye-Lund wrote:
> > Thanks for reporting. I gave it a quick go, and the issue seems to
> > also be present in the current 'master'.
> >
> > The problem is a NULL-pointer dereferencing introduced in 8809703
> > ("http: factor out http error code handling"), where the code assume
> > that slot->results still points to http_request::results. This
> > assumption seems to be wrong.
> >
> > This seems to step around the issue, but I don't know if
> > http_request::results should be set to NULL in the first place. Jeff?
>
> OK, it seems I jumped the gun, and Jeff already sent out a patch for
> it. Nevermind me, then :)
Independent verification is never a bad thing. :)
-Peff
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-10-12 17:13 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CAJa+X0OkzAX9E2SnDmU=on0yzzVZ9OMa2dJZgKMK=gQu2Rhf_Q@mail.gmail.com>
2012-10-12 4:58 ` git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0 Brad Hein
2012-10-12 6:22 ` [PATCH] http: fix segfault in handle_curl_result Jeff King
2012-10-12 7:30 ` Jeff King
2012-10-12 7:35 ` [PATCH 1/2] remote-curl: do not call run_slot repeatedly Jeff King
2012-10-12 7:35 ` [PATCH 2/2] http: do not set up curl auth after a 401 Jeff King
2012-10-12 13:39 ` Brad Hein
2012-10-12 16:46 ` [PATCH] http: fix segfault in handle_curl_result Junio C Hamano
2012-10-12 16:29 ` git fails: segfault at 0 ip 00000000004076d5 sp 00007fff7806ebc0 Erik Faye-Lund
2012-10-12 16:34 ` Erik Faye-Lund
2012-10-12 17:12 ` Jeff King
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).