[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    KDirWatch crash fix
From:       David Faure <faure () kde ! org>
Date:       2008-03-15 22:01:19
Message-ID: 200803152301.20459.faure () kde ! org
[Download RAW message or body]

After making some changes to kdirmodeltest I've been hitting a reproduceable crash in KDirWatch.

The problem comes from this code in removeEntry:
      if (e->isDir)
        removeEntry(0, QDir::cleanPath(e->path+"/.."), e);
      else
        removeEntry(0, QFileInfo(e->path).absolutePath(), e);

If the entry for that "parent directory" is in removeList (the list of removed deletions)
then when processing that list it will crash, due to looking at an already-deleted entry.

Any objections against the attached patch?

-- 
David Faure, faure@kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

["kdirwatch.diff" (text/x-diff)]

Index: kdirwatch.cpp
===================================================================
--- kdirwatch.cpp	(révision 785348)
+++ kdirwatch.cpp	(copie de travail)
@@ -607,8 +607,8 @@
   {
     if (sub_entry) {
        (*it).m_entries.append(sub_entry);
-       kDebug(7001) << "Added already watched Entry " << path
-                    << " (for " << sub_entry->path << ")";
+       kDebug(7001) << "Added already watched Entry" << path
+                    << "(for" << sub_entry->path << ")";
 #ifdef HAVE_SYS_INOTIFY_H
        Entry* e = &(*it);
        if( (e->m_mode == INotifyMode) && (e->wd > 0) ) {
@@ -639,10 +639,9 @@
   QByteArray tpath (QFile::encodeName(path));
   bool exists = (KDE_stat(tpath, &stat_buf) == 0);
 
-  Entry newEntry;
-  m_mapEntries.insert( path, newEntry );
+  EntryMap::iterator newIt = m_mapEntries.insert( path, Entry() );
   // the insert does a copy, so we have to use <e> now
-  Entry* e = &(m_mapEntries[path]);
+  Entry* e = &(*newIt);
 
   if (exists) {
     e->isDir = S_ISDIR(stat_buf.st_mode);
@@ -758,10 +757,10 @@
 
 
 void KDirWatchPrivate::removeEntry( KDirWatch* instance,
-                                    const QString& _path, Entry* sub_entry )
+                                    const QString& _path, // TODO Entry* e instead
+                                    Entry* sub_entry )
 {
-  kDebug(7001)  << "KDirWatchPrivate::removeEntry for" << _path
-                << "sub_entry:" << sub_entry;
+  kDebug(7001) << "path=" << _path << "sub_entry:" << sub_entry;
   Entry* e = entry(_path);
   if (!e) {
     kWarning(7001)  << "KDirWatch::removeDir can't handle"
@@ -785,6 +784,8 @@
     return;
   }
 
+  removeList.removeAll(e);
+
 #ifdef HAVE_FAM
   if (e->m_mode == FAMMode) {
     if ( e->m_status == Normal) {
@@ -1117,9 +1118,13 @@
 void KDirWatchPrivate::slotRemoveDelayed()
 {
   delayRemove = false;
-  foreach(Entry* e, removeList)
+  // Removing an entry could also take care of removing its parent
+  // (e.g. in FAM or inotify mode), which would remove other entries in removeList,
+  // so don't use foreach or iterators here...
+  while (!removeList.isEmpty()) {
+    Entry* e = removeList.takeFirst();
     removeEntry(0, e->path, 0);
-  removeList.clear();
+  }
 }
 
 /* Scan all entries to be watched for changes. This is done regularly


[prev in list] [next in list] [prev in thread] [next in thread]