lp1744550: Fix use-after-free (and more serious issues) in GlobalTrackCache

src/encoder/encoderffmpegcore.cpp

@@ -31,7 +31,6 @@ EncoderFfmpegCore::EncoderFfmpegCore(EncoderCallback* pCallback, CodecID codec)
 {
     m_bStreamInitialized = false;
     m_pCallback = pCallback;
-    m_pMetaData = TrackPointer(NULL);
 
     m_pEncodeFormatCtx = NULL;
     m_pEncoderAudioStream = NULL;

src/library/autodj/autodjfeature.cpp

@@ -278,7 +278,7 @@ void AutoDJFeature::onRightClickChild(const QPoint& globalPos,
         Crate crate;
         while (nonAutoDjCrates.populateNext(&crate)) {
             auto pAction = std::make_unique<QAction>(crate.getName(), &crateMenu);
-            m_crateMapper.setMapping(pAction.get(), crate.getId().toInt());
+            m_crateMapper.setMapping(pAction.get(), crate.getId().value());
             connect(pAction.get(), SIGNAL(triggered()), &m_crateMapper, SLOT(map()));
             crateMenu.addAction(pAction.get());
             pAction.release();

src/library/basetrackcache.cpp

@@ -141,31 +141,26 @@ void BaseTrackCache::setSearchColumns(const QStringList& columns) {
     m_searchColumns = columns;
 }
 
-TrackPointer BaseTrackCache::getRecentTrack(TrackId trackId) const {
+const TrackPointer& BaseTrackCache::getRecentTrack(TrackId trackId) const {
     DEBUG_ASSERT(m_bIsCaching);
     // Only refresh the recently used track if the identifiers
     // don't match. Otherwise simply return the corresponding
     // pointer to avoid accessing and locking the global track
     // cache excessively.
     if (m_recentTrackId != trackId) {
-        refreshRecentTrack(std::move(trackId));
+        if (trackId.isValid()) {
+            TrackPointer trackPtr =
+                    GlobalTrackCacheLocker().lookupTrackById(trackId);
+            replaceRecentTrack(
+                    std::move(trackId),
+                    std::move(trackPtr));
+        } else {
+            resetRecentTrack();
+        }
     }
     return m_recentTrackPtr;
 }
 
-void BaseTrackCache::refreshRecentTrack(TrackId trackId) const {
-    DEBUG_ASSERT(m_bIsCaching);
-    if (trackId.isValid()) {
-        auto trackPtr =
-                GlobalTrackCache::instance().lookupById(trackId).getTrack();
-        replaceRecentTrack(
-                std::move(trackId),
-                std::move(trackPtr));
-    } else {
-        resetRecentTrack();
-    }
-}
-
 void BaseTrackCache::replaceRecentTrack(TrackPointer pTrack) const {
     DEBUG_ASSERT(m_bIsCaching);
     DEBUG_ASSERT(pTrack);
@@ -180,7 +175,27 @@ void BaseTrackCache::replaceRecentTrack(TrackId trackId, TrackPointer pTrack) co
     if (m_recentTrackId.isValid()) {
         if (pTrack) {
             DEBUG_ASSERT(m_recentTrackId == pTrack->getId());
-            m_recentTrackPtr = std::move(pTrack);
+
+            // NOTE(uklotzde, 2018-02-07, Fedora 27, GCC 7.3.1, optimize=native (-O3), Core i5-6440HQ)
+            // Using the move assignment operator here will sooner or later
+            // store a nullptr in m_recentTrackPtr even if both m_recentTrackPtr
+            // and pTrack contain a valid but different pointer before the
+            // assignment and the custom deleter is invoked on m_recentTrackPtr
+            // during the assignment. The issue does not occur when either
+            // changing the optimization level from 'native' to 'none' or
+            // when replacing the move assignment with a swap operation (see below).
+            //
+            // Fucked up by the optimizer:
+            // m_recentTrackPtr = std::move(pTrack);
+            //
+            // The workaround:
+            m_recentTrackPtr.swap(pTrack);
+            //
+            // The following debug assertion will be triggered eventually
+            // without the workaround in place when browsing and editing
+            // properties of tracks.
+            DEBUG_ASSERT(m_recentTrackPtr);
+
             if (m_recentTrackPtr->isDirty()) {
                 m_dirtyTracks.insert(m_recentTrackId);
             } else {
@@ -316,8 +331,8 @@ void BaseTrackCache::updateTrackInIndex(TrackId trackId) {
     updateTracksInIndex(trackIds);
 }
 
-void BaseTrackCache::updateTracksInIndex(QSet<TrackId> trackIds) {
-    if (trackIds.size() == 0) {
+void BaseTrackCache::updateTracksInIndex(const QSet<TrackId>& trackIds) {
+    if (trackIds.isEmpty()) {
         return;
     }
 

src/library/basetrackcache.h

@@ -87,16 +87,15 @@ class BaseTrackCache : public QObject {
     void slotDbTrackAdded(TrackPointer pTrack);
 
   private:
-    TrackPointer getRecentTrack(TrackId trackId) const;
-    void refreshRecentTrack(TrackId trackId) const;
+    const TrackPointer& getRecentTrack(TrackId trackId) const;
     void replaceRecentTrack(TrackPointer pTrack) const;
     void replaceRecentTrack(TrackId trackId, TrackPointer pTrack) const;
     void resetRecentTrack() const;
 
     bool updateIndexWithQuery(const QString& query);
     bool updateIndexWithTrackpointer(TrackPointer pTrack);
     void updateTrackInIndex(TrackId trackId);
-    void updateTracksInIndex(QSet<TrackId> trackIds);
+    void updateTracksInIndex(const QSet<TrackId>& trackIds);
     void getTrackValueForColumn(TrackPointer pTrack, int column,
                                 QVariant& trackValue) const;
 

src/library/browse/browsetablemodel.cpp

@@ -345,10 +345,11 @@ void BrowseTableModel::trackLoaded(QString group, TrackPointer pTrack) {
             }
         }
         if (pTrack) {
+            QString trackLocation = pTrack->getLocation();
             for (int row = 0; row < rowCount(); ++row) {
                 QModelIndex i = index(row, COLUMN_PREVIEW);
                 QString location = getTrackLocation(i);
-                if (location == pTrack->getLocation()) {
+                if (location == trackLocation) {
                     QStandardItem* item = itemFromIndex(i);
                     item->setText("1");
                     break;

src/library/browse/browsethread.cpp

@@ -10,7 +10,6 @@
 #include "library/browse/browsetablemodel.h"
 
 #include "sources/soundsourceproxy.h"
-#include "track/globaltrackcache.h"
 #include "util/trace.h"
 
 
@@ -154,17 +153,10 @@ void BrowseThread::populateModel() {
 
         const QString filepath = fileIt.next();
         {
-            TrackPointer pTrack =
-                GlobalTrackCache::instance().resolve(
-                        filepath, thisPath.token()).getTrack();
-            // The GlobalTrackCache is unlocked instantly even if a new track object
-            // has been created and inserted into the cache. Newly created track
-            // objects will only contain a reference of the corresponding file,
-            // but not any metadata, yet. This reduces lock contention on the
-            // global track cache.
-
-            // Update the track object by (re-)importing metadata from the file
-            SoundSourceProxy(pTrack).updateTrackFromSource();
+            const TrackPointer pTrack =
+                    SoundSourceProxy::importTemporaryTrack(
+                            filepath,
+                            thisPath.token());
 
             item = new QStandardItem(pTrack->getFileName());
             item->setToolTip(item->text());

src/library/coverartutils.cpp

@@ -27,18 +27,19 @@ QString CoverArtUtils::supportedCoverArtExtensionsRegex() {
 
 //static
 QImage CoverArtUtils::extractEmbeddedCover(
-        const QFileInfo& fileInfo) {
-    SecurityTokenPointer pToken(Sandbox::openSecurityToken(
-        QFileInfo(fileInfo), true));
-    return extractEmbeddedCover(fileInfo, pToken);
+        QFileInfo fileInfo) {
+    SecurityTokenPointer pToken = Sandbox::openSecurityToken(
+            QFileInfo(fileInfo), true);
+    return extractEmbeddedCover(
+            std::move(fileInfo), std::move(pToken));
 }
 
 //static
 QImage CoverArtUtils::extractEmbeddedCover(
-        const QFileInfo& fileInfo,
+        QFileInfo fileInfo,
         SecurityTokenPointer pToken) {
-    auto pTrack = Track::newTemporary(fileInfo, pToken);
-    return SoundSourceProxy(pTrack).importCoverImage();
+    return SoundSourceProxy::importTemporaryCoverImage(
+            std::move(fileInfo), std::move(pToken));
 }
 
 //static

src/library/coverartutils.h

@@ -21,9 +21,9 @@ class CoverArtUtils {
     // Extracts the first cover art image embedded within the file at
     // fileInfo. If no security token is provided a new one is created.
     static QImage extractEmbeddedCover(
-            const QFileInfo& fileInfo);
+            QFileInfo fileInfo);
     static QImage extractEmbeddedCover(
-            const QFileInfo& fileInfo,
+            QFileInfo fileInfo,
             SecurityTokenPointer pToken);
 
     static QImage loadCover(const CoverInfo& info);

src/library/crate/cratefeature.cpp

@@ -777,7 +777,7 @@ void CrateFeature::htmlLinkClicked(const QUrl& link) {
 }
 
 void CrateFeature::slotTrackSelected(TrackPointer pTrack) {
-    m_pSelectedTrack = pTrack;
+    m_pSelectedTrack = std::move(pTrack);
 
     TreeItem* pRootItem = m_childModel.getRootItem();
     VERIFY_OR_DEBUG_ASSERT(pRootItem != nullptr) {
@@ -786,8 +786,8 @@ void CrateFeature::slotTrackSelected(TrackPointer pTrack) {
 
     TrackId selectedTrackId;
     std::vector<CrateId> sortedTrackCrates;
-    if (pTrack) {
-        selectedTrackId = pTrack->getId();
+    if (m_pSelectedTrack) {
+        selectedTrackId = m_pSelectedTrack->getId();
         CrateTrackSelectResult trackCratesIter(
                 m_pTrackCollection->crates().selectTrackCratesSorted(selectedTrackId));
         while (trackCratesIter.next()) {