Skip to content

Cleanup thread cleaning and fix PinPAD crash #370

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Cleanup thread cleaning and fix PinPAD crash #370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 59 additions & 121 deletions src/controller/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@
#include "threads/waitforcardthread.hpp"

#include "utils/utils.hpp"

#include "application.hpp"
#include "inputoutputmode.hpp"
#include "writeresponse.hpp"

#include <QApplication>

using namespace pcsc_cpp;
using namespace electronic_id;

Expand All @@ -44,19 +44,11 @@ const QString RESP_USER_CANCEL = QStringLiteral("ERR_WEBEID_USER_CANCELLED");

QVariantMap makeErrorObject(const QString& errorCode, const QString& errorMessage)
{
const auto errorBody = QVariantMap {
{QStringLiteral("code"), errorCode},
{QStringLiteral("message"), errorMessage},
};
return {{QStringLiteral("error"), errorBody}};
}

void interruptThread(QThread* thread)
{
qDebug() << "Interrupting thread" << uintptr_t(thread);
thread->disconnect();
thread->requestInterruption();
ControllerChildThread::waitForControllerNotify.wakeAll();
return {{QStringLiteral("error"),
QVariantMap {
{QStringLiteral("code"), errorCode},
{QStringLiteral("message"), errorMessage},
}}};
}

} // namespace
Expand All @@ -67,17 +59,19 @@ void Controller::run()
const bool isInCommandLineMode = bool(command);
isInStdinMode = !isInCommandLineMode;

qInfo() << qApp->applicationName() << "app" << qApp->applicationVersion() << "running in"
qInfo() << QCoreApplication::applicationName() << "app"
<< QCoreApplication::applicationVersion() << "running in"
<< (isInStdinMode ? "stdin/stdout" : "command-line") << "mode";

try {
// TODO: cut out stdin mode separate class to avoid bugs in safari mode
if (isInStdinMode) {
// In stdin/stdout mode we first output the version as required by the WebExtension
// and then wait for the actual command.
writeResponseToStdOut(isInStdinMode,
{{QStringLiteral("version"), qApp->applicationVersion()}},
"get-version");
writeResponseToStdOut(
isInStdinMode,
{{QStringLiteral("version"), QCoreApplication::applicationVersion()}},
"get-version");

command = readCommandFromStdin();
}
Expand Down Expand Up @@ -111,56 +105,33 @@ void Controller::startCommandExecution()
REQUIRE_NON_NULL(commandHandler)

// Reader monitor thread setup.
WaitForCardThread* waitForCardThread = new WaitForCardThread(this);
auto* waitForCardThread = new WaitForCardThread(this);
connect(this, &Controller::stopCardEventMonitorThread, waitForCardThread,
&WaitForCardThread::requestInterruption);
connect(waitForCardThread, &ControllerChildThread::failure, this,
&Controller::onCriticalFailure);
connect(waitForCardThread, &WaitForCardThread::statusUpdate, this, &Controller::statusUpdate);
connect(waitForCardThread, &WaitForCardThread::cardsAvailable, this,
&Controller::onCardsAvailable);
saveChildThreadPtrAndConnectFailureFinish(waitForCardThread);

// UI setup.
window = WebEidUI::createAndShowDialog(commandHandler->commandType());
connect(this, &Controller::statusUpdate, window, &WebEidUI::onSmartCardStatusUpdate);
connectOkCancelWaitingForPinPad();
createWindow();

// Finally, start the thread to wait for card insertion after everything is wired up.
waitForCardThread->start();
}

void Controller::saveChildThreadPtrAndConnectFailureFinish(ControllerChildThread* childThread)
{
REQUIRE_NON_NULL(childThread)
// Save the thread pointer in child thread tracking map to request interruption and wait for
// it to quit in waitForChildThreads().
childThreads[uintptr_t(childThread)] = childThread;

connect(childThread, &ControllerChildThread::failure, this, &Controller::onCriticalFailure);

// When the thread is finished, remove the pointer from the tracking map and call deleteLater()
// on it to free the thread object. Although the thread objects are freed through the Qt object
// tree ownership system anyway, it is better to delete them immediately when they finish.
connect(childThread, &ControllerChildThread::finished, this, [this, childThread]() {
QScopedPointer<ControllerChildThread, QScopedPointerDeleteLater> deleteLater {childThread};

const auto threadPtrAddress = uintptr_t(childThread);
if (childThreads.count(threadPtrAddress) && childThreads[threadPtrAddress]) {
childThreads[threadPtrAddress] = nullptr;
childThread->wait();
qDebug() << "Thread" << threadPtrAddress << "finished";
} else {
qWarning() << "Controller child thread" << childThread
<< "is missing or null in finish slot";
}
});
}

void Controller::connectOkCancelWaitingForPinPad()
void Controller::createWindow()
{
REQUIRE_NON_NULL(window)

window = WebEidUI::createAndShowDialog(commandHandler->commandType());
connect(this, &Controller::statusUpdate, window, &WebEidUI::onSmartCardStatusUpdate);
connect(this, &Controller::retry, window, &WebEidUI::onRetry);
connect(window, &WebEidUI::retry, this, &Controller::onRetry);
connect(window, &WebEidUI::accepted, this, &Controller::onDialogOK);
connect(window, &WebEidUI::rejected, this, &Controller::onDialogCancel);
connect(window, &WebEidUI::failure, this, &Controller::onCriticalFailure);
connect(window, &WebEidUI::waitingForPinPad, this, &Controller::onConfirmCommandHandler);
connect(window, &WebEidUI::destroyed, this, [this] { window = nullptr; });
}

void Controller::onCardsAvailable(
Expand Down Expand Up @@ -192,9 +163,8 @@ void Controller::onCardsAvailable(
void Controller::runCommandHandler(const std::vector<ElectronicID::ptr>& availableEids)
{
try {
CommandHandlerRunThread* commandHandlerRunThread =
auto* commandHandlerRunThread =
new CommandHandlerRunThread(this, *commandHandler, availableEids);
saveChildThreadPtrAndConnectFailureFinish(commandHandlerRunThread);
connectRetry(commandHandlerRunThread);

// When the command handler run thread retrieves certificates successfully, call
Expand All @@ -213,51 +183,34 @@ void Controller::runCommandHandler(const std::vector<ElectronicID::ptr>& availab

void Controller::onCertificatesLoaded()
{
CardEventMonitorThread* cardEventMonitorThread =
new CardEventMonitorThread(this, std::string(commandType()));
saveChildThreadPtrAndConnectFailureFinish(cardEventMonitorThread);
cardEventMonitorThreadKey = uintptr_t(cardEventMonitorThread);
auto* cardEventMonitorThread = new CardEventMonitorThread(this, commandType());
connect(this, &Controller::stopCardEventMonitorThread, cardEventMonitorThread,
&CardEventMonitorThread::requestInterruption);
connect(cardEventMonitorThread, &ControllerChildThread::failure, this,
&Controller::onCriticalFailure);
connect(cardEventMonitorThread, &CardEventMonitorThread::cardEvent, this, &Controller::onRetry);
cardEventMonitorThread->start();
}

void Controller::stopCardEventMonitorThread()
{
if (cardEventMonitorThreadKey) {
try {
auto cardEventMonitorThread = childThreads.at(cardEventMonitorThreadKey);
cardEventMonitorThreadKey = 0;
if (cardEventMonitorThread) {
interruptThread(cardEventMonitorThread);
}
} catch (const std::out_of_range&) {
qWarning() << "Card event monitor thread" << cardEventMonitorThreadKey
<< "is missing from childThreads map in stopCardEventMonitorThread()";
cardEventMonitorThreadKey = 0;
}
}
}

void Controller::disposeUI()
{
if (window) {
window->disconnect();
// As the Qt::WA_DeleteOnClose flag is set, the dialog is deleted automatically.
window->close();
window->forceClose();
window->deleteLater();
window = nullptr;
}
}

void Controller::onConfirmCommandHandler(const EidCertificateAndPinInfo& certAndPinInfo)
{
stopCardEventMonitorThread();
emit stopCardEventMonitorThread();

try {
CommandHandlerConfirmThread* commandHandlerConfirmThread =
auto* commandHandlerConfirmThread =
new CommandHandlerConfirmThread(this, *commandHandler, window, certAndPinInfo);
connect(commandHandlerConfirmThread, &CommandHandlerConfirmThread::completed, this,
&Controller::onCommandHandlerConfirmCompleted);
saveChildThreadPtrAndConnectFailureFinish(commandHandlerConfirmThread);
connectRetry(commandHandlerConfirmThread);

commandHandlerConfirmThread->start();
Expand Down Expand Up @@ -308,14 +261,10 @@ void Controller::onRetry()
void Controller::connectRetry(const ControllerChildThread* childThread)
{
REQUIRE_NON_NULL(childThread)
REQUIRE_NON_NULL(window)

disconnect(window, &WebEidUI::retry, nullptr, nullptr);

connect(childThread, &ControllerChildThread::retry, window, &WebEidUI::onRetry);
connect(childThread, &ControllerChildThread::failure, this, &Controller::onCriticalFailure);
connect(childThread, &ControllerChildThread::retry, this, &Controller::retry);
// This connection handles cancel events from PIN pad.
connect(childThread, &ControllerChildThread::cancel, this, &Controller::onPinPadCancel);
connect(window, &WebEidUI::retry, this, &Controller::onRetry);
connect(childThread, &ControllerChildThread::cancel, this, &Controller::onDialogCancel);
}

void Controller::onDialogOK(const EidCertificateAndPinInfo& certAndPinInfo)
Expand All @@ -324,67 +273,56 @@ void Controller::onDialogOK(const EidCertificateAndPinInfo& certAndPinInfo)
onConfirmCommandHandler(certAndPinInfo);
} else {
// This should not happen, and when it does, OK should be equivalent to cancel.
onPinPadCancel();
onDialogCancel();
}
}

void Controller::onDialogCancel()
{
REQUIRE_NON_NULL(window)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you emit stopCardEventMonitorThread() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see that there can be any running thread at this point

qDebug() << "User cancelled";

// Schedule application exit when the UI dialog is destroyed.
connect(window, &WebEidUI::destroyed, this, &Controller::exit);

_result = makeErrorObject(RESP_USER_CANCEL, QStringLiteral("User cancelled"));
writeResponseToStdOut(isInStdinMode, _result, commandType());
}

void Controller::onPinPadCancel()
{
REQUIRE_NON_NULL(window)

onDialogCancel();
window->quit();
exit();
}

void Controller::onCriticalFailure(const QString& error)
{
emit stopCardEventMonitorThread();
qCritical() << "Exiting due to command" << std::string(commandType())
<< "fatal error:" << error;
_result =
makeErrorObject(RESP_TECH_ERROR, QStringLiteral("Technical error, see application logs"));
writeResponseToStdOut(isInStdinMode, _result, commandType());
disposeUI();
if (qApp->isSafariExtensionContainingApp()) {
writeResponseToStdOut(isInStdinMode, _result, commandType());
}
WebEidUI::showFatalError();
if (!qApp->isSafariExtensionContainingApp()) {
// Write the error response to stdout after showing the fatal error dialog. Chrome will
// close the application immediately after this, so the UI dialog may not be visible to the
// user.
writeResponseToStdOut(isInStdinMode, _result, commandType());
}
exit();
}

void Controller::exit()
{
if (window) {
window->disconnect();
window = nullptr;
}
disposeUI();
waitForChildThreads();
emit quit();
}

void Controller::waitForChildThreads()
{
// Waiting for child threads must not happen in destructor.
// See https://tombarta.wordpress.com/2008/07/10/gcc-pure-virtual-method-called/ for details.
for (const auto& childThread : childThreads) {
auto thread = childThread.second;
if (thread) {
interruptThread(thread);
// Waiting for PIN input on PIN pad may take a long time, call processEvents() so that
// the UI doesn't freeze.
while (thread->isRunning()) {
thread->wait(100); // in milliseconds
QCoreApplication::processEvents();
}
for (auto* thread : findChildren<QThread*>()) {
qDebug() << "Interrupting thread" << uintptr_t(thread);
thread->disconnect();
thread->requestInterruption();
ControllerChildThread::waitForControllerNotify.wakeAll();
while (thread->isRunning()) {
using namespace std::chrono_literals;
thread->wait(100ms);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 100 millisecond wait made only sense along with event processing, recommend to add it back:

Suggested change
thread->wait(100ms);
thread->wait(100ms);
QCoreApplication::processEvents();

Otherwise the while loop does not have a purpose, it is equivalent to just waiting indefinitely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All calls waitForChildThreads() are after disposeUI, no point to process any more events.
and timer makes sure that function does not continue before all thread are ended

}
}
}
Expand Down
17 changes: 4 additions & 13 deletions src/controller/controller.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,7 @@

#include "commandhandler.hpp"

#include <unordered_map>

class ControllerChildThread;
class CardEventMonitorThread;

/** Controller coordinates the execution flow and interaction between all other components. */
class Controller : public QObject
Expand All @@ -41,7 +38,9 @@ class Controller : public QObject

signals:
void quit();
void statusUpdate(const RetriableError status);
void retry(const RetriableError error);
void statusUpdate(RetriableError status);
void stopCardEventMonitorThread();

public: // slots
void run();
Expand All @@ -65,29 +64,21 @@ class Controller : public QObject
void onDialogOK(const EidCertificateAndPinInfo& certAndPinInfo);
void onDialogCancel();

// Called when user presses cancel on PIN pad.
void onPinPadCancel();

// Failure handler, reports the error and quits the application.
void onCriticalFailure(const QString& error);

private:
void startCommandExecution();
void runCommandHandler(const std::vector<electronic_id::ElectronicID::ptr>& availableEids);
void connectOkCancelWaitingForPinPad();
void connectRetry(const ControllerChildThread* childThread);
void saveChildThreadPtrAndConnectFailureFinish(ControllerChildThread* childThread);
void stopCardEventMonitorThread();
void createWindow();
void disposeUI();
void exit();
void waitForChildThreads();
CommandType commandType();

CommandWithArgumentsPtr command;
CommandHandler::ptr commandHandler = nullptr;
std::unordered_map<uintptr_t, observer_ptr<ControllerChildThread>> childThreads;
// Key of card event monitor thread in childThreads map.
uintptr_t cardEventMonitorThreadKey = 0;
// As the Qt::WA_DeleteOnClose flag is set, the dialog is deleted automatically.
observer_ptr<WebEidUI> window = nullptr;
QVariantMap _result;
Expand Down
Loading
Loading