Re: t0300-credentials: poll failed: invalid argument

[Jeff King <peff@peff.net>]

On Sun, Aug 28, 2011 at 12:40:56AM -0400, Brian Gernhardt wrote:

> The only usage of poll I see in the credentials system is:
> 
> credentials-cache--daemon.c
> 177:	if (poll(&pfd, 1, 1000 * wakeup) < 0) {
> 
> My guess is that (1000 * wakeup) is more than INT_MAX and is becoming
> negative as the man page for poll seems to indicate that it will fail
> if timeout < -1.
> 
> Does anyone familiar with the credentials daemon want to try to figure
> out a reasonable fix?

Ugh, sorry, this is my fault. The check_expiration() function can return
a totally bogus value before we actually get any credentials.

Does this patch fix it for you?

-- >8 --
Subject: [PATCH] credential-cache: fix expiration calculation corner cases

The main credential-cache daemon loop calls poll to wait for
a client or to trigger the expiration of credentials. When
the last credential we hold expires, we exit.

However, there is a corner case: when we first start up, we
have no credentials, and are waiting for a client to
provide us with one. In this case, we ended up handing
complete junk for the timeout argument to poll(). On some
systems, this caused us to just wait a long time for the
client (which usually showed up within a second or so). On
OS X, however, the system quite reasonably complained about
our junk value with EINVAL.

Fixing this is pretty straightforward; we just notice that
we have no entries to compare against. However, that bug was
covering up another one: our expiration calculation didn't
give clients a chance to actually connect and provide us
with a credential before we decided that we should exit
because we weren't holding any credentials!

The new algorithm is:

  1. Sleep until it's time to expire the most recent
     credential.

  2. If we don't have any credentials yet, wait 30 seconds
     for a client to contact us and give us one.

  3. After expiring the last credential, wait 30 seconds for
     a client to provide us with one.

Technically only parts (1) and (2) are needed to implement
the original intended behavior.

But (3) is a minor optimization that is made easy by the new
code. When a client gives us a credential, then removes it
(e.g., because it had a bogus password), and then gives us
another one, we used to exit, forcing the client to start a
new daemon instance. Instead, we can just reuse the existing
daemon instance.

Signed-off-by: Jeff King <peff@peff.net>
---
 credential-cache--daemon.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index f520347..d6769b1 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -57,20 +57,33 @@ static void remove_credential(const struct credential *c)
 
 static int check_expirations(void)
 {
+	static unsigned long wait_for_entry_until;
 	int i = 0;
 	unsigned long now = time(NULL);
 	unsigned long next = (unsigned long)-1;
 
+	/*
+	 * Initially give the client 30 seconds to actually contact us
+	 * and store a credential before we decide there's no point in
+	 * keeping the daemon around.
+	 */
+	if (!wait_for_entry_until)
+		wait_for_entry_until = now + 30;
+
 	while (i < entries_nr) {
 		if (entries[i].expiration <= now) {
 			entries_nr--;
-			if (!entries_nr)
-				return 0;
 			free(entries[i].item.description);
 			free(entries[i].item.unique);
 			free(entries[i].item.username);
 			free(entries[i].item.password);
 			memcpy(&entries[i], &entries[entries_nr], sizeof(*entries));
+			/*
+			 * Stick around 30 seconds in case a new credential
+			 * shows up (e.g., because we just removed a failed
+			 * one, and we will soon get the correct one).
+			 */
+			wait_for_entry_until = now + 30;
 		}
 		else {
 			if (entries[i].expiration < next)
@@ -79,6 +92,12 @@ static int check_expirations(void)
 		}
 	}
 
+	if (!entries_nr) {
+		if (wait_for_entry_until <= now)
+			return 0;
+		next = wait_for_entry_until;
+	}
+
 	return next - now;
 }
 
-- 
1.7.6.10.g62f04