7b95d89 · unity8 · git.iko.soy

git.iko.soy/unity8

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/cancel/authenticate 100 times in quick succession. Without this branch, we'd crash reliably anywhere from attempt 2 to 7. With this branch, we're fine all 100. Fixes: #1425362 Approved by: Albert Astals Cid

commit7b95d892bee465fa38dc08eb21be2cf674513492 authorMichael Terry <michael.terry@canonical.com> dateMon 18 May 2015, 23:03 (11 years ago) parentc52616d 4dc3bec

4 files changed+117−22

compare with xenial

unifiedsplit

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

plugins/LightDM/liblightdm/GreeterPrivate.cpp+29−5

@@ -56,7 +56,8 @@ public:

5656 this, SLOT(handleMessage(pam_handle *, QString, QLightDM::Greeter::MessageType)));

5757 // This next connect is how we pass ResponseFutures between threads

5858 connect(this, SIGNAL(showPrompt(pam_handle *, QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)),

59− this, SLOT(handlePrompt(pam_handle *, QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)));

59+ this, SLOT(handlePrompt(pam_handle *, QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)),

60+ Qt::BlockingQueuedConnection);

6061 }

6162

6263 ~GreeterImpl()

@@ -68,6 +69,13 @@ public:

6869 {

6970 // Clear out any existing PAM interactions first

7071 cancelPam();

72+ if (pamHandle != nullptr) {

73+ // While we were cancelling pam above, we processed Qt events.

74+ // Which may have allowed someone to call start() on us again.

75+ // In which case, we'll bail on our current start() call.

76+ // This isn't racy because it's all in the same thread.

77+ return;

78+ }

7179

7280 AppData *appData = new AppData();

7381 appData->impl = this;

@@ -79,7 +87,7 @@ public:

7987

8088 if (pam_start("lightdm", username.toUtf8(), &conversation, &pamHandle) == PAM_SUCCESS) {

8189 appData->handle = pamHandle;

82− futureWatcher.setFuture(QtConcurrent::run(authenticateWithPam, pamHandle));

90+ futureWatcher.setFuture(QtConcurrent::mapped(QList<pam_handle*>() << pamHandle, authenticateWithPam));

8391 } else {

8492 delete appData;

8593 greeterPrivate->authenticated = false;

@@ -88,7 +96,7 @@ public:

8896 }

8997 }

9098

91− static int authenticateWithPam(pam_handle* pamHandle)

99+ static int authenticateWithPam(pam_handle* const& pamHandle)

92100 {

93101 int pamStatus = pam_authenticate(pamHandle, 0);

94102 if (pamStatus == PAM_SUCCESS) {

@@ -242,11 +250,22 @@ private Q_SLOTS:

242250private:

243251 void cancelPam()

244252 {

245− // Unfortunately we can't simply cancel our QFuture because QtConcurrent::run doesn't support cancel

246253 if (pamHandle != nullptr) {

254+ QFuture<int> pamFuture = futureWatcher.future();

247255 pam_handle *handle = pamHandle;

248256 pamHandle = nullptr; // to disable normal finishPam() handling

249− while (respond(QString())); // clear our local queue of QFutures

257+ pamFuture.cancel();

258+

259+ // Note the empty loop, we just want to clear the futures queue.

260+ // Any further prompts from the pam thread will be immediately

261+ // responded to/dismissed in handlePrompt().

262+ while (respond(QString()));

263+

264+ // Now let signal/slot handling happen so the thread can finish

265+ while (!pamFuture.isFinished()) {

266+ QCoreApplication::processEvents();

267+ }

268+

250269 pam_end(handle, PAM_CONV_ERR);

251270 }

252271 }

@@ -266,6 +285,11 @@ GreeterPrivate::GreeterPrivate(Greeter* parent)

266285{

267286}

268287

288+GreeterPrivate::~GreeterPrivate()

289+{

290+ delete m_impl;

291+}

292+

269293void GreeterPrivate::handleAuthenticate()

270294{

271295 m_impl->start(authenticationUser);

plugins/LightDM/liblightdm/GreeterPrivate.h+1−1

@@ -30,7 +30,7 @@ class GreeterPrivate

3030{

3131public:

3232 explicit GreeterPrivate(Greeter* parent=0);

33− virtual ~GreeterPrivate() = default;

33+ virtual ~GreeterPrivate();

3434

3535 // These variables may not be used by all subclasses, that's no problem

3636 bool authenticated;

tests/plugins/LightDM/CMakeLists.txt+35−16

@@ -1,8 +1,38 @@

1+add_definitions(

2+ -DCURRENT_SOURCE_DIR="${CMAKE_CURRENT_SOURCE_DIR}"

3+ )

4+include_directories(

5+ ${CMAKE_CURRENT_BINARY_DIR}

6+ )

18add_executable(GreeterDBusTestExec

29 dbus.cpp

310 ${CMAKE_SOURCE_DIR}/plugins/LightDM/Greeter.cpp

411 )

512qt5_use_modules(GreeterDBusTestExec Core DBus Quick Test)

13+target_link_libraries(GreeterDBusTestExec

14+ -L${CMAKE_BINARY_DIR}/tests/mocks/LightDM/liblightdm

15+ -llightdm-qt5-2

16+ )

17+target_include_directories(GreeterDBusTestExec PUBLIC

18+ ${CMAKE_SOURCE_DIR}/plugins/LightDM

19+ ${CMAKE_SOURCE_DIR}/tests/mocks/LightDM

20+ )

21+add_binary_qml_test(GreeterDBus "${CMAKE_BINARY_DIR}/tests/mocks/LightDM/liblightdm" MockLightDM "QML2_IMPORT_PATH=${CMAKE_BINARY_DIR}/tests/mocks")

22+

23+add_executable(GreeterPamTestExec

24+ pam.cpp

25+ ${CMAKE_SOURCE_DIR}/plugins/LightDM/liblightdm/GreeterPrivate.cpp

26+ )

27+qt5_use_modules(GreeterPamTestExec Concurrent Core Test)

28+target_link_libraries(GreeterPamTestExec

29+ integratedLightDM

30+ )

31+target_include_directories(GreeterPamTestExec PUBLIC

32+ ${CMAKE_SOURCE_DIR}/plugins/LightDM/liblightdm

33+ )

34+add_qmltest_target(testGreeterPam "${CMAKE_CURRENT_BINARY_DIR}/GreeterPamTestExec" TRUE FALSE)

35+add_dependencies(testGreeterPam GreeterPamTestExec)

636

737add_executable(GreeterUsersModelTestExec

838 usersmodel.cpp

@@ -10,24 +40,13 @@ add_executable(GreeterUsersModelTestExec

1040 ${CMAKE_SOURCE_DIR}/plugins/Utils/unitysortfilterproxymodelqml.cpp

1141 )

1242qt5_use_modules(GreeterUsersModelTestExec Core Test)

13−

14−include_directories(

15− ${CMAKE_CURRENT_BINARY_DIR}

43+target_link_libraries(GreeterUsersModelTestExec

44+ -L${CMAKE_BINARY_DIR}/tests/mocks/LightDM/liblightdm

45+ -llightdm-qt5-2

46+ )

47+target_include_directories(GreeterUsersModelTestExec PUBLIC

1648 ${CMAKE_SOURCE_DIR}/plugins/LightDM

1749 ${CMAKE_SOURCE_DIR}/plugins/Utils

1850 ${CMAKE_SOURCE_DIR}/tests/mocks/LightDM

1951 )

20−

21−target_link_libraries(GreeterDBusTestExec

22− MockLightDM

23− )

24−

25−target_link_libraries(GreeterUsersModelTestExec

26− MockLightDM

27− )

28−

29−add_definitions(-DCURRENT_SOURCE_DIR="${CMAKE_CURRENT_SOURCE_DIR}")

30−

31−add_binary_qml_test(GreeterDBus "${CMAKE_BINARY_DIR}/tests/mocks/LightDM/liblightdm" MockLightDM "QML2_IMPORT_PATH=${CMAKE_BINARY_DIR}/tests/mocks")

32−

3352add_binary_qml_test(GreeterUsersModel "${CMAKE_BINARY_DIR}/tests/mocks/LightDM/liblightdm" MockLightDM "LIBLIGHTDM_MOCK_MODE=full")

tests/plugins/LightDM/pam.cppnew file+52

@@ -0,0 +1,52 @@

1+/*

3+ *

4+ * This program is free software; you can redistribute it and/or modify

5+ * it under the terms of the GNU General Public License as published by

6+ * the Free Software Foundation; version 3.

7+ *

8+ * This program is distributed in the hope that it will be useful,

9+ * but WITHOUT ANY WARRANTY; without even the implied warranty of

10+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

11+ * GNU General Public License for more details.

12+ *

13+ * You should have received a copy of the GNU General Public License

14+ * along with this program. If not, see <http://www.gnu.org/licenses/>.

15+ */

16+

17+#include "GreeterPrivate.h"

18+

19+#include <QtTest>

20+

21+class GreeterPamTest : public QObject

22+{

23+ Q_OBJECT

24+

25+private Q_SLOTS:

26+

27+ void init()

28+ {

29+ m_greeterpriv = new QLightDM::GreeterPrivate();

30+ }

31+

32+ void cleanup()

33+ {

34+ delete m_greeterpriv;

35+ QTRY_COMPARE(QThreadPool::globalInstance()->activeThreadCount(), 0);

36+ }

37+

38+ void testRapidFireAuthentication()

39+ {

40+ m_greeterpriv->authenticationUser = qgetenv("USER");

41+ for (int i = 0; i < 100; i++) {

42+ m_greeterpriv->handleAuthenticate();

43+ }

44+ }

45+

46+private:

47+ QLightDM::GreeterPrivate *m_greeterpriv;

48+};

49+

50+QTEST_GUILESS_MAIN(GreeterPamTest)

51+

52+#include "pam.moc"