Proper error handling for reading databases.

src/format/KeePass2Reader.cpp

@@ -38,7 +38,8 @@ KeePass2Reader::KeePass2Reader()
 
 Database* KeePass2Reader::readDatabase(QIODevice* device, const CompositeKey& key)
 {
-    m_db = new Database();
+    QScopedPointer<Database> db(new Database());
+    m_db = db.data();
     m_device = device;
     m_error = false;
     m_errorStr = QString();
@@ -48,13 +49,13 @@ Database* KeePass2Reader::readDatabase(QIODevice* device, const CompositeKey& ke
 
     quint32 signature1 = Endian::readUInt32(m_device, KeePass2::BYTEORDER, &ok);
     if (!ok || signature1 != KeePass2::SIGNATURE_1) {
-        raiseError("1");
+        raiseError(tr("Not a KeePass database."));
         return 0;
     }
 
     quint32 signature2 = Endian::readUInt32(m_device, KeePass2::BYTEORDER, &ok);
     if (!ok || signature2 != KeePass2::SIGNATURE_2) {
-        raiseError("2");
+        raiseError(tr("Not a KeePass database."));
         return 0;
     }
 
@@ -62,11 +63,11 @@ Database* KeePass2Reader::readDatabase(QIODevice* device, const CompositeKey& ke
     quint32 expectedVersion = KeePass2::FILE_VERSION & KeePass2::FILE_VERSION_CRITICAL_MASK;
     // TODO do we support old Kdbx versions?
     if (!ok || (version != expectedVersion)) {
-        raiseError("3");
+        raiseError(tr("Unsupported KeePass database version."));
         return 0;
     }
 
-    while (readHeaderField() && !error()) {
+    while (readHeaderField() && !hasError()) {
     }
 
     // TODO check if all header fields have been parsed
@@ -84,7 +85,7 @@ Database* KeePass2Reader::readDatabase(QIODevice* device, const CompositeKey& ke
     QByteArray realStart = cipherStream.read(32);
 
     if (realStart != m_streamStartBytes) {
-        raiseError("4");
+        raiseError(tr("Wrong key or database file is corrupt."));
         return 0;
     }
 
@@ -117,28 +118,41 @@ Database* KeePass2Reader::readDatabase(QIODevice* device, const CompositeKey& ke
 
     KeePass2XmlReader xmlReader;
     xmlReader.readDatabase(xmlDevice, m_db, &randomStream);
-    // TODO forward error messages from xmlReader
+
-    return m_db;
+    if (xmlReader.hasError()) {
+        raiseError(xmlReader.errorString());
+        return 0;
+    }
+
+    return db.take();
 }
 
 Database* KeePass2Reader::readDatabase(const QString& filename, const CompositeKey& key)
 {
     QFile file(filename);
-    file.open(QFile::ReadOnly);
+    if (!file.open(QFile::ReadOnly)) {
-    Database* db = readDatabase(&file, key);
+        raiseError(file.errorString());
-    // TODO check for QFile errors
+        return 0;
-    return db;
+    }
+
+    QScopedPointer<Database> db(readDatabase(&file, key));
+
+    if (file.error() != QFile::NoError) {
+        raiseError(file.errorString());
+        return 0;
+    }
+
+    return db.take();
 }
 
-bool KeePass2Reader::error()
+bool KeePass2Reader::hasError()
 {
     return m_error;
 }
 
 QString KeePass2Reader::errorString()
 {
-    // TODO
+    return m_errorStr;
-    return QString();
 }
 
 void KeePass2Reader::setSaveXml(bool save)
@@ -153,9 +167,8 @@ QByteArray KeePass2Reader::xmlData()
 
 void KeePass2Reader::raiseError(const QString& str)
 {
-    // TODO
-    qWarning("KeePass2Reader error: %s", qPrintable(str));
     m_error = true;
+    m_errorStr = str;
 }
 
 bool KeePass2Reader::readHeaderField()

src/format/KeePass2Reader.h

@@ -33,7 +33,7 @@ public:
     KeePass2Reader();
     Database* readDatabase(QIODevice* device, const CompositeKey& key);
     Database* readDatabase(const QString& filename, const CompositeKey& key);
-    bool error();
+    bool hasError();
     QString errorString();
     void setSaveXml(bool save);
     QByteArray xmlData();

src/format/KeePass2XmlReader.cpp

@@ -34,6 +34,7 @@ KeePass2XmlReader::KeePass2XmlReader()
 
 void KeePass2XmlReader::readDatabase(QIODevice* device, Database* db, KeePass2RandomStream* randomStream)
 {
+    m_xml.clear();
     m_xml.setDevice(device);
 
     m_db = db;
@@ -48,9 +49,9 @@ void KeePass2XmlReader::readDatabase(QIODevice* device, Database* db, KeePass2Ra
         }
     }
 
-    // TODO check if m_tmpParent doesn't have entries
     if (!m_xml.error() && !m_tmpParent->children().isEmpty()) {
-        raiseError();
+        qWarning("KeePass2XmlReader::readDatabase: found %d invalid entry references",
+                 m_tmpParent->children().size());
     }
 
     delete m_tmpParent;
@@ -70,7 +71,7 @@ Database* KeePass2XmlReader::readDatabase(const QString& filename)
     return readDatabase(&file);
 }
 
-bool KeePass2XmlReader::error()
+bool KeePass2XmlReader::hasError()
 {
     return m_xml.hasError();
 }
@@ -300,7 +301,7 @@ Group* KeePass2XmlReader::parseGroup()
         if (m_xml.name() == "UUID") {
             Uuid uuid = readUuid();
             if (uuid.isNull()) {
-                raiseError();
+                raiseError(1);
             }
             else {
                 group = getGroup(uuid);
@@ -315,7 +316,7 @@ Group* KeePass2XmlReader::parseGroup()
         else if (m_xml.name() == "IconID") {
             int iconId = readNumber();
             if (iconId < 0) {
-                raiseError();
+                raiseError(2);
             }
             else {
                 if (iconId >= databaseIcons()->iconCount()) {
@@ -352,7 +353,7 @@ Group* KeePass2XmlReader::parseGroup()
                 group->setAutoTypeEnabled(Group::Disable);
             }
             else {
-                raiseError();
+                raiseError(3);
             }
 
         }
@@ -369,7 +370,7 @@ Group* KeePass2XmlReader::parseGroup()
                 group->setSearchingEnabled(Group::Disable);
             }
             else {
-                raiseError();
+                raiseError(4);
             }
         }
         else if (m_xml.name() == "LastTopVisibleEntry") {
@@ -419,7 +420,7 @@ void KeePass2XmlReader::parseDeletedObject()
         if (m_xml.name() == "UUID") {
             Uuid uuid = readUuid();
             if (uuid.isNull()) {
-                raiseError();
+                raiseError(5);
             }
             else {
                 delObj.uuid = uuid;
@@ -445,7 +446,7 @@ Entry* KeePass2XmlReader::parseEntry(bool history)
         if (m_xml.name() == "UUID") {
             Uuid uuid = readUuid();
             if (uuid.isNull()) {
-                raiseError();
+                raiseError(6);
             }
             else {
                 if (history) {
@@ -460,7 +461,7 @@ Entry* KeePass2XmlReader::parseEntry(bool history)
         else if (m_xml.name() == "IconID") {
             int iconId = readNumber();
             if (iconId < 0) {
-                raiseError();
+                raiseError(7);
             }
             else {
                 entry->setIcon(iconId);
@@ -498,7 +499,7 @@ Entry* KeePass2XmlReader::parseEntry(bool history)
         }
         else if (m_xml.name() == "History") {
             if (history) {
-                raiseError();
+                raiseError(8);
             }
             else {
                 parseEntryHistory(entry);
@@ -532,7 +533,7 @@ void KeePass2XmlReader::parseEntryString(Entry *entry)
                     value = m_randomStream->process(QByteArray::fromBase64(value.toAscii()));
                 }
                 else {
-                    raiseError();
+                    raiseError(9);
                 }
             }
 
@@ -679,7 +680,7 @@ bool KeePass2XmlReader::readBool()
         return false;
     }
     else {
-        raiseError();
+        raiseError(10);
         return false;
     }
 }
@@ -690,7 +691,7 @@ QDateTime KeePass2XmlReader::readDateTime()
     QDateTime dt = QDateTime::fromString(str, Qt::ISODate);
 
     if (!dt.isValid()) {
-        raiseError();
+        raiseError(11);
     }
 
     return dt;
@@ -705,7 +706,7 @@ QColor KeePass2XmlReader::readColor()
     }
 
     if (colorStr.length() != 7 || colorStr[0] != '#') {
-        raiseError();
+        raiseError(12);
         return QColor();
     }
 
@@ -715,7 +716,7 @@ QColor KeePass2XmlReader::readColor()
         bool ok;
         int rgbPart = rgbPartStr.toInt(&ok, 16);
         if (!ok || rgbPart > 255) {
-            raiseError();
+            raiseError(13);
             return QColor();
         }
 
@@ -738,7 +739,7 @@ int KeePass2XmlReader::readNumber()
     bool ok;
     int result = readString().toInt(&ok);
     if (!ok) {
-        raiseError();
+        raiseError(14);
     }
     return result;
 }
@@ -747,7 +748,7 @@ Uuid KeePass2XmlReader::readUuid()
 {
     QByteArray uuidBin = readBinary();
     if (uuidBin.length() != Uuid::LENGTH) {
-        raiseError();
+        raiseError(15);
         return Uuid();
     }
     else {
@@ -798,9 +799,9 @@ Entry* KeePass2XmlReader::getEntry(const Uuid& uuid)
     return entry;
 }
 
-void KeePass2XmlReader::raiseError()
+void KeePass2XmlReader::raiseError(int internalNumber)
 {
-    m_xml.raiseError(tr("Invalid database file"));
+    m_xml.raiseError(tr("Invalid database file (error no %1).").arg(QString::number(internalNumber)));
 }
 
 void KeePass2XmlReader::skipCurrentElement()

src/format/KeePass2XmlReader.h

@@ -42,7 +42,7 @@ public:
     Database* readDatabase(QIODevice* device);
     void readDatabase(QIODevice* device, Database* db, KeePass2RandomStream* randomStream = 0);
     Database* readDatabase(const QString& filename);
-    bool error();
+    bool hasError();
     QString errorString();
 
 private:
@@ -75,7 +75,7 @@ private:
 
     Group* getGroup(const Uuid& uuid);
     Entry* getEntry(const Uuid& uuid);
-    void raiseError();
+    void raiseError(int internalNumber);
     void skipCurrentElement();
 
     QXmlStreamReader m_xml;

tests/TestKeePass2Reader.cpp

@@ -41,7 +41,7 @@ void TestKeePass2Reader::testNonAscii()
     KeePass2Reader* reader = new KeePass2Reader();
     Database* db = reader->readDatabase(filename, key);
     QVERIFY(db);
-    QVERIFY(!reader->error());
+    QVERIFY(!reader->hasError());
     QCOMPARE(db->metadata()->name(), QString("NonAsciiTest"));
 
     delete db;
@@ -56,7 +56,7 @@ void TestKeePass2Reader::testCompressed()
     KeePass2Reader* reader = new KeePass2Reader();
     Database* db = reader->readDatabase(filename, key);
     QVERIFY(db);
-    QVERIFY(!reader->error());
+    QVERIFY(!reader->hasError());
     QCOMPARE(db->metadata()->name(), QString("Compressed"));
 
     delete db;
@@ -71,7 +71,7 @@ void TestKeePass2Reader::testProtectedStrings()
     KeePass2Reader* reader = new KeePass2Reader();
     Database* db = reader->readDatabase(filename, key);
     QVERIFY(db);
-    QVERIFY(!reader->error());
+    QVERIFY(!reader->hasError());
     QCOMPARE(db->metadata()->name(), QString("Protected Strings Test"));
 
     Entry* entry = db->rootGroup()->entries().at(0);

tests/TestKeePass2Writer.cpp

@@ -60,7 +60,7 @@ void TestKeePass2Writer::initTestCase()
     buffer.seek(0);
     KeePass2Reader reader;
     m_dbTest = reader.readDatabase(&buffer, key);
-    QVERIFY(!reader.error());
+    QVERIFY(!reader.hasError());
     QVERIFY(m_dbTest);
 }
 

tests/TestKeePass2XmlReader.cpp

@@ -71,7 +71,7 @@ void TestKeePass2XmlReader::initTestCase()
     QString xmlFile = QString(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.xml");
     m_db = reader->readDatabase(xmlFile);
     QVERIFY(m_db);
-    QVERIFY(!reader->error());
+    QVERIFY(!reader->hasError());
 }
 
 void TestKeePass2XmlReader::testMetadata()

tests/TestKeys.cpp

@@ -77,7 +77,7 @@ void TestKeys::testFileKey()
 
     Database* db = reader.readDatabase(dbFilename, compositeKey);
     QVERIFY(db);
-    QVERIFY(!reader.error());
+    QVERIFY(!reader.hasError());
     QCOMPARE(db->metadata()->name(), QString("%1 Database").arg(name));
 
     delete db;
@@ -122,7 +122,7 @@ void TestKeys::testCreateFileKey()
     KeePass2Reader reader;
     Database* dbRead = reader.readDatabase(&dbBuffer, compositeKey);
     QVERIFY(dbRead);
-    QVERIFY(!reader.error());
+    QVERIFY(!reader.hasError());
     QCOMPARE(dbRead->metadata()->name(), dbName);
     delete dbRead;
 }

utils/kdbx-extract.cpp

@@ -57,7 +57,7 @@ int main(int argc, char **argv)
     reader.setSaveXml(true);
     reader.readDatabase(&dbFile, key);
 
-    if (reader.error()) {
+    if (reader.hasError()) {
         qCritical("Error while reading the database.");
         return 1;
     }