{"thread":{"id":"28239","subject":"t0300-credentials: poll failed: invalid argument","startedAt":"2011-08-28T04:40:56Z","lastAt":"2011-09-14T14:49:32Z","messageCount":8,"participants":["Brian Gernhardt","Jeff King","Thomas Rast"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"174402","messageId":"5C993C44-D045-4344-95C1-94D3E6DB0316@silverinsanity.com","threadId":"28239","inReplyTo":null,"subject":"t0300-credentials: poll failed: invalid argument","fromName":"Brian Gernhardt","fromEmail":"benji@silverinsanity.com","sentAt":"2011-08-28T04:40:56Z","receivedAt":"2011-08-28T04:40:56Z","isPatch":false,"sender":{"key":"benji@silverinsanity.com","avatar":"https://gravatar.com/avatar/e06c101dbc25c68114d859b4a9ec7cf8a2c52fd2b0270ef0eac0e2e63ff22311?d=mp&s=160"},"body":"The only usage of poll I see in the credentials system is:\n\ncredentials-cache--daemon.c\n177:\tif (poll(&pfd, 1, 1000 * wakeup) < 0) {\n\nMy 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.\n\nDoes anyone familiar with the credentials daemon want to try to figure out a reasonable fix?\n\n~~ Brian\n"},{"id":"174468","messageId":"20110829171411.GB756@sigill.intra.peff.net","threadId":"28239","inReplyTo":"5C993C44-D045-4344-95C1-94D3E6DB0316@silverinsanity.com","subject":"Re: t0300-credentials: poll failed: invalid argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-29T17:14:11Z","receivedAt":"2011-08-29T17:14:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 28, 2011 at 12:40:56AM -0400, Brian Gernhardt wrote:\n\n> The only usage of poll I see in the credentials system is:\n> \n> credentials-cache--daemon.c\n> 177:\tif (poll(&pfd, 1, 1000 * wakeup) < 0) {\n> \n> My guess is that (1000 * wakeup) is more than INT_MAX and is becoming\n> negative as the man page for poll seems to indicate that it will fail\n> if timeout < -1.\n> \n> Does anyone familiar with the credentials daemon want to try to figure\n> out a reasonable fix?\n\nUgh, sorry, this is my fault. The check_expiration() function can return\na totally bogus value before we actually get any credentials.\n\nDoes this patch fix it for you?\n\n-- >8 --\nSubject: [PATCH] credential-cache: fix expiration calculation corner cases\n\nThe main credential-cache daemon loop calls poll to wait for\na client or to trigger the expiration of credentials. When\nthe last credential we hold expires, we exit.\n\nHowever, there is a corner case: when we first start up, we\nhave no credentials, and are waiting for a client to\nprovide us with one. In this case, we ended up handing\ncomplete junk for the timeout argument to poll(). On some\nsystems, this caused us to just wait a long time for the\nclient (which usually showed up within a second or so). On\nOS X, however, the system quite reasonably complained about\nour junk value with EINVAL.\n\nFixing this is pretty straightforward; we just notice that\nwe have no entries to compare against. However, that bug was\ncovering up another one: our expiration calculation didn't\ngive clients a chance to actually connect and provide us\nwith a credential before we decided that we should exit\nbecause we weren't holding any credentials!\n\nThe new algorithm is:\n\n 1. Sleep until it's time to expire the most recent\n credential.\n\n 2. If we don't have any credentials yet, wait 30 seconds\n for a client to contact us and give us one.\n\n 3. After expiring the last credential, wait 30 seconds for\n a client to provide us with one.\n\nTechnically only parts (1) and (2) are needed to implement\nthe original intended behavior.\n\nBut (3) is a minor optimization that is made easy by the new\ncode. When a client gives us a credential, then removes it\n(e.g., because it had a bogus password), and then gives us\nanother one, we used to exit, forcing the client to start a\nnew daemon instance. Instead, we can just reuse the existing\ndaemon instance.\n\nSigned-off-by: Jeff King \n---\n credential-cache--daemon.c | 23 +++++++++++++++++++++--\n 1 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/credential-cache--daemon.c b/credential-cache--daemon.c\nindex f520347..d6769b1 100644\n--- a/credential-cache--daemon.c\n+++ b/credential-cache--daemon.c\n@@ -57,20 +57,33 @@ static void remove_credential(const struct credential *c)\n \n static int check_expirations(void)\n {\n+\tstatic unsigned long wait_for_entry_until;\n \tint i = 0;\n \tunsigned long now = time(NULL);\n \tunsigned long next = (unsigned long)-1;\n \n+\t/*\n+\t * Initially give the client 30 seconds to actually contact us\n+\t * and store a credential before we decide there's no point in\n+\t * keeping the daemon around.\n+\t */\n+\tif (!wait_for_entry_until)\n+\t\twait_for_entry_until = now + 30;\n+\n \twhile (i < entries_nr) {\n \t\tif (entries[i].expiration <= now) {\n \t\t\tentries_nr--;\n-\t\t\tif (!entries_nr)\n-\t\t\t\treturn 0;\n \t\t\tfree(entries[i].item.description);\n \t\t\tfree(entries[i].item.unique);\n \t\t\tfree(entries[i].item.username);\n \t\t\tfree(entries[i].item.password);\n \t\t\tmemcpy(&entries[i], &entries[entries_nr], sizeof(*entries));\n+\t\t\t/*\n+\t\t\t * Stick around 30 seconds in case a new credential\n+\t\t\t * shows up (e.g., because we just removed a failed\n+\t\t\t * one, and we will soon get the correct one).\n+\t\t\t */\n+\t\t\twait_for_entry_until = now + 30;\n \t\t}\n \t\telse {\n \t\t\tif (entries[i].expiration < next)\n@@ -79,6 +92,12 @@ static int check_expirations(void)\n \t\t}\n \t}\n \n+\tif (!entries_nr) {\n+\t\tif (wait_for_entry_until <= now)\n+\t\t\treturn 0;\n+\t\tnext = wait_for_entry_until;\n+\t}\n+\n \treturn next - now;\n }\n \n-- \n1.7.6.10.g62f04\n"},{"id":"174471","messageId":"01E9C05C-A19D-45B0-B15D-DA6B911C11A9@silverinsanity.com","threadId":"28239","inReplyTo":"20110829171411.GB756@sigill.intra.peff.net","subject":"Re: t0300-credentials: poll failed: invalid argument","fromName":"Brian Gernhardt","fromEmail":"benji@silverinsanity.com","sentAt":"2011-08-29T17:28:05Z","receivedAt":"2011-08-29T17:28:05Z","isPatch":false,"sender":{"key":"benji@silverinsanity.com","avatar":"https://gravatar.com/avatar/e06c101dbc25c68114d859b4a9ec7cf8a2c52fd2b0270ef0eac0e2e63ff22311?d=mp&s=160"},"body":"\nOn Aug 29, 2011, at 1:14 PM, Jeff King wrote:\n\n> On Sun, Aug 28, 2011 at 12:40:56AM -0400, Brian Gernhardt wrote:\n> \n>> The only usage of poll I see in the credentials system is:\n>> \n>> credentials-cache--daemon.c\n>> 177:\tif (poll(&pfd, 1, 1000 * wakeup) < 0) {\n>> \n>> My guess is that (1000 * wakeup) is more than INT_MAX and is becoming\n>> negative as the man page for poll seems to indicate that it will fail\n>> if timeout < -1.\n>> \n>> Does anyone familiar with the credentials daemon want to try to figure\n>> out a reasonable fix?\n> \n> Ugh, sorry, this is my fault. The check_expiration() function can return\n> a totally bogus value before we actually get any credentials.\n> \n> Does this patch fix it for you?\n\nYes it does! Surprisingly enough, non-bogus parameters keeps poll from erroring with EINVAL. Funny that. ;-)\n\nMany thanks,\n~~ Brian\n"},{"id":"174472","messageId":"20110829174309.GA11524@sigill.intra.peff.net","threadId":"28239","inReplyTo":"01E9C05C-A19D-45B0-B15D-DA6B911C11A9@silverinsanity.com","subject":"Re: t0300-credentials: poll failed: invalid argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-29T17:43:09Z","receivedAt":"2011-08-29T17:43:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 29, 2011 at 01:28:05PM -0400, Brian Gernhardt wrote:\n\n> > Ugh, sorry, this is my fault. The check_expiration() function can return\n> > a totally bogus value before we actually get any credentials.\n> > \n> > Does this patch fix it for you?\n> \n> Yes it does! Surprisingly enough, non-bogus parameters keeps poll\n> from erroring with EINVAL. Funny that. ;-)\n\nGreat. I'm working on a few more patches on top of that topic, so I'll\nadd it to my list to send out in the next day or so.\n\n-Peff\n"},{"id":"175191","messageId":"201109091613.13137.trast@student.ethz.ch","threadId":"28239","inReplyTo":"20110829174309.GA11524@sigill.intra.peff.net","subject":"Re: t0300-credentials: poll failed: invalid argument","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-09T14:13:13Z","receivedAt":"2011-09-09T14:13:13Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King wrote:\n> On Mon, Aug 29, 2011 at 01:28:05PM -0400, Brian Gernhardt wrote:\n> \n> > > Ugh, sorry, this is my fault. The check_expiration() function can return\n> > > a totally bogus value before we actually get any credentials.\n> > > \n> > > Does this patch fix it for you?\n> > \n> > Yes it does! Surprisingly enough, non-bogus parameters keeps poll\n> > from erroring with EINVAL. Funny that. ;-)\n> \n> Great. I'm working on a few more patches on top of that topic, so I'll\n> add it to my list to send out in the next day or so.\n\nI'm still seeing this with current pu (from repo.or.cz), but only on\nOS X\n\n $ uname -a\n 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\n\nWhere \"this\" is:\n\n --- expect-stderr 2011-09-09 14:12:13.000000000 +0000\n +++ stderr 2011-09-09 14:12:13.000000000 +0000\n @@ -1,2 +1,3 @@\n askpass: Username:\n askpass: Password:\n +fatal: poll failed: Invalid argument\n\nfor each of the tests 15--19. Is it supposed to be fixed?\n\nI don't have time to look into it without knowing what to search for,\nbut if you want me to test anything on that OS X just ask.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"175459","messageId":"201109141015.58333.trast@student.ethz.ch","threadId":"28239","inReplyTo":"201109091613.13137.trast@student.ethz.ch","subject":"Re: t0300-credentials: poll failed: invalid argument","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-14T08:15:58Z","receivedAt":"2011-09-14T08:15:58Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast wrote:\n> $ uname -a\n> 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\n[...]\n> --- expect-stderr 2011-09-09 14:12:13.000000000 +0000\n> +++ stderr 2011-09-09 14:12:13.000000000 +0000\n> @@ -1,2 +1,3 @@\n> askpass: Username:\n> askpass: Password:\n> +fatal: poll failed: Invalid argument\n> \n> for each of the tests 15--19. Is it supposed to be fixed?\n\nPing?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"175473","messageId":"C9FED2AA-15CB-4850-B3DA-F4FC12F06EB4@silverinsanity.com","threadId":"28239","inReplyTo":"201109141015.58333.trast@student.ethz.ch","subject":"Re: t0300-credentials: poll failed: invalid argument","fromName":"Brian Gernhardt","fromEmail":"benji@silverinsanity.com","sentAt":"2011-09-14T14:24:01Z","receivedAt":"2011-09-14T14:24:01Z","isPatch":false,"sender":{"key":"benji@silverinsanity.com","avatar":"https://gravatar.com/avatar/e06c101dbc25c68114d859b4a9ec7cf8a2c52fd2b0270ef0eac0e2e63ff22311?d=mp&s=160"},"body":"\nOn Sep 14, 2011, at 4:15 AM, Thomas Rast wrote:\n\n> Thomas Rast wrote:\n>> $ uname -a\n>> 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\n> [...]\n>> --- expect-stderr 2011-09-09 14:12:13.000000000 +0000\n>> +++ stderr 2011-09-09 14:12:13.000000000 +0000\n>> @@ -1,2 +1,3 @@\n>> askpass: Username:\n>> askpass: Password:\n>> +fatal: poll failed: Invalid argument\n>> \n>> for each of the tests 15--19. Is it supposed to be fixed?\n> \n> Ping?\n\nJeff'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*\n\n~~ Brian\n"},{"id":"175474","messageId":"20110914144932.GA12175@sigill.intra.peff.net","threadId":"28239","inReplyTo":"C9FED2AA-15CB-4850-B3DA-F4FC12F06EB4@silverinsanity.com","subject":"Re: t0300-credentials: poll failed: invalid argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-14T14:49:32Z","receivedAt":"2011-09-14T14:49:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 14, 2011 at 10:24:01AM -0400, Brian Gernhardt wrote:\n\n> >> --- expect-stderr 2011-09-09 14:12:13.000000000 +0000\n> >> +++ stderr 2011-09-09 14:12:13.000000000 +0000\n> >> @@ -1,2 +1,3 @@\n> >> askpass: Username:\n> >> askpass: Password:\n> >> +fatal: poll failed: Invalid argument\n> >> \n> >> for each of the tests 15--19. Is it supposed to be fixed?\n> > \n> > Ping?\n> \n> Jeff's patch did fix this for me, but it never appears to have made it\n> into git.git. He mentioned something about re-rolling it along with\n> some other fixes... *hint, hint*\n\nYeah, sorry, I wanted to add some more tests for handling multiple\nusernames, and then maybe the helper interface was changing, and then...\n\nThere's no reason to hold this up, though. I'll repost it later today.\nPromise this time. ;)\n\n-Peff\n"}]}