From cbe8c1014621e863c95ffc856d87a3cff12ec608 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Jo=C3=A3o=20Abecasis?= Date: Tue, 27 Mar 2012 17:32:43 +0200 Subject: [PATCH] Clean up and make robust the file loading code The used_mmap variable was set to true the first time an mmap operation was successful, but it was never reset back to false. While that can be a good indicator that future calls might succeed it is not a guarantee. Not properly resetting could mean we'd unmap memory allocated with new, instead of deleting it. Since that variable is only used inside defined(QT_USE_MMAP) blocks, its declaration is scoped the same way. While mmap is still handled "by hand", use QFile for the other operations. Calling mmap here is less than ideal, as it prevents use of other memory mapping methods, such as native Windows APIs, but is less intrusive as it allows QTranslator to retain control over lifetime of the map. Using QFile for remaining operations reduces the number of filesystem operations. The file size is now checked to be minimally sane (<4GB), the limit of the 32-bit variable that will hold mapping's length. Translation files should be expected to be much smaller in practice, but there isn't a sane hard-limit. The file format is broken down to sections, each of which has a 32-bit length. Finally, when loading a file fails, release resources immediately, instead of delaying to next load attempt or the destructor. Change-Id: I5cc1b626a99d229e8861eb0fbafc42b928b6a122 Reviewed-by: Oswald Buddenhagen --- src/corelib/kernel/qtranslator.cpp | 88 ++++++++++++++++++++++---------------- 1 file changed, 51 insertions(+), 37 deletions(-) diff --git a/src/corelib/kernel/qtranslator.cpp b/src/corelib/kernel/qtranslator.cpp index 02cd85f..3e77465 100644 --- a/src/corelib/kernel/qtranslator.cpp +++ b/src/corelib/kernel/qtranslator.cpp @@ -227,15 +227,20 @@ class QTranslatorPrivate : public QObjectPrivate public: enum { Contexts = 0x2f, Hashes = 0x42, Messages = 0x69, NumerusRules = 0x88 }; - QTranslatorPrivate() - : used_mmap(0), unmapPointer(0), unmapLength(0), + QTranslatorPrivate() : +#if defined(QT_USE_MMAP) + used_mmap(0), +#endif + unmapPointer(0), unmapLength(0), messageArray(0), offsetArray(0), contextArray(0), numerusRulesArray(0), messageLength(0), offsetLength(0), contextLength(0), numerusRulesLength(0) {} // for mmap'ed files, this is what needs to be unmapped. +#if defined(QT_USE_MMAP) bool used_mmap : 1; +#endif char *unmapPointer; - unsigned int unmapLength; + quint32 unmapLength; // for squeezed but non-file data, this is what needs to be deleted const uchar *messageArray; @@ -448,6 +453,15 @@ bool QTranslatorPrivate::do_load(const QString &realname) QTranslatorPrivate *d = this; bool ok = false; + QFile file(realname); + if (!file.open(QIODevice::ReadOnly)) + return false; + + qint64 fileSize = file.size(); + if (!fileSize || quint32(-1) <= fileSize) + return false; + d->unmapLength = quint32(fileSize); + #ifdef QT_USE_MMAP #ifndef MAP_FILE @@ -457,48 +471,47 @@ bool QTranslatorPrivate::do_load(const QString &realname) #define MAP_FAILED -1 #endif - int fd = -1; - if (!realname.startsWith(QLatin1Char(':'))) - fd = QT_OPEN(QFile::encodeName(realname), O_RDONLY); + int fd = file.handle(); if (fd >= 0) { - QT_STATBUF st; - if (!QT_FSTAT(fd, &st)) { - char *ptr; - ptr = reinterpret_cast( - mmap(0, st.st_size, // any address, whole file - PROT_READ, // read-only memory - MAP_FILE | MAP_PRIVATE, // swap-backed map from file - fd, 0)); // from offset 0 of fd - if (ptr && ptr != reinterpret_cast(MAP_FAILED)) { - d->used_mmap = true; - d->unmapPointer = ptr; - d->unmapLength = st.st_size; - ok = true; - } + char *ptr; + ptr = reinterpret_cast( + mmap(0, d->unmapLength, // any address, whole file + PROT_READ, // read-only memory + MAP_FILE | MAP_PRIVATE, // swap-backed map from file + fd, 0)); // from offset 0 of fd + if (ptr && ptr != reinterpret_cast(MAP_FAILED)) { + file.close(); + d->used_mmap = true; + d->unmapPointer = ptr; + ok = true; } - ::close(fd); } #endif // QT_USE_MMAP if (!ok) { - QFile file(realname); - d->unmapLength = file.size(); - if (!d->unmapLength) - return false; d->unmapPointer = new char[d->unmapLength]; - - if (file.open(QIODevice::ReadOnly)) - ok = (d->unmapLength == (uint)file.read(d->unmapPointer, d->unmapLength)); - - if (!ok) { - delete [] d->unmapPointer; - d->unmapPointer = 0; - d->unmapLength = 0; - return false; + if (d->unmapPointer) { + qint64 readResult = file.read(d->unmapPointer, d->unmapLength); + if (readResult == qint64(unmapLength)) + ok = true; } } - return d->do_load(reinterpret_cast(d->unmapPointer), d->unmapLength); + if (ok && d->do_load(reinterpret_cast(d->unmapPointer), d->unmapLength)) + return true; + +#if defined(QT_USE_MMAP) + if (used_mmap) { + used_mmap = false; + munmap(unmapPointer, unmapLength); + } else +#endif + delete [] unmapPointer; + + d->unmapPointer = 0; + d->unmapLength = 0; + + return false; } static QString find_translation(const QLocale & locale, @@ -900,9 +913,10 @@ void QTranslatorPrivate::clear() Q_Q(QTranslator); if (unmapPointer && unmapLength) { #if defined(QT_USE_MMAP) - if (used_mmap) + if (used_mmap) { + used_mmap = false; munmap(unmapPointer, unmapLength); - else + } else #endif delete [] unmapPointer; } -- 2.7.4