t0300-credentials: poll failed: invalid argument

t0300-credentials: poll failed: invalid argument

[Brian Gernhardt]

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?

~~ Brian

Re: t0300-credentials: poll failed: invalid argument

[Jeff King]

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

Re: t0300-credentials: poll failed: invalid argument

[Brian Gernhardt]

On Aug 29, 2011, at 1:14 PM, Jeff King wrote:

> 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?

Yes it does!  Surprisingly enough, non-bogus parameters keeps poll from erroring with EINVAL.  Funny that.  ;-)

Many thanks,
~~ Brian

Re: t0300-credentials: poll failed: invalid argument

[Jeff King]

On Mon, Aug 29, 2011 at 01:28:05PM -0400, Brian Gernhardt wrote:

> > 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?
> 
> Yes it does!  Surprisingly enough, non-bogus parameters keeps poll
> from erroring with EINVAL.  Funny that.  ;-)

Great. I'm working on a few more patches on top of that topic, so I'll
add it to my list to send out in the next day or so.

-Peff

Re: t0300-credentials: poll failed: invalid argument

[Thomas Rast]

Jeff King wrote:
> On Mon, Aug 29, 2011 at 01:28:05PM -0400, Brian Gernhardt wrote:
> 
> > > 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?
> > 
> > Yes it does!  Surprisingly enough, non-bogus parameters keeps poll
> > from erroring with EINVAL.  Funny that.  ;-)
> 
> Great. I'm working on a few more patches on top of that topic, so I'll
> add it to my list to send out in the next day or so.

I'm still seeing this with current pu (from repo.or.cz), but only on
OS X

  $ uname -a
  Darwin mackeller.inf.ethz.ch 11.1.0 Darwin Kernel Version 11.1.0: Tue Jul 26 16:07:11 PDT 2011; root:xnu-1699.22.81~1/RELEASE_X86_64 x86_64

Where "this" is:

  --- expect-stderr       2011-09-09 14:12:13.000000000 +0000
  +++ stderr      2011-09-09 14:12:13.000000000 +0000
  @@ -1,2 +1,3 @@
   askpass: Username:
   askpass: Password:
  +fatal: poll failed: Invalid argument

for each of the tests 15--19.  Is it supposed to be fixed?

I don't have time to look into it without knowing what to search for,
but if you want me to test anything on that OS X just ask.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: t0300-credentials: poll failed: invalid argument

[Thomas Rast]

Thomas Rast wrote:
>   $ uname -a
>   Darwin mackeller.inf.ethz.ch 11.1.0 Darwin Kernel Version 11.1.0: Tue Jul 26 16:07:11 PDT 2011; root:xnu-1699.22.81~1/RELEASE_X86_64 x86_64
[...]
>   --- expect-stderr       2011-09-09 14:12:13.000000000 +0000
>   +++ stderr      2011-09-09 14:12:13.000000000 +0000
>   @@ -1,2 +1,3 @@
>    askpass: Username:
>    askpass: Password:
>   +fatal: poll failed: Invalid argument
> 
> for each of the tests 15--19.  Is it supposed to be fixed?

Ping?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: t0300-credentials: poll failed: invalid argument

[Brian Gernhardt]

On Sep 14, 2011, at 4:15 AM, Thomas Rast wrote:

> Thomas Rast wrote:
>>  $ uname -a
>>  Darwin mackeller.inf.ethz.ch 11.1.0 Darwin Kernel Version 11.1.0: Tue Jul 26 16:07:11 PDT 2011; root:xnu-1699.22.81~1/RELEASE_X86_64 x86_64
> [...]
>>  --- expect-stderr       2011-09-09 14:12:13.000000000 +0000
>>  +++ stderr      2011-09-09 14:12:13.000000000 +0000
>>  @@ -1,2 +1,3 @@
>>   askpass: Username:
>>   askpass: Password:
>>  +fatal: poll failed: Invalid argument
>> 
>> for each of the tests 15--19.  Is it supposed to be fixed?
> 
> Ping?

Jeff's patch did fix this for me, but it never appears to have made it into git.git.  He mentioned something about re-rolling it along with some other fixes...  *hint, hint*

~~ Brian

Re: t0300-credentials: poll failed: invalid argument

[Jeff King]

On Wed, Sep 14, 2011 at 10:24:01AM -0400, Brian Gernhardt wrote:

> >>  --- expect-stderr       2011-09-09 14:12:13.000000000 +0000
> >>  +++ stderr      2011-09-09 14:12:13.000000000 +0000
> >>  @@ -1,2 +1,3 @@
> >>   askpass: Username:
> >>   askpass: Password:
> >>  +fatal: poll failed: Invalid argument
> >> 
> >> for each of the tests 15--19.  Is it supposed to be fixed?
> > 
> > Ping?
> 
> Jeff's patch did fix this for me, but it never appears to have made it
> into git.git.  He mentioned something about re-rolling it along with
> some other fixes...  *hint, hint*

Yeah, sorry, I wanted to add some more tests for handling multiple
usernames, and then maybe the helper interface was changing, and then...

There's no reason to hold this up, though. I'll repost it later today.
Promise this time. ;)

-Peff