Merge lp:~mterry/unity8/cancel-pam-harder into lp:unity8
| Status: | Merged |
|---|---|
| Approved by: | Albert Astals Cid on 2015-05-08 |
| Approved revision: | 1644 |
| Merged at revision: | 1780 |
| Proposed branch: | lp:~mterry/unity8/cancel-pam-harder |
| Merge into: | lp:unity8 |
| Diff against target: |
225 lines (+117/-22) 4 files modified
plugins/LightDM/liblightdm/GreeterPrivate.cpp (+29/-5) plugins/LightDM/liblightdm/GreeterPrivate.h (+1/-1) tests/plugins/LightDM/CMakeLists.txt (+35/-16) tests/plugins/LightDM/pam.cpp (+52/-0) |
| To merge this branch: | bzr merge lp:~mterry/unity8/cancel-pam-harder |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Albert Astals Cid (community) | 2015-02-26 | Approve on 2015-05-08 | |
| PS Jenkins bot | continuous-integration | Needs Fixing on 2015-04-23 | |
| Michał Sawicz | Needs Fixing on 2015-03-16 | ||
|
Review via email:
|
|||
Commit Message
Fix a possible crash in our PAM threading code. (LP: #1425362)
So it was possible for a PAM subthread (which does the actual authenticating) to get mismatched state with the main thread. When this happened (i.e. when the subthread tried to use a pam_handle object that the main thread had already closed), a crash occurs. Like LP: #1425362.
So I've done a few cleanups here:
- Use a cancellable QConcurrent method (::mapped instead of ::run). Being able to cancel the thread is a stronger hammer than just hoping we answer all questions from the thread and it will close itself.
- We now block when the subthread sends a prompt request to the main thread. This makes handling those signals more predictable when we're trying to close the thread.
- We will process events while waiting for the subthread to close, so that we can actually process the above signals in the main thread.
I've added a dumb little test that tries to authenticate/
Description of the Change
Fix a possible crash in our PAM threading code. (LP: #1425362)
So it was possible for a PAM subthread (which does the actual authenticating) to get mismatched state with the main thread. When this happened (i.e. when the subthread tried to use a pam_handle object that the main thread had already closed), a crash occurs. Like LP: #1425362.
So I've done a few cleanups here:
- Use a cancellable QConcurrent method (::mapped instead of ::run). Being able to cancel the thread is a stronger hammer than just hoping we answer all questions from the thread and it will close itself.
- We now block when the subthread sends a prompt request to the main thread. This makes handling those signals more predictable when we're trying to close the thread.
- We will process events while waiting for the subthread to close, so that we can actually process the above signals in the main thread.
I've added a dumb little test that tries to authenticate/
Users didn't notice this bug, because the timing is tough to reproduce naturally (I would have thought, but then bug 1425362 noticed in a VM... Maybe the slowness of a VM helps surface this).
== Checklist ==
* Are there any related MPs required for this MP to build/function as expected? Please list.
No
* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes... I added a test anyway, that stresses this code
* Did you make sure that your branch does not contain spurious tags?
Yes
* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
NA
* If you changed the UI, has there been a design review?
NA
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1636
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1637
http://
Executed test runs:
UNSTABLE: http://
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Michael Terry (mterry) wrote : | # |
Marco Trevisan (the original bug filer) notes that this branch fixes his crash.
| Albert Astals Cid (aacid) wrote : | # |
Tests never seem to finish now
6/22 Test #6: testGreeterPam .......
| Albert Astals Cid (aacid) wrote : | # |
The test passes for me locally but fails in CI, i wonder if either it needs some extra dependency we don't have on CI or if it needs X/xvfb
| Michał Sawicz (saviq) wrote : | # |
Interestingly, this is what fails for me under sbuild:
22/23 Test #23: wizardsystemtest .......
********* Start testing of SystemTest *********
Config: Using QtTest library 5.4.0, Qt 5.4.0 (x86_64-
PASS : SystemTest:
PASS : SystemTest:
QInotifyFileSys
PASS : SystemTest:
QInotifyFileSys
FAIL! : SystemTest:
Actual (((spy.count()))): 3
Expected (4) : 4
Loc: [/«BUILDDIR»
PASS : SystemTest:
Totals: 4 passed, 1 failed, 0 skipped, 0 blacklisted
********* Finished testing of SystemTest *********
| Michael Terry (mterry) wrote : | # |
Huh. During build it passes in jenkins (since make test is run during build). It's just during the qmluitest run that it fails.
| Michael Terry (mterry) wrote : | # |
<Saviq> mterry, huh, the failure I reported actually is there in trunk for me...
So that leaves just the jenkins failure. I'm trying to see how the qmluitests environment is different from the build one.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1638
http://
Executed test runs:
UNSTABLE: http://
FAILURE: http://
SUCCESS: http://
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1639
http://
Executed test runs:
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Michael Terry (mterry) wrote : | # |
OK, addressed the build failure in jenkins. The remaining qmluitest failures are issues in trunk.
| Francis Ginther (fginther) wrote : | # |
The above mako testing failed due to an unbootable image making it into the pipeline. The ci job has been restarted.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1639
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Albert Astals Cid (aacid) wrote : | # |
Small fix:
* You can change the two futureWatcher.
About QCoreApplicatio
Also does respond(QString()) need to happen repeteadly inside the loop or just once?
| Michael Terry (mterry) wrote : | # |
> You can change the two futureWatcher.
> to futureWatcher.
Done.
> About QCoreApplicatio
> side, is it possible to put the code in a slot attached to
> the futurewatcher finished signal?
This would mean holding on to a map of QFuture objects to pam_handle objects so we'd know which ones need to have pam_end called on them when the thread is done. And keeping a set of waiting-
I just figured threading code and PAM code are each complicated enough that I didn't want to try to be fancy. So I favored the direct route.
But I agree that a manual processEvents isn't super sexy. I can change it to the above if you'd like.
> Also does respond(QString()) need to happen
> repeteadly inside the loop or just once?
PAM can offer multiple prompts in a row. The child-thread will block on each response one at a time in a small loop. QFuture.cancel() will not interrupt a waiting child-thread.
But if you can promise that it *will* interrupt the thread the second it stops waiting, before it can get to waiting on a second prompt, then we only need respond() once. And we could even get rid of the loop and just call processEvents once.
I was not so confident in how cancel() worked. But I haven't dealt with Qt threads often.
| Albert Astals Cid (aacid) wrote : | # |
As far as i know, no, cancel won't terminate the thread, actually i think would not cancel authenticateWithPam in the middle of an execution, it's just that the future has not started executing and is cancelled before doing so.
As discussed on irc, please "add code to verify that all respond() have been properly dispatched"
| Albert Astals Cid (aacid) wrote : | # |
add code to the test :D
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1640
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Albert Astals Cid (aacid) wrote : | # |
* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes
* Did CI run pass?
No, but the added one passes
* Did you make sure that the branch does not contain spurious tags?
Yes
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1641
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Michał Sawicz (saviq) wrote : | # |
This prevents me from unlocking unity8 on my desktop. The password entry field goes wiggle-
The unity8.log prints a seemingly related:
> This backend doesn't support multiple users
| Michael Terry (mterry) wrote : | # |
OK, try this again. I was able to reproduce your problem on the desktop, Saviq. Fixed it.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1644
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1644
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Albert Astals Cid (aacid) wrote : | # |
I retested the saviq was having and i can't reproduce anymore.

FAILED: Continuous integration, rev:1636 jenkins. qa.ubuntu. com/job/ unity8- ci/5366/ jenkins. qa.ubuntu. com/job/ generic- deb-autopilot- vivid-touch/ 1565/console jenkins. qa.ubuntu. com/job/ unity-phablet- qmluitests- vivid/530/ console jenkins. qa.ubuntu. com/job/ unity8- vivid-amd64- ci/531/ console jenkins. qa.ubuntu. com/job/ unity8- vivid-i386- ci/531/ console jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- vivid-armhf/ 1563/console
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/unity8- ci/5366/ rebuild
http://