QSslSocketPrivate::systemCaCertificates() hangs sometimes on Symbian

The patch fixes the hanging issues on some Symbian devices that occurs while
retrieving certificates from the Symbian certificate store. The hanging was
caused by the certificate info array not being closed before exiting the
thread. This alone wouldn't make the existing implementation work, so the patch
replaces it with a pure Symbian style implementation which doesn't seem to be
affected (probably some OpenC threads issue).

Merge-request: 808
Reviewed-by: Shane Kearns
Reviewed-by: Simon Hausmann <simon.hausmann@nokia.com>
Task: QTBUG-13033

2 files changed, 136 insertions, 134 deletions

diff --git a/src/network/ssl/qsslsocket_openssl.cpp b/src/network/ssl/qsslsocket_openssl.cpp
index 94b1568..5033393 100644
--- a/src/network/ssl/qsslsocket_openssl.cpp
+++ b/src/network/ssl/qsslsocket_openssl.cpp
@@ -71,6 +71,9 @@ QT_BEGIN_NAMESPACE
     PtrCertFindCertificateInStore QSslSocketPrivate::ptrCertFindCertificateInStore = 0;
     PtrCertCloseStore QSslSocketPrivate::ptrCertCloseStore = 0;
 #elif defined(Q_OS_SYMBIAN)
+#include <e32base.h>
+#include <e32std.h>
+#include <e32debug.h>
 #include <QtCore/private/qcore_symbian_p.h>
 #endif
 
@@ -570,120 +573,138 @@ void QSslSocketPrivate::resetDefaultCiphers()
 
 #if defined(Q_OS_SYMBIAN)
 
-QCertificateRetriever::QCertificateRetriever(QCertificateConsumer* parent)
-    : CActive(EPriorityStandard)
-    , certStore(0)
-    , certFilter(0)
-    , consumer(parent)
-    , currentCertificateIndex(0)
-    , certDescriptor(0, 0)
+CSymbianCertificateRetriever::CSymbianCertificateRetriever() : CActive(CActive::EPriorityStandard),
+        iSequenceError(KErrNone)
 {
-    CActiveScheduler::Add(this);
-    QT_TRAP_THROWING(certStore = CUnifiedCertStore::NewL(qt_s60GetRFs(), EFalse));
-    QT_TRAP_THROWING(certFilter = CCertAttributeFilter::NewL());
-    certFilter->SetFormat(EX509Certificate);
 }
 
-QCertificateRetriever::~QCertificateRetriever()
+CSymbianCertificateRetriever::~CSymbianCertificateRetriever()
 {
-    delete certFilter;
-    delete certStore;
-    Cancel();
+    iThread.Close();
 }
 
-void QCertificateRetriever::fetch()
+CSymbianCertificateRetriever* CSymbianCertificateRetriever::NewL()
 {
-    certStore->Initialize(iStatus);
-    state = Initializing;
-    SetActive();
+    CSymbianCertificateRetriever* self = new (ELeave) CSymbianCertificateRetriever();
+    CleanupStack::PushL(self);
+    self->ConstructL();
+    CleanupStack::Pop();
+    return self;
 }
 
-void QCertificateRetriever::list()
+int CSymbianCertificateRetriever::GetCertificates(QList<QByteArray> &certificates)
 {
-    certStore->List(certs, *certFilter, iStatus);
-    state = Listing;
-    SetActive();
+    iCertificates = &certificates;
+
+    TRequestStatus status;
+    iThread.Logon(status);
+    iThread.Resume();
+    User::WaitForRequest(status);
+    if (iThread.ExitType() == EExitKill)
+        return KErrDied;
+    else
+        return status.Int();    // Logon() completes with the thread's exit value
 }
 
-void QCertificateRetriever::retrieveNextCertificate()
+void CSymbianCertificateRetriever::doThreadEntryL()
 {
-    CCTCertInfo* cert = certs[currentCertificateIndex];
-    currentCertificate.resize(cert->Size());
-    certDescriptor.Set((TUint8*)currentCertificate.data(), 0, currentCertificate.size());
-    certStore->Retrieve(*cert, certDescriptor, iStatus);
-    state = RetrievingCertificates;
+    CActiveScheduler* activeScheduler = new (ELeave) CActiveScheduler;
+    CleanupStack::PushL(activeScheduler);
+    CActiveScheduler::Install(activeScheduler);
+
+    CActiveScheduler::Add(this);
+
+    // These aren't deleted in the destructor so leaving the to CS is ok
+    iCertStore = CUnifiedCertStore::NewLC(qt_s60GetRFs(), EFalse);
+    iCertFilter = CCertAttributeFilter::NewLC();
+
+    // Kick off the sequence by initializing the cert store
+    iState = Initializing;
+    iCertStore->Initialize(iStatus);
     SetActive();
-}
 
-void QCertificateRetriever::RunL()
-{
-    QT_TRYCATCH_LEAVING(run());
+    CActiveScheduler::Start();
+
+    // Sequence complete, clean up
+
+    // These MUST be cleaned up before the installed CActiveScheduler is destroyed and can't be left to the
+    // destructor of CSymbianCertificateRetriever. Otherwise the destructor of CActiveScheduler will get
+    // stuck.
+    iCertInfos.Close();
+    CleanupStack::PopAndDestroy(3);     // activeScheduler, iCertStore, iCertFilter
 }
 
-void QCertificateRetriever::run()
+
+TInt CSymbianCertificateRetriever::ThreadEntryPoint(TAny* aParams)
 {
-    switch (state) {
-    case Initializing:
-        list();
-        break;
-    case Listing:
-        currentCertificateIndex = 0;
-        retrieveNextCertificate();
-        break;
-    case RetrievingCertificates:
-        consumer->addEncodedCertificate(currentCertificate);
-        currentCertificate = QByteArray();
+    CTrapCleanup* cleanupStack = CTrapCleanup::New();
 
-        currentCertificateIndex++;
+    CSymbianCertificateRetriever* self = (CSymbianCertificateRetriever*) aParams;
+    TRAPD(err, self->doThreadEntryL());
+    delete cleanupStack;
 
-        if (currentCertificateIndex < certs.Count())
-            retrieveNextCertificate();
-        else
-            consumer->finish();
-        break;
-    }
+    // doThreadEntryL() can leave only before the retrieval sequence is started
+    if (err)
+        return err;
+    else
+        return self->iSequenceError;    // return any error that occured during the retrieval
 }
 
-void QCertificateRetriever::DoCancel()
+void CSymbianCertificateRetriever::ConstructL()
 {
-    switch (state) {
-    case Initializing:
-        certStore->CancelInitialize();
-        break;
-    case Listing:
-        certStore->CancelList();
-        break;
-    case RetrievingCertificates:
-        certStore->CancelRetrieve();
-        break;
-    }
+    User::LeaveIfError(iThread.Create(_L("CertWorkerThread"),
+        CSymbianCertificateRetriever::ThreadEntryPoint, 16384, NULL, this));
 }
 
-QCertificateConsumer::QCertificateConsumer(QObject* parent)
-    : QObject(parent)
-    , retriever(0)
+void CSymbianCertificateRetriever::DoCancel()
 {
+    // We never cancel the sequence
 }
 
-QCertificateConsumer::~QCertificateConsumer()
+TInt CSymbianCertificateRetriever::RunError(TInt aError)
 {
-    delete retriever;
+    // If something goes wrong in the sequence, abort the sequence
+    iSequenceError = aError;    // this gets reported to the client in the TRequestStatus
+    CActiveScheduler::Stop();
+    return KErrNone;
 }
 
-void QCertificateConsumer::finish()
+void CSymbianCertificateRetriever::GetCertificateL()
 {
-    delete retriever;
-    retriever = 0;
-    emit finished();
+    CCTCertInfo* certInfo = iCertInfos[iCurrentCertIndex];
+    iCertificateData.resize(certInfo->Size());
+    TPtr8 des((TUint8*)iCertificateData.data(), 0, iCertificateData.size());
+    iCertStore->Retrieve(*certInfo, des, iStatus);
+    iState = RetrievingCertificates;
+    SetActive();
 }
 
-void QCertificateConsumer::start()
+void CSymbianCertificateRetriever::RunL()
 {
-    retriever = new QCertificateRetriever(this);
-    Q_CHECK_PTR(retriever);
-    retriever->fetch();
-}
+    switch (iState) {
+    case Initializing:
+        iState = Listing;
+        iCertStore->List(iCertInfos, *iCertFilter, iStatus);
+        SetActive();
+        break;
 
+    case Listing:
+        iCurrentCertIndex = 0;
+        GetCertificateL();
+        break;
+
+    case RetrievingCertificates:
+        iCertificates->append(iCertificateData);
+        iCertificateData = QByteArray();
+        iCurrentCertIndex++;
+        if (iCurrentCertIndex < iCertInfos.Count())
+            GetCertificateL();
+        else
+            // Stop the scheduler to return to the thread entry function
+            CActiveScheduler::Stop();
+        break;
+    }
+}
 #endif // defined(Q_OS_SYMBIAN)
 
 QList<QSslCertificate> QSslSocketPrivate::systemCaCertificates()
@@ -760,23 +781,15 @@ QList<QSslCertificate> QSslSocketPrivate::systemCaCertificates()
     systemCerts.append(QSslCertificate::fromPath(QLatin1String("/usr/share/ssl/*.pem"), QSsl::Pem, QRegExp::Wildcard)); // Centos, Redhat, SuSE
     systemCerts.append(QSslCertificate::fromPath(QLatin1String("/usr/local/ssl/*.pem"), QSsl::Pem, QRegExp::Wildcard)); // Normal OpenSSL Tarball
 #elif defined(Q_OS_SYMBIAN)
-    QThread* certThread = new QThread;
+    QList<QByteArray> certs;
+    QScopedPointer<CSymbianCertificateRetriever> retriever(CSymbianCertificateRetriever::NewL());
 
-    QCertificateConsumer *consumer = new QCertificateConsumer();
-    consumer->moveToThread(certThread);
-    QObject::connect(certThread, SIGNAL(started()), consumer, SLOT(start()));
-    QObject::connect(consumer, SIGNAL(finished()), certThread, SLOT(quit()), Qt::DirectConnection);
-
-    certThread->start();
-    certThread->wait();
-    foreach (const QByteArray &encodedCert, consumer->encodedCertificates()) {
+    retriever->GetCertificates(certs);
+    foreach (const QByteArray &encodedCert, certs) {
         QSslCertificate cert(encodedCert, QSsl::Der);
         if (!cert.isNull())
             systemCerts.append(cert);
     }
-
-    delete consumer;
-    delete certThread;
 #endif
 
     return systemCerts;
diff --git a/src/network/ssl/qsslsocket_openssl_p.h b/src/network/ssl/qsslsocket_openssl_p.h
index 7a4e6b6..dec98ae 100644
--- a/src/network/ssl/qsslsocket_openssl_p.h
+++ b/src/network/ssl/qsslsocket_openssl_p.h
@@ -120,67 +120,56 @@ public:
 };
 
 #if defined(Q_OS_SYMBIAN)
+
+#include <QByteArray>
+#include <e32base.h>
+#include <f32file.h>
 #include <unifiedcertstore.h>     // link against certstore.lib
 #include <ccertattributefilter.h> // link against ctframework.lib
 
-class QCertificateRetriever;
+// The purpose of this class is to wrap the asynchronous API of Symbian certificate store to one
+// synchronizable call. The user of this class needs to provide a TRequestStatus object which can
+// be used with User::WaitForRequest() unlike with the calls of the certificate store API.
+// A thread is used instead of a CActiveSchedulerWait scheme, because that would make the call
+// asynchronous (other events might be processed during the call even though the call would be seemingly
+// synchronous).
 
-class QCertificateConsumer : public QObject
+class CSymbianCertificateRetriever : public CActive
 {
-    Q_OBJECT
 public:
-    QCertificateConsumer(QObject* parent = 0);
-    ~QCertificateConsumer();
-
-    void finish();
-
-    void addEncodedCertificate(const QByteArray& certificate)
-    { certificates.append(certificate); }
-    QList<QByteArray> encodedCertificates() const { return certificates; }
-
-public slots:
-    void start();
+    static CSymbianCertificateRetriever* NewL();
+    ~CSymbianCertificateRetriever();
 
-signals:
-    void finished();
+    int GetCertificates(QList<QByteArray> &aCertificates);
 
 private:
-    QList<QByteArray> certificates;
-    QCertificateRetriever *retriever;
-};
-
-
-class QCertificateRetriever : public CActive
-{
-public:
-    QCertificateRetriever(QCertificateConsumer* consumer);
-    ~QCertificateRetriever();
-
-    void fetch();
+    void ConstructL();
+    CSymbianCertificateRetriever();
+    static TInt ThreadEntryPoint(TAny* aParams);
+    void doThreadEntryL();
+    void GetCertificateL();
+    void DoCancel();
+    void RunL();
+    TInt RunError(TInt aError);
 
 private:
-    virtual void RunL();
-    virtual void DoCancel();
-
-    void run();
-    void list();
-    void retrieveNextCertificate();
-
     enum {
         Initializing,
         Listing,
         RetrievingCertificates
-    } state;
-
-    CUnifiedCertStore* certStore;
-    RMPointerArray<CCTCertInfo> certs;
-    CCertAttributeFilter* certFilter;
-    QCertificateConsumer* consumer;
-    int currentCertificateIndex;
-    QByteArray currentCertificate;
-    TPtr8 certDescriptor;
+    } iState;
+
+    RThread iThread;
+    CUnifiedCertStore* iCertStore;
+    RMPointerArray<CCTCertInfo> iCertInfos;
+    CCertAttributeFilter* iCertFilter;
+    TInt iCurrentCertIndex;
+    QByteArray iCertificateData;
+    QList<QByteArray>* iCertificates;
+    TInt iSequenceError;
 };
 
+
 #endif