Re: [PATCH 2/5] safe_create_leading_directories(): reduce scope of local variable

[Michael Haggerty]

On 12/26/2013 10:55 PM, Jonathan Nieder wrote:
 Michael Haggerty wrote:
 
 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
  sha1_file.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

 diff --git a/sha1_file.c b/sha1_file.c
 index c9245a6..cc9957e 100644
 --- a/sha1_file.c
 +++ b/sha1_file.c
 @@ -108,9 +108,10 @@ int mkdir_in_gitdir(const char *path)
  int safe_create_leading_directories(char *path)
  {
  char *pos = path + offset_1st_component(path);
 -struct stat st;
  
  while (pos) {
 +struct stat st;
 
 Is this to make it easier to reason about whether 'st' has been
 properly initialized at any given moment, or is there a more subtle
 reason?

No, just the boring reason, the one that makes me reduce the scope of
variables whenever possible.  I'll buff up the log message.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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 3/5] safe_create_leading_directories(): add slash pointer

[Michael Haggerty]

On 12/26/2013 11:34 PM, Jonathan Nieder wrote:
 Michael Haggerty wrote:
 
 [Subject: safe_create_leading_directories(): add slash pointer]
 
 Is this a cleanup or improving the (internal) functionality of the
 function somehow?  The above one-liner doesn't sum up for me in an
 obvious way why this is a good change.

It's hard to make the subject more self-explanatory, given so few
characters.  But I will make the rest of the log message better in the
reroll.

 Keep track of the position of the slash character separately, and
 
 Separately from what?
 
 restore the slash character at a single place, at the end of the while
 loop.  This makes the next change easier to implement.

 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 
 Ah, do I understand correctly that this is about cleaning up
 after the code that scribbles over 'path' in one place, to make
 it harder to forget to do that cleanup as new code paths are
 introduced?

Yes.

 It's too bad there's no variant of 'stat' and 'mkdir' that takes
 a (buf, len) pair which would avoid the scribbling altogether.

Yes.

 ---
  sha1_file.c | 36 ++--
  1 file changed, 18 insertions(+), 18 deletions(-)

 diff --git a/sha1_file.c b/sha1_file.c
 index cc9957e..dcfd35a 100644
 --- a/sha1_file.c
 +++ b/sha1_file.c
 @@ -107,40 +107,40 @@ int mkdir_in_gitdir(const char *path)
  
  int safe_create_leading_directories(char *path)
  {
 -char *pos = path + offset_1st_component(path);
 +char *next_component = path + offset_1st_component(path);
 
 This name change is probably worth also mentioning in the commit
 message (or lifting into a separate patch) so the reader doesn't get
 distracted.

OK, I split the renaming into a separate commit.

 +int retval = 0;
  
 -while (pos) {
 +while (!retval  next_component) {
 
 A more usual style would be
 [...]
 Using retval for control flow instead makes it eight lines more
 concise, which is probably worth it.

Agreed.

 [...]
  if (!S_ISDIR(st.st_mode)) {
 -*pos = '/';
 -return -3;
 +retval = -3;
  }
 
 Now the 'if' body is one line, so we can drop the braces and save
 another line. :)

Will fix.

 One more nit: elsewhere in this file, a variable keeping track of the
 return value is named 'ret', so it probably makes sense to also use
 that name here.

OK, will change.

 That would mean the following changes to be potentially squashed in
 [...]

While going over the code again, I noticed another problem in the
original version; namely, that the handling of redundant multiple
slashes in the input path is not correct.  I will fix this problem and
split up the commit into smaller steps in the re-roll.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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 4/5] safe_create_leading_directories(): fix a mkdir/rmdir race

[Michael Haggerty]

On 12/27/2013 12:02 AM, Jonathan Nieder wrote:
 Michael Haggerty wrote:
 
 It could be that some other process is trying to clean up empty
 directories at the same time that safe_create_leading_directories() is
 attempting to create them.  In this case, it could happen that
 directory a/b was present at the end of one iteration of the loop
 (either it was already present or we just created it ourselves), but
 by the time we try to create directory a/b/c, directory a/b has
 been deleted.  In fact, directory a might also have been deleted.
 
 When does this happen in practice?  Is this about git racing with
 itself or with some other program?

I think it could be triggered by a reference creation racing with a
reference packing.  See below.

 I fear that the aggressive directory creator fighting the aggressive
 directory remover might be waging a losing battle.

That may be so, but it would be strange for a directory remover to be
aggressive.  And even if it were, the worst consequence would be that
the director creator would try three times before giving up.

 Is this about a push that creates a ref racing against a push that
 deletes a ref from the same hierarchy?

The race could be triggered in this scenario but I don't think it would
result in a spurious error (at least not if there are only two
racers...)  The reason is that empty loose reference directories are not
deleted when a reference is *deleted*, but rather when a new
d/f-conflicting reference is *created*.  E.g., if

git branch foo/bar
git branch -d foo/bar # this leaves directory foo behind
# this removes directory foo and creates file foo:
git branch foo 
git branch foo/baz

The last two commands could fight.  However, in this scenario one of the
reference creations would ultimately have to fail anyway, so this patch
doesn't really help.

However, when packing references, the directories that used to hold the
old references are deleted right away.  So

git branch foo/bar
git pack-refs --all 
git branch foo/baz

Here, the last two commands could fight.

 So, if a call to mkdir() fails with ENOENT, then try checking/making
 all directories again from the beginning.  Attempt up to three times
 before giving up.
 
 If we are racing against a ref deletion, then we can retry while our
 rival keeps walking up the directory tree and deleting parent
 directories.  As soon as we successfully create a directory, we have
 won the race.
 
 But what happens if the entire safe_create_leading_directories
 operation succeeds and *then* our racing partner deletes the
 directory?  No one is putting in a file to reserve the directory for
 the directory creator.

 If we care enough to retry more than once, I fear this is retrying at
 the wrong level.

I realize that this change doesn't solve the whole problem.  But you
make a good point, that if the caller is going to retry anyway, then
there is no need to retry within this function.  It would be sufficient
for this function to return a specific error value indicating that
creating the directory failed, but there's a chance of success if you
try again.

On the other hand, your argument assumes that all callers really *do*
retry, which isn't the case, and I doubt that all callers are going to
be fixed.  So there might be some value in retrying within this function
anyway (it's a game of averages we're playing here anyway).

I'll think some more about it.

 Tests?

I can't think of how to test this short of either instrumenting the code
(which I did for my own tests, but didn't include the test code in this
submission) or running the test within some kind of malicious virtual
filesystem.  Ideas?

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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

[PATCH] drop unnecessary copying in credential_ask_one

[Tay Ray Chuan]

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.

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

[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

[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