threads / discuss / 28239
Subject: t0300-credentials: poll failed: invalid argument
Brian GernhardtThe 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
Jeff KingRe: t0300-credentials: poll failed: invalid argument
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
Brian GernhardtRe: t0300-credentials: poll failed: invalid argument
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
Jeff KingRe: t0300-credentials: poll failed: invalid argument
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
Thomas RastRe: t0300-credentials: poll failed: invalid argument
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.chThomas RastRe: t0300-credentials: poll failed: invalid argument
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.chBrian GernhardtRe: t0300-credentials: poll failed: invalid argument
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
Jeff KingRe: t0300-credentials: poll failed: invalid argument
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