Change source identifier type in ProString

The strings remember in which file they were created/assigned. However, this used a non-counting reference to a ProFile, which could become dangling. If a subsequent ProFile re-used the exact same address, a string's source would be mis-identified, which would be fatal in conjunction with discard_from(). Since we actually need only a unique id for comparison, let's use an integer for that. Task-number: QTBUG-62434 Started-by: Simon Hausmann <[email protected]> Change-Id: I395153afaf7c835d0119690ee7f4b915e6f90d4a Reviewed-by: Simon Hausmann <[email protected]>
wangkaigithub · Aug 15, 2017 · 190aa94 · 190aa94
1 parent e6fad35
commit 190aa94
Show file tree

Hide file tree

Showing 7 changed files with 53 additions and 28 deletions.
diff --git a/qmake/library/proitems.cpp b/qmake/library/proitems.cpp
@@ -477,9 +477,10 @@ bool ProStringList::contains(const char *str, Qt::CaseSensitivity cs) const
     return false;
 }
 
-ProFile::ProFile(const QString &fileName)
+ProFile::ProFile(int id, const QString &fileName)
     : m_refCount(1),
       m_fileName(fileName),
+      m_id(id),
       m_ok(true),
       m_hostBuild(false)
 {
@@ -496,7 +497,7 @@ ProString ProFile::getStr(const ushort *&tPtr)
 {
     uint len = *tPtr++;
     ProString ret(items(), tPtr - tokPtr(), len);
-    ret.setSource(this);
+    ret.setSource(m_id);
     tPtr += len;
     return ret;
 }

diff --git a/qmake/library/proitems.h b/qmake/library/proitems.h
@@ -73,8 +73,8 @@ class ProString {
     void setValue(const QString &str);
     void clear() { m_string.clear(); m_length = 0; }
     ProString &setSource(const ProString &other) { m_file = other.m_file; return *this; }
-    ProString &setSource(const ProFile *pro) { m_file = pro; return *this; }
-    const ProFile *sourceFile() const { return m_file; }
+    ProString &setSource(int id) { m_file = id; return *this; }
+    int sourceFile() const { return m_file; }
 
     ProString &prepend(const ProString &other);
     ProString &append(const ProString &other, bool *pending = 0);
@@ -163,7 +163,7 @@ class ProString {
 
     QString m_string;
     int m_offset, m_length;
-    const ProFile *m_file;
+    int m_file;
     mutable uint m_hash;
     QChar *prepareExtend(int extraLen, int thisTarget, int extraTarget);
     uint updatedHash() const;
@@ -340,9 +340,10 @@ enum ProToken {
 class QMAKE_EXPORT ProFile
 {
 public:
-    explicit ProFile(const QString &fileName);
+    ProFile(int id, const QString &fileName);
     ~ProFile();
 
+    int id() const { return m_id; }
     QString fileName() const { return m_fileName; }
     QString directoryName() const { return m_directoryName; }
     const QString &items() const { return m_proitems; }
@@ -367,6 +368,7 @@ class QMAKE_EXPORT ProFile
     QString m_proitems;
     QString m_fileName;
     QString m_directoryName;
+    int m_id;
     bool m_ok;
     bool m_hostBuild;
 };

diff --git a/qmake/library/qmakebuiltins.cpp b/qmake/library/qmakebuiltins.cpp
@@ -713,9 +713,9 @@ QMakeEvaluator::VisitReturn QMakeEvaluator::evaluateBuiltinExpand(
                 after = args[3];
             const ProStringList &var = values(map(args.at(0)));
             if (!var.isEmpty()) {
-                const ProFile *src = currentProFile();
+                int src = currentFileId();
                 for (const ProString &v : var)
-                    if (const ProFile *s = v.sourceFile()) {
+                    if (int s = v.sourceFile()) {
                         src = s;
                         break;
                     }
@@ -1064,7 +1064,7 @@ QMakeEvaluator::VisitReturn QMakeEvaluator::evaluateBuiltinExpand(
                             dirs.append(fname + QLatin1Char('/'));
                     }
                     if (regex.exactMatch(qdir[i]))
-                        ret += ProString(fname).setSource(currentProFile());
+                        ret += ProString(fname).setSource(currentFileId());
                 }
             }
         }
@@ -1331,7 +1331,7 @@ QMakeEvaluator::VisitReturn QMakeEvaluator::evaluateBuiltinConditional(
             return ReturnFalse;
         }
         QString fn = resolvePath(args.at(0).toQString(m_tmp1));
-        ProFile *pro = m_parser->parsedProFile(fn, QMakeParser::ParseOnlyCached);
+        int pro = m_parser->idForFileName(fn);
         if (!pro)
             return ReturnFalse;
         ProValueMap &vmap = m_valuemapStack.first();
@@ -1351,18 +1351,17 @@ QMakeEvaluator::VisitReturn QMakeEvaluator::evaluateBuiltinConditional(
             ++vit;
         }
         for (auto fit = m_functionDefs.testFunctions.begin(); fit != m_functionDefs.testFunctions.end(); ) {
-            if (fit->pro() == pro)
+            if (fit->pro()->id() == pro)
                 fit = m_functionDefs.testFunctions.erase(fit);
             else
                 ++fit;
         }
         for (auto fit = m_functionDefs.replaceFunctions.begin(); fit != m_functionDefs.replaceFunctions.end(); ) {
-            if (fit->pro() == pro)
+            if (fit->pro()->id() == pro)
                 fit = m_functionDefs.replaceFunctions.erase(fit);
             else
                 ++fit;
         }
-        pro->deref();
         ProStringList &iif = m_valuemapStack.first()[ProKey("QMAKE_INTERNAL_INCLUDED_FILES")];
         int idx = iif.indexOf(ProString(fn));
         if (idx >= 0)

diff --git a/qmake/library/qmakeevaluator.cpp b/qmake/library/qmakeevaluator.cpp
@@ -271,13 +271,13 @@ void QMakeEvaluator::skipHashStr(const ushort *&tokPtr)
 
 // FIXME: this should not build new strings for direct sections.
 // Note that the E_SPRINTF and E_LIST implementations rely on the deep copy.
-ProStringList QMakeEvaluator::split_value_list(const QStringRef &vals, const ProFile *source)
+ProStringList QMakeEvaluator::split_value_list(const QStringRef &vals, int source)
 {
     QString build;
     ProStringList ret;
 
     if (!source)
-        source = currentProFile();
+        source = currentFileId();
 
     const QChar *vals_data = vals.data();
     const int vals_len = vals.length();
@@ -1299,7 +1299,7 @@ void QMakeEvaluator::setupProject()
 {
     setTemplate();
     ProValueMap &vars = m_valuemapStack.top();
-    ProFile *proFile = currentProFile();
+    int proFile = currentFileId();
     vars[ProKey("TARGET")] << ProString(QFileInfo(currentFileName()).baseName()).setSource(proFile);
     vars[ProKey("_PRO_FILE_")] << ProString(currentFileName()).setSource(proFile);
     vars[ProKey("_PRO_FILE_PWD_")] << ProString(currentDirectory()).setSource(proFile);
@@ -1593,6 +1593,14 @@ ProFile *QMakeEvaluator::currentProFile() const
     return 0;
 }
 
+int QMakeEvaluator::currentFileId() const
+{
+    ProFile *pro = currentProFile();
+    if (pro)
+        return pro->id();
+    return 0;
+}
+
 QString QMakeEvaluator::currentFileName() const
 {
     ProFile *pro = currentProFile();

diff --git a/qmake/library/qmakeevaluator.h b/qmake/library/qmakeevaluator.h
@@ -176,12 +176,13 @@ class QMAKE_EXPORT QMakeEvaluator
 
     void setTemplate();
 
-    ProStringList split_value_list(const QStringRef &vals, const ProFile *source = 0);
+    ProStringList split_value_list(const QStringRef &vals, int source = 0);
     VisitReturn expandVariableReferences(const ushort *&tokPtr, int sizeHint, ProStringList *ret, bool joined);
 
     QString currentFileName() const;
     QString currentDirectory() const;
     ProFile *currentProFile() const;
+    int currentFileId() const;
     QString resolvePath(const QString &fileName) const
         { return QMakeInternal::IoUtils::resolvePath(currentDirectory(), fileName); }
 

diff --git a/qmake/library/qmakeparser.cpp b/qmake/library/qmakeparser.cpp
@@ -167,7 +167,7 @@ QMakeParser::QMakeParser(ProFileCache *cache, QMakeVfs *vfs, QMakeParserHandler
 ProFile *QMakeParser::parsedProFile(const QString &fileName, ParseFlags flags)
 {
     ProFile *pro;
-    if ((flags & (ParseUseCache|ParseOnlyCached)) && m_cache) {
+    if ((flags & ParseUseCache) && m_cache) {
         ProFileCache::Entry *ent;
 #ifdef PROPARSER_THREAD_SAFE
         QMutexLocker locker(&m_cache->mutex);
@@ -189,13 +189,13 @@ ProFile *QMakeParser::parsedProFile(const QString &fileName, ParseFlags flags)
 #endif
             if ((pro = ent->pro))
                 pro->ref();
-        } else if (!(flags & ParseOnlyCached)) {
+        } else {
             ent = &m_cache->parsed_files[fileName];
 #ifdef PROPARSER_THREAD_SAFE
             ent->locker = new ProFileCache::Entry::Locker;
             locker.unlock();
 #endif
-            pro = new ProFile(fileName);
+            pro = new ProFile(idForFileName(fileName), fileName);
             if (!read(pro, flags)) {
                 delete pro;
                 pro = 0;
@@ -214,29 +214,36 @@ ProFile *QMakeParser::parsedProFile(const QString &fileName, ParseFlags flags)
                 ent->locker = 0;
             }
 #endif
-        } else {
-            pro = 0;
         }
-    } else if (!(flags & ParseOnlyCached)) {
-        pro = new ProFile(fileName);
+    } else {
+        pro = new ProFile(idForFileName(fileName), fileName);
         if (!read(pro, flags)) {
             delete pro;
             pro = 0;
         }
-    } else {
-        pro = 0;
     }
     return pro;
 }
 
 ProFile *QMakeParser::parsedProBlock(
         const QStringRef &contents, const QString &name, int line, SubGrammar grammar)
 {
-    ProFile *pro = new ProFile(name);
+    ProFile *pro = new ProFile(0, name);
     read(pro, contents, line, grammar);
     return pro;
 }
 
+int QMakeParser::idForFileName(const QString &fileName)
+{
+#ifdef PROPARSER_THREAD_SAFE
+    QMutexLocker lck(&fileIdMutex);
+#endif
+    int &place = fileIdMap[fileName];
+    if (!place)
+        place = ++fileIdCounter;
+    return place;
+}
+
 void QMakeParser::discardFileFromCache(const QString &fileName)
 {
     if (m_cache)

diff --git a/qmake/library/qmakeparser.h b/qmake/library/qmakeparser.h
@@ -78,7 +78,6 @@ class QMAKE_EXPORT QMakeParser
     enum ParseFlag {
         ParseDefault = 0,
         ParseUseCache = 1,
-        ParseOnlyCached = 2,
         ParseReportMissing = 4
     };
     Q_DECLARE_FLAGS(ParseFlags, ParseFlag)
@@ -91,6 +90,8 @@ class QMAKE_EXPORT QMakeParser
     ProFile *parsedProBlock(const QStringRef &contents, const QString &name, int line = 0,
                             SubGrammar grammar = FullGrammar);
 
+    int idForFileName(const QString &fileName);
+
     void discardFileFromCache(const QString &fileName);
 
 #ifdef PROPARSER_DEBUG
@@ -181,6 +182,12 @@ class QMAKE_EXPORT QMakeParser
 
     QString m_tmp; // Temporary for efficient toQString
 
+    QHash<QString, int> fileIdMap;
+#ifdef PROEVALUATOR_THREAD_SAFE
+    QMutex fileIdMutex;
+#endif
+    int fileIdCounter = 0;
+
     ProFileCache *m_cache;
     QMakeParserHandler *m_handler;
     QMakeVfs *m_vfs;