Re: [PATCH] drop unnecessary copying in credential_ask_one

2014-01-07 Thread Jeff King 
On Thu, Jan 02, 2014 at 11:08:51AM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  ... But the test suite, of course, always uses askpass because it
  cannot rely on accessing a terminal (we'd have to do some magic with
  lib-terminal, I think).
 
  So it doesn't detect the problem in your patch, but I wonder if it is
  worth applying the patch below anyway, as it makes the test suite
  slightly more robust.
 
 Sounds like a good first step in the right direction.  Thanks.

I took a brief look at adding real terminal tests for the credential
code using our test-terminal/lib-terminal.sh setup. Unfortunately, it
falls short of what we need.

test-terminal only handles stdout and stderr streams as fake terminals.
We could pretty easily add stdin for input, as it uses fork() to work
asynchronously.  But the credential code does not actually read from
stdin. It opens and reads from /dev/tty explicitly. So I think we'd have
to actually fake setting up a controlling terminal. And that means magic
with setsid() and ioctl(TIOCSCTTY), which in turn sounds like a
portability headache.

So it's definitely possible under Linux, and probably under most Unixes.
But I'm not sure it's worth the effort, given that review already caught
the potential bug here.

Another option would be to instrument git_terminal_prompt with a
mock-terminal interface (say, reading from a file specified in an
environment variable). But I really hate polluting the code with test
cruft, and it would not actually be testing an interesting segment of
the code, anyway.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drop unnecessary copying in credential_ask_one

2014-01-07 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 On Thu, Jan 02, 2014 at 11:08:51AM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  ... But the test suite, of course, always uses askpass because it
  cannot rely on accessing a terminal (we'd have to do some magic with
  lib-terminal, I think).
 
  So it doesn't detect the problem in your patch, but I wonder if it is
  worth applying the patch below anyway, as it makes the test suite
  slightly more robust.
 
 Sounds like a good first step in the right direction.  Thanks.

 I took a brief look at adding real terminal tests for the credential
 code using our test-terminal/lib-terminal.sh setup. Unfortunately, it
 falls short of what we need.

 test-terminal only handles stdout and stderr streams as fake terminals.
 We could pretty easily add stdin for input, as it uses fork() to work
 asynchronously.  But the credential code does not actually read from
 stdin. It opens and reads from /dev/tty explicitly. So I think we'd have
 to actually fake setting up a controlling terminal. And that means magic
 with setsid() and ioctl(TIOCSCTTY), which in turn sounds like a
 portability headache.

I wonder if expect has already solved that for us.

 So it's definitely possible under Linux, and probably under most Unixes.
 But I'm not sure it's worth the effort, given that review already caught
 the potential bug here.

 Another option would be to instrument git_terminal_prompt with a
 mock-terminal interface (say, reading from a file specified in an
 environment variable). But I really hate polluting the code with test
 cruft, and it would not actually be testing an interesting segment of
 the code, anyway.

Agreed.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drop unnecessary copying in credential_ask_one

2014-01-07 Thread Jeff King 
On Tue, Jan 07, 2014 at 11:44:00AM -0800, Junio C Hamano wrote:

  test-terminal only handles stdout and stderr streams as fake terminals.
  We could pretty easily add stdin for input, as it uses fork() to work
  asynchronously.  But the credential code does not actually read from
  stdin. It opens and reads from /dev/tty explicitly. So I think we'd have
  to actually fake setting up a controlling terminal. And that means magic
  with setsid() and ioctl(TIOCSCTTY), which in turn sounds like a
  portability headache.
 
 I wonder if expect has already solved that for us.

I would not be surprised if it did. Though it introduces its own
portability issues, since we cannot depend on having it. But it is
probably enough to just

  test_lazy_prereq EXPECT 'expect --version'

or something. I dunno. I have never used expect, do not have it
installed, and am not excited about introducing a new tool dependency.
But if you want to explore it, be my guest.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drop unnecessary copying in credential_ask_one

2014-01-02 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 ... But the test suite, of course, always uses askpass because it
 cannot rely on accessing a terminal (we'd have to do some magic with
 lib-terminal, I think).

 So it doesn't detect the problem in your patch, but I wonder if it is
 worth applying the patch below anyway, as it makes the test suite
 slightly more robust.

Sounds like a good first step in the right direction.  Thanks.


 -- 8 --
 Subject: use distinct username/password for http auth tests

 The httpd server we set up to test git's http client code
 knows about a single account, in which both the username and
 password are user@host (the unusual use of the @ here is
 to verify that we handle the character correctly when URL
 escaped).

 This means that we may miss a certain class of errors in
 which the username and password are mixed up internally by
 git. We can make our tests more robust by having distinct
 values for the username and password.

 In addition to tweaking the server passwd file and the
 client URL, we must teach the askpass harness to accept
 multiple values. As a bonus, this makes the setup of some
 tests more obvious; when we are expecting git to ask
 only about the password, we can seed the username askpass
 response with a bogus value.

 Signed-off-by: Jeff King p...@peff.net
 ---
  t/lib-httpd.sh| 15 ---
  t/lib-httpd/passwd|  2 +-
  t/t5540-http-push.sh  |  4 ++--
  t/t5541-http-push.sh  |  6 +++---
  t/t5550-http-fetch.sh | 10 +-
  t/t5551-http-fetch.sh |  6 +++---
  6 files changed, 26 insertions(+), 17 deletions(-)

 diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
 index c470784..bfdff2a 100644
 --- a/t/lib-httpd.sh
 +++ b/t/lib-httpd.sh
 @@ -129,7 +129,7 @@ prepare_httpd() {
   HTTPD_DEST=127.0.0.1:$LIB_HTTPD_PORT
   HTTPD_URL=$HTTPD_PROTO://$HTTPD_DEST
   HTTPD_URL_USER=$HTTPD_PROTO://user%40host@$HTTPD_DEST
 - HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:user%40host@$HTTPD_DEST
 + HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:pass%40host@$HTTPD_DEST
  
   if test -n $LIB_HTTPD_DAV -o -n $LIB_HTTPD_SVN
   then
 @@ -217,7 +217,15 @@ setup_askpass_helper() {
   test_expect_success 'setup askpass helper' '
   write_script $TRASH_DIRECTORY/askpass -\EOF 
   echo $TRASH_DIRECTORY/askpass-query askpass: $* 
 - cat $TRASH_DIRECTORY/askpass-response
 + case $* in
 + *Username*)
 + what=user
 + ;;
 + *Password*)
 + what=pass
 + ;;
 + esac 
 + cat $TRASH_DIRECTORY/askpass-$what
   EOF
   GIT_ASKPASS=$TRASH_DIRECTORY/askpass 
   export GIT_ASKPASS 
 @@ -227,7 +235,8 @@ setup_askpass_helper() {
  
  set_askpass() {
   $TRASH_DIRECTORY/askpass-query 
 - echo $* $TRASH_DIRECTORY/askpass-response
 + echo $1 $TRASH_DIRECTORY/askpass-user 
 + echo $2 $TRASH_DIRECTORY/askpass-pass
  }
  
  expect_askpass() {
 diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
 index f2fbcad..99a34d6 100644
 --- a/t/lib-httpd/passwd
 +++ b/t/lib-httpd/passwd
 @@ -1 +1 @@
 -user@host:nKpa8pZUHx/ic
 +user@host:xb4E8pqD81KQs
 diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
 index 01d0d95..5b0198c 100755
 --- a/t/t5540-http-push.sh
 +++ b/t/t5540-http-push.sh
 @@ -154,7 +154,7 @@ test_http_push_nonff 
 $HTTPD_DOCUMENT_ROOT_PATH/test_repo.git \
  
  test_expect_success 'push to password-protected repository (user in URL)' '
   test_commit pw-user 
 - set_askpass user@host 
 + set_askpass user@host pass@host 
   git push $HTTPD_URL_USER/auth/dumb/test_repo.git HEAD 
   git rev-parse --verify HEAD expect 
   git --git-dir=$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/test_repo.git \
 @@ -168,7 +168,7 @@ test_expect_failure 'user was prompted only once for 
 password' '
  
  test_expect_failure 'push to password-protected repository (no user in URL)' 
 '
   test_commit pw-nouser 
 - set_askpass user@host 
 + set_askpass user@host pass@host 
   git push $HTTPD_URL/auth/dumb/test_repo.git HEAD 
   expect_askpass both user@host
   git rev-parse --verify HEAD expect 
 diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
 index 470ac54..bfd241e 100755
 --- a/t/t5541-http-push.sh
 +++ b/t/t5541-http-push.sh
 @@ -274,7 +274,7 @@ test_expect_success 'push over smart http with auth' '
   cd $ROOT_PATH/test_repo_clone 
   echo push-auth-test expect 
   test_commit push-auth-test 
 - set_askpass user@host 
 + set_askpass user@host pass@host 
   git push $HTTPD_URL/auth/smart/test_repo.git 
   git --git-dir=$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git \
   log -1 --format=%s actual 
 @@ -286,7 +286,7 @@ test_expect_success 'push to auth-only-for-push repo' '
   cd $ROOT_PATH/test_repo_clone 
   echo push-half-auth expect 
   test_commit

Re: [PATCH] drop unnecessary copying in credential_ask_one

2014-01-01 Thread Jeff King 
On Thu, Jan 02, 2014 at 09:06:33AM +0800, Tay Ray Chuan wrote:

 We were leaking memory in there, as after obtaining a string from
 git_getpass, we returned a copy of it, yet no one else held the original
 string, apart from credential_ask_one.

I don't think this change is correct by itself.

credential_ask_one calls git_prompt. That function in turn calls
git_terminal_prompt, which returns a pointer to a static buffer (because
it may be backed by the system getpass() implementation).

So there is no leak there, and dropping the strdup would be bad (the
call to ask for the password would overwrite the value we got for the
username).

However, git_prompt may also call do_askpass if GIT_ASKPASS is set, and
here there is a leak, as we duplicate the buffer.  To stop the leak, we
need to first harmonize the do_askpass and git_terminal_prompt code
paths to either both allocate, or both return a static buffer (and then
either strdup or not in the caller, depending on which way we go).

It looks like what I originally wrote was correct, as both code paths
matched.  But then I stupidly broke it with 31b49d9, which failed to
notice the static specifier on the strbuf in do_askpass, and started
using strbuf_detach.

I think this is the simplest fix:

-- 8 --
Subject: Revert prompt: clean up strbuf usage

This reverts commit 31b49d9b653803e7c7fd18b21c8bdd86e3421668.

That commit taught do_askpass to hand ownership of our
buffer back to the caller rather than simply return a
pointer into our internal strbuf.  What it failed to notice,
though, was that our internal strbuf is static, because we
are trying to emulate the getpass() interface.

By handing off ownership, we created a memory leak that
cannot be solved. Sometimes git_prompt returns a static
buffer from getpass() (or our smarter git_terminal_prompt
wrapper), and sometimes it returns an allocated string from
do_askpass.

Signed-off-by: Jeff King p...@peff.net
---
 prompt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/prompt.c b/prompt.c
index d851807..d7bb17c 100644
--- a/prompt.c
+++ b/prompt.c
@@ -22,6 +22,7 @@ static char *do_askpass(const char *cmd, const char *prompt)
if (start_command(pass))
return NULL;
 
+   strbuf_reset(buffer);
if (strbuf_read(buffer, pass.out, 20)  0)
err = 1;
 
@@ -38,7 +39,7 @@ static char *do_askpass(const char *cmd, const char *prompt)
 
strbuf_setlen(buffer, strcspn(buffer.buf, \r\n));
 
-   return strbuf_detach(buffer, NULL);
+   return buffer.buf;
 }
 
 char *git_prompt(const char *prompt, int flags)
-- 
1.8.5.2.434.g63b1477






 
 Signed-off-by: Tay Ray Chuan rcta...@gmail.com
 ---
  credential.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/credential.c b/credential.c
 index 86397f3..0d02ad8 100644
 --- a/credential.c
 +++ b/credential.c
 @@ -54,7 +54,7 @@ static char *credential_ask_one(const char *what, struct 
 credential *c)
  
   strbuf_release(desc);
   strbuf_release(prompt);
 - return xstrdup(r);
 + return r;
  }
  
  static void credential_getpass(struct credential *c)
 -- 
 1.8.5-rc2
 
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drop unnecessary copying in credential_ask_one

2014-01-01 Thread Jeff King 
On Wed, Jan 01, 2014 at 10:03:30PM -0500, Jeff King wrote:

 On Thu, Jan 02, 2014 at 09:06:33AM +0800, Tay Ray Chuan wrote:
 
  We were leaking memory in there, as after obtaining a string from
  git_getpass, we returned a copy of it, yet no one else held the original
  string, apart from credential_ask_one.
 
 I don't think this change is correct by itself.
 
 credential_ask_one calls git_prompt. That function in turn calls
 git_terminal_prompt, which returns a pointer to a static buffer (because
 it may be backed by the system getpass() implementation).
 
 So there is no leak there, and dropping the strdup would be bad (the
 call to ask for the password would overwrite the value we got for the
 username).

By the way, you can see the breakage from your patch pretty easily by
testing the terminal input. Disable any credential helper config you
have, and then run:

  GIT_CURL_VERBOSE=1 \
  git ls-remote https://github.com/peff/ask-for-auth 21 |
  perl -lne '/Authorization: Basic (.*)/ and print $1' |
  openssl base64 -d

enter myuser and mypass respectively on the terminal. The result is
that we send mypass:mypass to the server. And then double-free the
result, which cases glibc to barf.

I wondered why we did not see this breakage in test suite. My assumption
was that it was simply because our test user has the same username and
password. So I fixed that, but to my surprise we still did not detect
the problem. The issue is that your patch does the right thing when
GIT_ASKPASS is in use, and breaks only when the user types into the
terminal. But the test suite, of course, always uses askpass because it
cannot rely on accessing a terminal (we'd have to do some magic with
lib-terminal, I think).

So it doesn't detect the problem in your patch, but I wonder if it is
worth applying the patch below anyway, as it makes the test suite
slightly more robust.

-- 8 --
Subject: use distinct username/password for http auth tests

The httpd server we set up to test git's http client code
knows about a single account, in which both the username and
password are user@host (the unusual use of the @ here is
to verify that we handle the character correctly when URL
escaped).

This means that we may miss a certain class of errors in
which the username and password are mixed up internally by
git. We can make our tests more robust by having distinct
values for the username and password.

In addition to tweaking the server passwd file and the
client URL, we must teach the askpass harness to accept
multiple values. As a bonus, this makes the setup of some
tests more obvious; when we are expecting git to ask
only about the password, we can seed the username askpass
response with a bogus value.

Signed-off-by: Jeff King p...@peff.net
---
 t/lib-httpd.sh| 15 ---
 t/lib-httpd/passwd|  2 +-
 t/t5540-http-push.sh  |  4 ++--
 t/t5541-http-push.sh  |  6 +++---
 t/t5550-http-fetch.sh | 10 +-
 t/t5551-http-fetch.sh |  6 +++---
 6 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index c470784..bfdff2a 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -129,7 +129,7 @@ prepare_httpd() {
HTTPD_DEST=127.0.0.1:$LIB_HTTPD_PORT
HTTPD_URL=$HTTPD_PROTO://$HTTPD_DEST
HTTPD_URL_USER=$HTTPD_PROTO://user%40host@$HTTPD_DEST
-   HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:user%40host@$HTTPD_DEST
+   HTTPD_URL_USER_PASS=$HTTPD_PROTO://user%40host:pass%40host@$HTTPD_DEST
 
if test -n $LIB_HTTPD_DAV -o -n $LIB_HTTPD_SVN
then
@@ -217,7 +217,15 @@ setup_askpass_helper() {
test_expect_success 'setup askpass helper' '
write_script $TRASH_DIRECTORY/askpass -\EOF 
echo $TRASH_DIRECTORY/askpass-query askpass: $* 
-   cat $TRASH_DIRECTORY/askpass-response
+   case $* in
+   *Username*)
+   what=user
+   ;;
+   *Password*)
+   what=pass
+   ;;
+   esac 
+   cat $TRASH_DIRECTORY/askpass-$what
EOF
GIT_ASKPASS=$TRASH_DIRECTORY/askpass 
export GIT_ASKPASS 
@@ -227,7 +235,8 @@ setup_askpass_helper() {
 
 set_askpass() {
$TRASH_DIRECTORY/askpass-query 
-   echo $* $TRASH_DIRECTORY/askpass-response
+   echo $1 $TRASH_DIRECTORY/askpass-user 
+   echo $2 $TRASH_DIRECTORY/askpass-pass
 }
 
 expect_askpass() {
diff --git a/t/lib-httpd/passwd b/t/lib-httpd/passwd
index f2fbcad..99a34d6 100644
--- a/t/lib-httpd/passwd
+++ b/t/lib-httpd/passwd
@@ -1 +1 @@
-user@host:nKpa8pZUHx/ic
+user@host:xb4E8pqD81KQs
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 01d0d95..5b0198c 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -154,7 +154,7 @@ test_http_push_nonff 
$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git \
 
 test_expect_success 'push to password-protected repository (user in