Created attachment 39080 [details] [review] Patch to fix the race Hi, There's a race that means that the user can type in their password, have the text entry disappear, and then nothing else happen for a couple of minutes until the call times out and they get a cryptic error. If they cancel the dialog in that time then the program will work as they are successfully authenticated. The reason for this is that if the SIGCHLD handler is called before the stdout one it unregisters the stdout handler, so it is never triggered, and that is the only way that a response is sent, positive or negative, except for cancelling. I'm attaching a patch which works around this in most cases by giving the stdout handler a chance to go first. You may prefer to reorganise a bit to not have one handler remove the other instead. It also ensures that the stdout handler is registered first, in case the child exits very quickly, but I don't know if that's possible. I'm also attaching a couple of other patches I produced in the course of investigating this that might be of interest. They fix theoretical problems, but they may never occur in the real world. Thanks, James
Created attachment 39081 [details] [review] Patch to ensure that the helper is declared running before a handler can fire
Created attachment 39082 [details] [review] Patch to ensure that things written to the child are written in full
Hi, I've been asked to patch this in Ubuntu, as there are apparently a fair number of users for whom policykit rarely works due to this issue. I'm not sure what the variable is there given that I have very rarely seen this myself. Could you review the changes please? If I don't hear either way soon then I will add distro patch just reviewed by Ubuntu developers. Thanks, James
Pretty sure these patches don't apply to current master as there were some changes post 0.96 - any chance you can check if this is still an issue in master (I can't reproduce this) and, if so, update the patches? Thanks.
(In reply to comment #4) > Pretty sure these patches don't apply to current master as there were some > changes post 0.96 - any chance you can check if this is still an issue in > master (I can't reproduce this) and, if so, update the patches? Thanks. I can't easily check this either, as I can't reproduce it at will. I've looked at the changes you make, and textually these won't apply, but I don't see that they will have fixed the race here, unless the g_source functions will somehow ensure that the stdout watch is serviced before the child one? Thanks, James
Still can't reproduce and haven't seen any authentication agent bugs mentioning this problem so closing as WORKSFORME. Please reopen if you manage to reproduce. Thanks.
Hi, I have no consistent way to reproduce, but the Ubuntu bug report was very "popular" indeed, with many duplicates. https://bugs.launchpad.net/ubuntu/+source/policykit-1/+bug/649939 for where I worked with Jean-Baptiste to find the patch, and https://bugs.launchpad.net/ubuntu/+source/update-manager/+bug/445303 for the bug that received most of the attention. There was a theoretical race that I found via code inspection, and a patch to remove it was confirmed to fix the issue with someone who could reliably reproduce. Unless you know that the g_source functions remove the race, then I think you should apply the patch. Thanks, James
In the absence of a recent polkit release, I'm looking into updating Debian experimental's polkit (which currently includes this patch) to current git master. This looks suspiciously like Bug #60847. James, does the patch that was merged for that bug look OK? It stops using the child watch at all, and only reads stdout, which seems a more correct solution to this.
(In reply to Simon McVittie from comment #8) > In the absence of a recent polkit release, I'm looking into updating Debian > experimental's polkit (which currently includes this patch) to current git > master. > > This looks suspiciously like Bug #60847. James, does the patch that was > merged for that bug look OK? It stops using the child watch at all, and only > reads stdout, which seems a more correct solution to this. Hi, That does sound rather similar, yes, though the symptoms in the arch bug sound a bit different. If it's not using the child watch then the problem may well be gone. Unfortunately I can't remember the specifics of a race condition from over 4 years ago to say for sure whether it will be handled by the other patch. Thanks, James
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.