From 219f318c051a37c05dd66687695ebec9f1a90e53 Mon Sep 17 00:00:00 2001 From: Etienne Samson Date: Wed, 3 Jul 2013 22:02:29 +0200 Subject: [PATCH 1/6] Fix a crash if git_remote_set_cred_acquire_cb wasn't called before connecting. Fixes #1700. --- src/transports/ssh.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/transports/ssh.c b/src/transports/ssh.c index a312c8d08..8e3657dde 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -345,14 +345,16 @@ static int _git_ssh_setup_conn( if (user && pass) { if (git_cred_userpass_plaintext_new(&t->cred, user, pass) < 0) goto on_error; - } else { + } else if (t->owner->cred_acquire_cb) { if (t->owner->cred_acquire_cb(&t->cred, t->owner->url, user, GIT_CREDTYPE_USERPASS_PLAINTEXT | GIT_CREDTYPE_SSH_KEYFILE_PASSPHRASE, t->owner->cred_acquire_payload) < 0) return -1; - } + } else { + goto on_error; + } assert(t->cred); if (!user) { From f6bd0863352f70905b3068284f8ccd6beaa9a590 Mon Sep 17 00:00:00 2001 From: Etienne Samson Date: Wed, 3 Jul 2013 22:02:44 +0200 Subject: [PATCH 2/6] Fix a probable leak. --- src/transports/ssh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transports/ssh.c b/src/transports/ssh.c index 8e3657dde..f2480b2cd 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -351,7 +351,7 @@ static int _git_ssh_setup_conn( user, GIT_CREDTYPE_USERPASS_PLAINTEXT | GIT_CREDTYPE_SSH_KEYFILE_PASSPHRASE, t->owner->cred_acquire_payload) < 0) - return -1; + goto on_error; } else { goto on_error; } From 367c1903e9aefd07de0d5be98c56640d13e3420d Mon Sep 17 00:00:00 2001 From: Etienne Samson Date: Wed, 10 Jul 2013 10:29:09 +0200 Subject: [PATCH 3/6] Add some missing error messages. --- src/transports/ssh.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/transports/ssh.c b/src/transports/ssh.c index f2480b2cd..bb3843886 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -353,24 +353,31 @@ static int _git_ssh_setup_conn( t->owner->cred_acquire_payload) < 0) goto on_error; } else { - goto on_error; - } + giterr_set(GITERR_NET, "Cannot set up SSH connection without credentials"); + goto on_error; + } assert(t->cred); if (!user) { user = git__strdup(default_user); } - if (_git_ssh_session_create(&session, s->socket) < 0) + if (_git_ssh_session_create(&session, s->socket) < 0) { + giterr_set(GITERR_NET, "Failed to initialize SSH session"); goto on_error; - - if (_git_ssh_authenticate_session(session, user, t->cred) < 0) + } + + if (_git_ssh_authenticate_session(session, user, t->cred) < 0) { + giterr_set(GITERR_NET, "Failed to authenticate SSH session"); goto on_error; - + } + channel = libssh2_channel_open_session(session); - if (!channel) - goto on_error; - + if (!channel) { + giterr_set(GITERR_NET, "Failed to open SSH channel"); + goto on_error; + } + libssh2_channel_set_blocking(channel, 1); s->session = session; From 08bf80fa2bf95e17af79a1da5832aefb275d9565 Mon Sep 17 00:00:00 2001 From: Etienne Samson Date: Wed, 10 Jul 2013 10:29:32 +0200 Subject: [PATCH 4/6] Tab indent. --- src/transports/ssh.c | 182 ++++++++++++++++++++++--------------------- 1 file changed, 92 insertions(+), 90 deletions(-) diff --git a/src/transports/ssh.c b/src/transports/ssh.c index bb3843886..2eaaa45c9 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -46,27 +46,27 @@ typedef struct { static int gen_proto(git_buf *request, const char *cmd, const char *url) { char *repo; - + if (!git__prefixcmp(url, prefix_ssh)) { url = url + strlen(prefix_ssh); repo = strchr(url, '/'); } else { repo = strchr(url, ':'); } - + if (!repo) { return -1; } - + int len = strlen(cmd) + 1 /* Space */ + 1 /* Quote */ + strlen(repo) + 1 /* Quote */ + 1; - + git_buf_grow(request, len); git_buf_printf(request, "%s '%s'", cmd, repo); git_buf_putc(request, '\0'); - + if (git_buf_oom(request)) return -1; - + return 0; } @@ -74,11 +74,11 @@ static int send_command(ssh_stream *s) { int error; git_buf request = GIT_BUF_INIT; - + error = gen_proto(&request, s->cmd, s->url); if (error < 0) goto cleanup; - + error = libssh2_channel_exec( s->channel, request.ptr @@ -86,9 +86,9 @@ static int send_command(ssh_stream *s) if (0 != error) goto cleanup; - + s->sent_command = 1; - + cleanup: git_buf_free(&request); return error; @@ -101,18 +101,18 @@ static int ssh_stream_read( size_t *bytes_read) { ssh_stream *s = (ssh_stream *)stream; - + *bytes_read = 0; - + if (!s->sent_command && send_command(s) < 0) return -1; - + int rc = libssh2_channel_read(s->channel, buffer, buf_size); if (rc < 0) return -1; - + *bytes_read = rc; - + return 0; } @@ -122,15 +122,15 @@ static int ssh_stream_write( size_t len) { ssh_stream *s = (ssh_stream *)stream; - + if (!s->sent_command && send_command(s) < 0) return -1; - + int rc = libssh2_channel_write(s->channel, buffer, len); if (rc < 0) { return -1; } - + return rc; } @@ -139,26 +139,26 @@ static void ssh_stream_free(git_smart_subtransport_stream *stream) ssh_stream *s = (ssh_stream *)stream; ssh_subtransport *t = OWNING_SUBTRANSPORT(s); int ret; - + GIT_UNUSED(ret); - + t->current_stream = NULL; - + if (s->channel) { libssh2_channel_close(s->channel); libssh2_channel_free(s->channel); s->channel = NULL; } - + if (s->session) { libssh2_session_free(s->session), s->session = NULL; } - + if (s->socket.socket) { ret = gitno_close(&s->socket); assert(!ret); } - + git__free(s->url); git__free(s); } @@ -170,26 +170,26 @@ static int ssh_stream_alloc( git_smart_subtransport_stream **stream) { ssh_stream *s; - + if (!stream) return -1; - + s = git__calloc(sizeof(ssh_stream), 1); GITERR_CHECK_ALLOC(s); - + s->parent.subtransport = &t->parent; s->parent.read = ssh_stream_read; s->parent.write = ssh_stream_write; s->parent.free = ssh_stream_free; - + s->cmd = cmd; s->url = git__strdup(url); - + if (!s->url) { git__free(s); return -1; } - + *stream = &s->parent; return 0; } @@ -201,14 +201,14 @@ static int git_ssh_extract_url_parts( { char *colon, *at; const char *start; - + colon = strchr(url, ':'); - + if (colon == NULL) { giterr_set(GITERR_NET, "Malformed URL: missing :"); return -1; } - + at = strchr(url, '@'); if (at) { start = at+1; @@ -217,9 +217,9 @@ static int git_ssh_extract_url_parts( start = url; *username = git__strdup(default_user); } - + *host = git__substrdup(start, colon - start); - + return 0; } @@ -235,7 +235,7 @@ static int _git_ssh_authenticate_session( case GIT_CREDTYPE_USERPASS_PLAINTEXT: { git_cred_userpass_plaintext *c = (git_cred_userpass_plaintext *)cred; rc = libssh2_userauth_password( - session, + session, c->username, c->password ); @@ -244,7 +244,7 @@ static int _git_ssh_authenticate_session( case GIT_CREDTYPE_SSH_KEYFILE_PASSPHRASE: { git_cred_ssh_keyfile_passphrase *c = (git_cred_ssh_keyfile_passphrase *)cred; rc = libssh2_userauth_publickey_fromfile( - session, + session, user, c->publickey, c->privatekey, @@ -267,13 +267,12 @@ static int _git_ssh_authenticate_session( default: rc = LIBSSH2_ERROR_AUTHENTICATION_FAILED; } - } while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc); - - return rc; + } while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc); + + return rc; } -static int _git_ssh_session_create -( +static int _git_ssh_session_create( LIBSSH2_SESSION** session, gitno_socket socket ) @@ -281,31 +280,30 @@ static int _git_ssh_session_create if (!session) { return -1; } - + LIBSSH2_SESSION* s = libssh2_session_init(); - if (!s) - return -1; - - int rc = 0; - do { - rc = libssh2_session_startup(s, socket.socket); - } while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc); - - if (0 != rc) { - goto on_error; - } - + if (!s) + return -1; + + int rc = 0; + do { + rc = libssh2_session_startup(s, socket.socket); + } while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc); + + if (0 != rc) + goto on_error; + libssh2_session_set_blocking(s, 1); - + *session = s; - + return 0; - + on_error: if (s) { libssh2_session_free(s), s = NULL; } - + return -1; } @@ -321,13 +319,13 @@ static int _git_ssh_setup_conn( ssh_stream *s; LIBSSH2_SESSION* session=NULL; LIBSSH2_CHANNEL* channel=NULL; - + *stream = NULL; if (ssh_stream_alloc(t, url, cmd, stream) < 0) return -1; - + s = (ssh_stream *)*stream; - + if (!git__prefixcmp(url, prefix_ssh)) { url = url + strlen(prefix_ssh); if (gitno_extract_url_parts(&host, &port, &user, &pass, url, default_port) < 0) @@ -338,10 +336,10 @@ static int _git_ssh_setup_conn( port = git__strdup(default_port); GITERR_CHECK_ALLOC(port); } - + if (gitno_connect(&s->socket, host, port, 0) < 0) goto on_error; - + if (user && pass) { if (git_cred_userpass_plaintext_new(&t->cred, user, pass) < 0) goto on_error; @@ -357,11 +355,11 @@ static int _git_ssh_setup_conn( goto on_error; } assert(t->cred); - + if (!user) { user = git__strdup(default_user); } - + if (_git_ssh_session_create(&session, s->socket) < 0) { giterr_set(GITERR_NET, "Failed to initialize SSH session"); goto on_error; @@ -379,10 +377,10 @@ static int _git_ssh_setup_conn( } libssh2_channel_set_blocking(channel, 1); - + s->session = session; s->channel = channel; - + t->current_stream = s; git__free(host); git__free(port); @@ -390,11 +388,15 @@ static int _git_ssh_setup_conn( git__free(pass); return 0; - + on_error: + s->session = NULL; + s->channel = NULL; + t->current_stream = NULL; + if (*stream) ssh_stream_free(*stream); - + git__free(host); git__free(port); git__free(user); @@ -413,7 +415,7 @@ static int ssh_uploadpack_ls( { if (_git_ssh_setup_conn(t, url, cmd_uploadpack, stream) < 0) return -1; - + return 0; } @@ -423,12 +425,12 @@ static int ssh_uploadpack( git_smart_subtransport_stream **stream) { GIT_UNUSED(url); - + if (t->current_stream) { *stream = &t->current_stream->parent; return 0; } - + giterr_set(GITERR_NET, "Must call UPLOADPACK_LS before UPLOADPACK"); return -1; } @@ -440,7 +442,7 @@ static int ssh_receivepack_ls( { if (_git_ssh_setup_conn(t, url, cmd_receivepack, stream) < 0) return -1; - + return 0; } @@ -450,12 +452,12 @@ static int ssh_receivepack( git_smart_subtransport_stream **stream) { GIT_UNUSED(url); - + if (t->current_stream) { *stream = &t->current_stream->parent; return 0; } - + giterr_set(GITERR_NET, "Must call RECEIVEPACK_LS before RECEIVEPACK"); return -1; } @@ -467,21 +469,21 @@ static int _ssh_action( git_smart_service_t action) { ssh_subtransport *t = (ssh_subtransport *) subtransport; - + switch (action) { case GIT_SERVICE_UPLOADPACK_LS: return ssh_uploadpack_ls(t, url, stream); - + case GIT_SERVICE_UPLOADPACK: return ssh_uploadpack(t, url, stream); - + case GIT_SERVICE_RECEIVEPACK_LS: return ssh_receivepack_ls(t, url, stream); - + case GIT_SERVICE_RECEIVEPACK: return ssh_receivepack(t, url, stream); } - + *stream = NULL; return -1; } @@ -489,38 +491,38 @@ static int _ssh_action( static int _ssh_close(git_smart_subtransport *subtransport) { ssh_subtransport *t = (ssh_subtransport *) subtransport; - + assert(!t->current_stream); - + GIT_UNUSED(t); - + return 0; } static void _ssh_free(git_smart_subtransport *subtransport) { ssh_subtransport *t = (ssh_subtransport *) subtransport; - + assert(!t->current_stream); - + git__free(t); } int git_smart_subtransport_ssh(git_smart_subtransport **out, git_transport *owner) { ssh_subtransport *t; - + if (!out) return -1; - + t = git__calloc(sizeof(ssh_subtransport), 1); GITERR_CHECK_ALLOC(t); - + t->owner = (transport_smart *)owner; t->parent.action = _ssh_action; t->parent.close = _ssh_close; t->parent.free = _ssh_free; - + *out = (git_smart_subtransport *) t; return 0; } From c2de6b1adf49f8588dd59188774c96b783181e2f Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 10 Jul 2013 10:21:24 -0700 Subject: [PATCH 5/6] Bring SSH error reporting up to base standards The SSH error checking and reporting could still be further improved by using the libssh2 native methods to get error info, but at least this ensures that all error codes are checked and translated into libgit2 error messages. --- src/transports/cred.c | 3 +- src/transports/ssh.c | 151 ++++++++++++++++++++---------------------- 2 files changed, 73 insertions(+), 81 deletions(-) diff --git a/src/transports/cred.c b/src/transports/cred.c index d713f8992..a6727e902 100644 --- a/src/transports/cred.c +++ b/src/transports/cred.c @@ -33,8 +33,7 @@ int git_cred_userpass_plaintext_new( { git_cred_userpass_plaintext *c; - if (!cred) - return -1; + assert(cred); c = git__malloc(sizeof(git_cred_userpass_plaintext)); GITERR_CHECK_ALLOC(c); diff --git a/src/transports/ssh.c b/src/transports/ssh.c index 0155dd745..7fb53bc3c 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -55,6 +55,7 @@ static int gen_proto(git_buf *request, const char *cmd, const char *url) } if (!repo) { + giterr_set(GITERR_NET, "Malformed git protocol URL"); return -1; } @@ -79,13 +80,11 @@ static int send_command(ssh_stream *s) if (error < 0) goto cleanup; - error = libssh2_channel_exec( - s->channel, - request.ptr - ); - - if (0 != error) + error = libssh2_channel_exec(s->channel, request.ptr); + if (error < 0) { + giterr_set(GITERR_NET, "SSH could not execute request"); goto cleanup; + } s->sent_command = 1; @@ -100,6 +99,7 @@ static int ssh_stream_read( size_t buf_size, size_t *bytes_read) { + int rc; ssh_stream *s = (ssh_stream *)stream; *bytes_read = 0; @@ -107,9 +107,10 @@ static int ssh_stream_read( if (!s->sent_command && send_command(s) < 0) return -1; - int rc = libssh2_channel_read(s->channel, buffer, buf_size); - if (rc < 0) + if ((rc = libssh2_channel_read(s->channel, buffer, buf_size)) < 0) { + giterr_set(GITERR_NET, "SSH could not read data"); return -1; + } *bytes_read = rc; @@ -126,12 +127,12 @@ static int ssh_stream_write( if (!s->sent_command && send_command(s) < 0) return -1; - int rc = libssh2_channel_write(s->channel, buffer, len); - if (rc < 0) { + if (libssh2_channel_write(s->channel, buffer, len) < 0) { + giterr_set(GITERR_NET, "SSH could not write data"); return -1; } - return rc; + return 0; } static void ssh_stream_free(git_smart_subtransport_stream *stream) @@ -151,12 +152,13 @@ static void ssh_stream_free(git_smart_subtransport_stream *stream) } if (s->session) { - libssh2_session_free(s->session), s->session = NULL; + libssh2_session_free(s->session); + s->session = NULL; } if (s->socket.socket) { - ret = gitno_close(&s->socket); - assert(!ret); + (void)gitno_close(&s->socket); + /* can't do anything here with error return value */ } git__free(s->url); @@ -171,8 +173,7 @@ static int ssh_stream_alloc( { ssh_stream *s; - if (!stream) - return -1; + assert(stream); s = git__calloc(sizeof(ssh_stream), 1); GITERR_CHECK_ALLOC(s); @@ -183,8 +184,8 @@ static int ssh_stream_alloc( s->parent.free = ssh_stream_free; s->cmd = cmd; - s->url = git__strdup(url); + s->url = git__strdup(url); if (!s->url) { git__free(s); return -1; @@ -217,8 +218,10 @@ static int git_ssh_extract_url_parts( start = url; *username = git__strdup(default_user); } + GITERR_CHECK_ALLOC(*username); *host = git__substrdup(start, colon - start); + GITERR_CHECK_ALLOC(*host); return 0; } @@ -229,67 +232,63 @@ static int _git_ssh_authenticate_session( git_cred* cred) { int rc; + do { switch (cred->credtype) { - case GIT_CREDTYPE_USERPASS_PLAINTEXT: { - git_cred_userpass_plaintext *c = (git_cred_userpass_plaintext *)cred; - rc = libssh2_userauth_password( - session, - c->username, - c->password - ); - break; - } - case GIT_CREDTYPE_SSH_KEYFILE_PASSPHRASE: { - git_cred_ssh_keyfile_passphrase *c = (git_cred_ssh_keyfile_passphrase *)cred; - rc = libssh2_userauth_publickey_fromfile( - session, - user, - c->publickey, - c->privatekey, - c->passphrase - ); - break; - } - case GIT_CREDTYPE_SSH_PUBLICKEY: { - git_cred_ssh_publickey *c = (git_cred_ssh_publickey *)cred; - rc = libssh2_userauth_publickey( - session, - user, - (const unsigned char *)c->publickey, - c->publickey_len, - c->sign_callback, - &c->sign_data - ); - break; - } - default: - rc = LIBSSH2_ERROR_AUTHENTICATION_FAILED; + case GIT_CREDTYPE_USERPASS_PLAINTEXT: { + git_cred_userpass_plaintext *c = (git_cred_userpass_plaintext *)cred; + rc = libssh2_userauth_password(session, c->username, c->password); + break; + } + case GIT_CREDTYPE_SSH_KEYFILE_PASSPHRASE: { + git_cred_ssh_keyfile_passphrase *c = (git_cred_ssh_keyfile_passphrase *)cred; + rc = libssh2_userauth_publickey_fromfile( + session, user, c->publickey, c->privatekey, c->passphrase); + break; + } + case GIT_CREDTYPE_SSH_PUBLICKEY: { + git_cred_ssh_publickey *c = (git_cred_ssh_publickey *)cred; + rc = libssh2_userauth_publickey( + session, user, (const unsigned char *)c->publickey, + c->publickey_len, c->sign_callback, &c->sign_data); + break; + } + default: + rc = LIBSSH2_ERROR_AUTHENTICATION_FAILED; } } while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc); - return rc; + if (rc != 0) { + giterr_set(GITERR_NET, "Failed to authenticate SSH session"); + return -1; + } + + return 0; } static int _git_ssh_session_create( LIBSSH2_SESSION** session, gitno_socket socket) { - if (!session) { + int rc = 0; + LIBSSH2_SESSION* s; + + assert(session); + + s = libssh2_session_init(); + if (!s) { + giterr_set(GITERR_NET, "Failed to initialize SSH session"); return -1; } - LIBSSH2_SESSION* s = libssh2_session_init(); - if (!s) - return -1; - - int rc = 0; do { rc = libssh2_session_startup(s, socket.socket); } while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc); if (0 != rc) { - goto on_error; + libssh2_session_free(s); + giterr_set(GITERR_NET, "Failed to start SSH session"); + return -1; } libssh2_session_set_blocking(s, 1); @@ -297,21 +296,13 @@ static int _git_ssh_session_create( *session = s; return 0; - -on_error: - if (s) { - libssh2_session_free(s), s = NULL; - } - - return -1; } static int _git_ssh_setup_conn( ssh_subtransport *t, const char *url, const char *cmd, - git_smart_subtransport_stream **stream -) + git_smart_subtransport_stream **stream) { char *host, *port=NULL, *user=NULL, *pass=NULL; const char *default_port="22"; @@ -343,12 +334,17 @@ static int _git_ssh_setup_conn( if (git_cred_userpass_plaintext_new(&t->cred, user, pass) < 0) goto on_error; } else if (t->owner->cred_acquire_cb) { - if (t->owner->cred_acquire_cb(&t->cred, - t->owner->url, - user, - GIT_CREDTYPE_USERPASS_PLAINTEXT | GIT_CREDTYPE_SSH_KEYFILE_PASSPHRASE, + if (t->owner->cred_acquire_cb( + &t->cred, t->owner->url, user, + GIT_CREDTYPE_USERPASS_PLAINTEXT | + GIT_CREDTYPE_SSH_KEYFILE_PASSPHRASE, t->owner->cred_acquire_payload) < 0) goto on_error; + + if (!t->cred) { + giterr_set(GITERR_NET, "Callback failed to initialize SSH credentials"); + goto on_error; + } } else { giterr_set(GITERR_NET, "Cannot set up SSH connection without credentials"); goto on_error; @@ -357,17 +353,14 @@ static int _git_ssh_setup_conn( if (!user) { user = git__strdup(default_user); + GITERR_CHECK_ALLOC(user); } - if (_git_ssh_session_create(&session, s->socket) < 0) { - giterr_set(GITERR_NET, "Failed to initialize SSH session"); + if (_git_ssh_session_create(&session, s->socket) < 0) goto on_error; - } - if (_git_ssh_authenticate_session(session, user, t->cred) < 0) { - giterr_set(GITERR_NET, "Failed to authenticate SSH session"); + if (_git_ssh_authenticate_session(session, user, t->cred) < 0) goto on_error; - } channel = libssh2_channel_open_session(session); if (!channel) { @@ -402,7 +395,7 @@ on_error: git__free(pass); if (session) - libssh2_session_free(session), session = NULL; + libssh2_session_free(session); return -1; } From 33c8c6f0b81badadf805d18f39f79384a6494bac Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 10 Jul 2013 10:48:32 -0700 Subject: [PATCH 6/6] trivial whitespace fixup --- src/transports/smart_protocol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transports/smart_protocol.c b/src/transports/smart_protocol.c index 636616717..0cd5e831d 100644 --- a/src/transports/smart_protocol.c +++ b/src/transports/smart_protocol.c @@ -372,7 +372,7 @@ int git_smart__negotiate_fetch(git_transport *transport, git_repository *repo, c return error; if (pkt->type == GIT_PKT_NAK || - (pkt->type == GIT_PKT_ACK && pkt->status != GIT_ACK_CONTINUE)) { + (pkt->type == GIT_PKT_ACK && pkt->status != GIT_ACK_CONTINUE)) { git__free(pkt); break; }