[haiku-commits] haiku: hrev49570 - src/servers/registrar/mime

  • From: mmlr@xxxxxxxx
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Sun, 23 Aug 2015 12:31:03 +0200 (CEST)

hrev49570 adds 2 changesets to branch 'master'
old head: 1deb22eb4bc0b7e58fa9feccc94fab9eb63d52ab
new head: 462bfeede0cd123afe2e79d465876289e925ca53
overview:
http://cgit.haiku-os.org/haiku/log/?qt=range&q=462bfeede0cd+%5E1deb22eb4bc0

----------------------------------------------------------------------------

dfb3208fa385: registrar: Whitespace and style cleanup only.

Generally this code still looks horrible (both from a style and from a
complexity point of view) and should eventually be reworked.

462bfeede0cd: registrar: Fix race condition on MimeUpdateThread termination.

When the MimeUpdateThread is done, it marks itself as finished and
notifies the thread manager to clean up finished threads. Since multiple
such threads might finish at the same time and trigger the cleanup
notification, other threads that already marked themselves finished but
haven't actually exited yet might already be deleted and removed. This
would then lead to a use-after-free when they subsequently tried to send
their own cleanup message.

To solve the race condition, the thread manager will now wait for the
thread to actually exit before cleaning it up.

The introduction of the launch_daemon has made this race condition more
likely due to more applications starting in parallel, each triggering a
CreateAppMetaMimeThread which is a subclass of MimeUpdateThread. This
commit might therefore fix #12237.

[ Michael Lotz <mmlr@xxxxxxxx> ]

----------------------------------------------------------------------------

6 files changed, 119 insertions(+), 106 deletions(-)
.../registrar/mime/CreateAppMetaMimeThread.h | 4 +-
src/servers/registrar/mime/MimeUpdateThread.cpp | 64 +++++++-------
src/servers/registrar/mime/RegistrarThread.cpp | 27 +++---
src/servers/registrar/mime/RegistrarThread.h | 26 +++---
.../registrar/mime/RegistrarThreadManager.cpp | 89 +++++++++++---------
.../registrar/mime/RegistrarThreadManager.h | 15 ++--

############################################################################

Commit: dfb3208fa3856c5c6d582fb826b7c991c67fa54d
URL: http://cgit.haiku-os.org/haiku/commit/?id=dfb3208fa385
Author: Michael Lotz <mmlr@xxxxxxxx>
Date: Sat Aug 22 21:05:45 2015 UTC

registrar: Whitespace and style cleanup only.

Generally this code still looks horrible (both from a style and from a
complexity point of view) and should eventually be reworked.

----------------------------------------------------------------------------

diff --git a/src/servers/registrar/mime/CreateAppMetaMimeThread.h
b/src/servers/registrar/mime/CreateAppMetaMimeThread.h
index 797ec63..4ec491a 100644
--- a/src/servers/registrar/mime/CreateAppMetaMimeThread.h
+++ b/src/servers/registrar/mime/CreateAppMetaMimeThread.h
@@ -1,10 +1,10 @@
//----------------------------------------------------------------------
-// This software is part of the OpenBeOS distribution and is covered
+// This software is part of the OpenBeOS distribution and is covered
// by the OpenBeOS license.
//---------------------------------------------------------------------
/*!
\file CreateAppMetaMimeThread.h
- CreateAppMetaMimeThread interface declaration
+ CreateAppMetaMimeThread interface declaration
*/

#ifndef _CREATE_APP_META_MIME_THREAD_H
diff --git a/src/servers/registrar/mime/MimeUpdateThread.cpp
b/src/servers/registrar/mime/MimeUpdateThread.cpp
index 5b530fb..733e8a9 100644
--- a/src/servers/registrar/mime/MimeUpdateThread.cpp
+++ b/src/servers/registrar/mime/MimeUpdateThread.cpp
@@ -42,10 +42,11 @@ namespace Mime {

If \a replyee is non-NULL and construction succeeds, the
MimeThreadObject
assumes resposibility for its deletion.
-
- Also, if \c non-NULL, \a replyee is expected to be a \c
B_REG_MIME_UPDATE_MIME_INFO
- or a \c B_REG_MIME_CREATE_APP_META_MIME message with a \c true \c
"synchronous"
- field detached from the registrar's mime manager looper (though
this is not verified).
+
+ Also, if \c non-NULL, \a replyee is expected to be a
+ \c B_REG_MIME_UPDATE_MIME_INFO or a \c B_REG_MIME_CREATE_APP_META_MIME
+ message with a \c true \c "synchronous" field detached from the
registrar's
+ mime manager looper (though this is not verified).
The message will be replied to at the end of the thread's execution.
*/
MimeUpdateThread::MimeUpdateThread(const char *name, int32 priority,
@@ -65,9 +66,9 @@ MimeUpdateThread::MimeUpdateThread(const char *name, int32
priority,

/*! \brief Destroys the MimeUpdateThread object.

- If the object was properly initialized (i.e. InitCheck() returns \c
B_OK) and
- the replyee message passed to the constructor was \c non-NULL, the
replyee
- message is deleted.
+ If the object was properly initialized (i.e. InitCheck() returns \c
B_OK)
+ and the replyee message passed to the constructor was \c non-NULL, the
+ replyee message is deleted.
*/
MimeUpdateThread::~MimeUpdateThread()
{
@@ -122,20 +123,21 @@ MimeUpdateThread::ThreadFunction()
// Notify the thread manager to make a cleanup run
if (!err) {
BMessage msg(B_REG_MIME_UPDATE_THREAD_FINISHED);
- status_t error = fManagerMessenger.SendMessage(&msg,
(BHandler*)NULL, 500000);
+ status_t error = fManagerMessenger.SendMessage(&msg,
(BHandler*)NULL,
+ 500000);
if (error)
- OUT("WARNING: ThreadManager::ThreadEntryFunction():
Termination notification "
- "failed with error 0x%" B_PRIx32 "\n", error);
+ OUT("WARNING: ThreadManager::ThreadEntryFunction():
Termination"
+ " notification failed with error 0x%" B_PRIx32
"\n", error);
}
- DBG(OUT("(id: %ld) exiting mime update thread with result 0x%" B_PRIx32
"\n",
- find_thread(NULL), err));
+ DBG(OUT("(id: %ld) exiting mime update thread with result 0x%" B_PRIx32
+ "\n", find_thread(NULL), err));
return err;
}


/*! \brief Returns true if the given device supports attributes, false
if not (or if an error occurs while determining).
-
+
Device numbers and their corresponding support info are cached in
a std::list to save unnecessarily \c statvfs()ing devices that have
already been statvfs()ed (which might otherwise happen quite often
@@ -152,13 +154,12 @@ MimeUpdateThread::DeviceSupportsAttributes(dev_t device)
// See if an entry for this device already exists
std::list< std::pair<dev_t,bool> >::iterator i;
for (i = fAttributeSupportList.begin();
- i != fAttributeSupportList.end();
- i++)
+ i != fAttributeSupportList.end(); i++)
{
if (i->first == device)
return i->second;
}
-
+
bool result = false;

// If we get here, no such device is yet in our list,
@@ -175,24 +176,24 @@ MimeUpdateThread::DeviceSupportsAttributes(dev_t device)
else
fAttributeSupportList.push_back(p);
}
-
- return result;
+
+ return result;
}

// UpdateEntry
-/*! \brief Updates the given entry and then recursively updates all the
entry's child
- entries if the entry is a directory and \c fRecursive is true.
+/*! \brief Updates the given entry and then recursively updates all the entry's
+ child entries if the entry is a directory and \c fRecursive is true.
*/
status_t
MimeUpdateThread::UpdateEntry(const entry_ref *ref)
{
status_t err = ref ? B_OK : B_BAD_VALUE;
bool entryIsDir = false;
-
+
// Look to see if we're being terminated
if (!err && fShouldExit)
err = B_CANCELED;
-
+
// Before we update, make sure this entry lives on a device that
supports
// attributes. If not, we skip it and any of its children for
// updates (we don't signal an error, however).
@@ -202,7 +203,7 @@ MimeUpdateThread::UpdateEntry(const entry_ref *ref)
// (DeviceSupportsAttributes(ref->device) ? "yes" : "no"));

if (!err && (device_is_root_device(ref->device)
- || DeviceSupportsAttributes(ref->device))) {
+ || DeviceSupportsAttributes(ref->device))) {
// Update this entry
if (!err) {
// R5 appears to ignore whether or not the update
succeeds.
@@ -211,8 +212,8 @@ MimeUpdateThread::UpdateEntry(const entry_ref *ref)

// If we're recursing and this is a directory, update
// each of the directory's children as well
- if (!err && fRecursive && entryIsDir) {
- BDirectory dir;
+ if (!err && fRecursive && entryIsDir) {
+ BDirectory dir;
err = dir.SetTo(ref);
if (!err) {
entry_ref childRef;
@@ -222,16 +223,15 @@ MimeUpdateThread::UpdateEntry(const entry_ref *ref)
// If we've come to the end of
the directory listing,
// it's not an error.
if (err == B_ENTRY_NOT_FOUND)
- err = B_OK;
+ err = B_OK;
break;
- } else {
- err = UpdateEntry(&childRef);

- }
- }
- }
+ } else
+ err = UpdateEntry(&childRef);
+ }
+ }
}
}
- return err;
+ return err;
}

} // namespace Mime
diff --git a/src/servers/registrar/mime/RegistrarThread.cpp
b/src/servers/registrar/mime/RegistrarThread.cpp
index b50faec..2ce9af6 100644
--- a/src/servers/registrar/mime/RegistrarThread.cpp
+++ b/src/servers/registrar/mime/RegistrarThread.cpp
@@ -15,27 +15,28 @@

/*! \class RegistrarThread
\brief Base thread class for threads spawned and managed by the
registrar
-
*/

// constructor
/*! \brief Creates a new RegistrarThread object, spawning the object's
thread.
-
+
Call Run() to actually get the thread running.
-
+
\param name The desired name of the new thread
\param priority The desired priority of the new thread
\param managerMessenger A BMessenger to the thread manager to which this
thread does or will belong.
*/
-RegistrarThread::RegistrarThread(const char *name, int32 priority, BMessenger
managerMessenger)
- : fManagerMessenger(managerMessenger)
- , fShouldExit(false)
- , fIsFinished(false)
- , fStatus(B_NO_INIT)
- , fId(-1)
- , fPriority(priority)
+RegistrarThread::RegistrarThread(const char *name, int32 priority,
+ BMessenger managerMessenger)
+ :
+ fManagerMessenger(managerMessenger),
+ fShouldExit(false),
+ fIsFinished(false),
+ fStatus(B_NO_INIT),
+ fId(-1),
+ fPriority(priority)
{
fName[0] = 0;
status_t err = name && fManagerMessenger.IsValid() ? B_OK : B_BAD_VALUE;
@@ -71,7 +72,7 @@ RegistrarThread::Run()
fId = spawn_thread(&RegistrarThread::EntryFunction, fName,
fPriority, (void*)this);
err = fId >= 0 ? B_OK : fId;
- if (err == B_OK)
+ if (err == B_OK)
err = resume_thread(fId);
}
return err;
@@ -86,13 +87,13 @@ RegistrarThread::Id() const
}

// Name
-//! Returns the name of the thread
+//! Returns the name of the thread
const char*
RegistrarThread::Name() const
{
return fName;
}
-
+
// AskToExit
/*! \brief Signals to thread that it needs to quit politely as soon
as possible.
diff --git a/src/servers/registrar/mime/RegistrarThread.h
b/src/servers/registrar/mime/RegistrarThread.h
index 48d9edc..42bcde2 100644
--- a/src/servers/registrar/mime/RegistrarThread.h
+++ b/src/servers/registrar/mime/RegistrarThread.h
@@ -1,10 +1,10 @@
//----------------------------------------------------------------------
-// This software is part of the OpenBeOS distribution and is covered
+// This software is part of the OpenBeOS distribution and is covered
// by the OpenBeOS license.
//---------------------------------------------------------------------
/*!
\file RegistrarThread.h
- RegistrarThread interface declaration
+ RegistrarThread interface declaration
*/

#ifndef REGISTRAR_THREAD_H
@@ -15,30 +15,32 @@

class RegistrarThread {
public:
- RegistrarThread(const char *name, int32 priority, BMessenger
managerMessenger);
+ RegistrarThread(const char *name, int32 priority,
+ BMessenger managerMessenger);
virtual ~RegistrarThread();
-
- virtual status_t InitCheck();
+
+ virtual status_t InitCheck();
status_t Run();
-
+
thread_id Id() const;
const char* Name() const;
-
+
void AskToExit();
bool IsFinished() const;
-
+
protected:
//! The function executed in the object's thread when Run() is called
virtual status_t ThreadFunction() = 0;
-
- BMessenger fManagerMessenger;
+
+ BMessenger fManagerMessenger;
bool fShouldExit; // Initially false, may be set to true by
AskToExit()
- bool fIsFinished; // Initially false, set to true by the thread
itself upon completion
+ bool fIsFinished; // Initially false, set to true by the thread
itself
+ // upon completion
private:
static int32 EntryFunction(void *data);

status_t fStatus;
- thread_id fId;
+ thread_id fId;
char fName[B_OS_NAME_LENGTH];
int32 fPriority;
};
diff --git a/src/servers/registrar/mime/RegistrarThreadManager.cpp
b/src/servers/registrar/mime/RegistrarThreadManager.cpp
index 7c924d9..504c75f 100644
--- a/src/servers/registrar/mime/RegistrarThreadManager.cpp
+++ b/src/servers/registrar/mime/RegistrarThreadManager.cpp
@@ -24,8 +24,8 @@ using namespace BPrivate;

/*!
\class RegistrarThreadManager
- \brief RegistrarThreadManager is the master of all threads spawned by
the registrar
-
+ \brief RegistrarThreadManager is the master of all threads spawned by
the
+ registrar
*/

//! Creates a new RegistrarThreadManager object
@@ -35,8 +35,8 @@ RegistrarThreadManager::RegistrarThreadManager()
}

// destructor
-/*! \brief Destroys the RegistrarThreadManager object, killing and deleting
any still
- running threads.
+/*! \brief Destroys the RegistrarThreadManager object, killing and deleting any
+ still running threads.
*/
RegistrarThreadManager::~RegistrarThreadManager()
{
@@ -44,9 +44,11 @@ RegistrarThreadManager::~RegistrarThreadManager()
}

// MessageReceived
-/*! \brief Handles \c B_REG_MIME_UPDATE_THREAD_FINISHED messages, passing on
all others.
+/*! \brief Handles \c B_REG_MIME_UPDATE_THREAD_FINISHED messages, passing on
all
+ others.

- Each \c B_REG_MIME_UPDATE_THREAD_FINISHED message triggers a call to
CleanupThreads().
+ Each \c B_REG_MIME_UPDATE_THREAD_FINISHED message triggers a call to
+ CleanupThreads().
*/
void
RegistrarThreadManager::MessageReceived(BMessage* message)
@@ -57,7 +59,7 @@ RegistrarThreadManager::MessageReceived(BMessage* message)
CleanupThreads();
break;
}
-
+
default:
{
BHandler::MessageReceived(message);
@@ -71,15 +73,16 @@ RegistrarThreadManager::MessageReceived(BMessage* message)
RegistrarThreadManager object.

\param thread Pointer to a newly allocated \c RegistrarThread object.
-
- If the result of the function is \c B_OK, the \c RegistrarThreadManager
object
- assumes ownership of \a thread; if the result is an error code, it
+
+ If the result of the function is \c B_OK, the \c RegistrarThreadManager
+ object assumes ownership of \a thread; if the result is an error code,
it
does not.
-
+
\return
- \c B_OK: success
- - \c B_NO_MORE_THREADS: the number of concurrently allowed threads
(defined by
- RegistrarThreadManager::kThreadLimit) has
already been reached
+ - \c B_NO_MORE_THREADS: the number of concurrently allowed threads
(defined
+ by
RegistrarThreadManager::kThreadLimit) has
+ already been reached
- \c B_BAD_THREAD_STATE: the thread has already been launched
- other error code: failure
*/
@@ -110,14 +113,14 @@ RegistrarThreadManager::LaunchThread(RegistrarThread
*thread)
}
}
if (!err)
- DBG(OUT("RegistrarThreadManager::LaunchThread(): launched new
'%s' thread, "
- "id %ld\n", thread->Name(), thread->Id()));
+ DBG(OUT("RegistrarThreadManager::LaunchThread(): launched new
'%s'"
+ " thread, id %ld\n", thread->Name(), thread->Id()));
return err;
}

// CleanupThreads
/*! \brief Frees the resources of any threads that are no longer running
-
+
\todo This function should perhaps be triggered periodically by a
BMessageRunner once we have our own BMessageRunner implementation.
*/
@@ -128,18 +131,19 @@ RegistrarThreadManager::CleanupThreads()
for (i = fThreads.begin(); i != fThreads.end(); ) {
if (*i) {
if ((*i)->IsFinished()) {
-
DBG(OUT("RegistrarThreadManager::CleanupThreads(): Cleaning up thread %ld\n",
- (*i)->Id()));
+
DBG(OUT("RegistrarThreadManager::CleanupThreads(): Cleaning up"
+ " thread %ld\n", (*i)->Id()));
RemoveThread(i);
// adjusts i
} else
++i;
} else {
- OUT("WARNING: RegistrarThreadManager::CleanupThreads():
NULL mime_update_thread_shared_data "
- "pointer found in and removed from
RegistrarThreadManager::fThreads list\n");
+ OUT("WARNING: RegistrarThreadManager::CleanupThreads():
NULL"
+ " mime_update_thread_shared_data pointer found
in and removed"
+ " from RegistrarThreadManager::fThreads
list\n");
i = fThreads.erase(i);
}
- }
+ }
return B_OK;
}

@@ -157,28 +161,29 @@ RegistrarThreadManager::ShutdownThreads()
for (i = fThreads.begin(); i != fThreads.end(); ) {
if (*i) {
if ((*i)->IsFinished()) {
-
DBG(OUT("RegistrarThreadManager::ShutdownThreads(): Cleaning up thread %ld\n",
- (*i)->Id()));
+
DBG(OUT("RegistrarThreadManager::ShutdownThreads(): Cleaning up"
+ " thread %ld\n", (*i)->Id()));
RemoveThread(i);
// adjusts i
} else {
-
DBG(OUT("RegistrarThreadManager::ShutdownThreads(): Shutting down thread %ld\n",
- (*i)->Id()));
+
DBG(OUT("RegistrarThreadManager::ShutdownThreads(): Shutting"
+ " down thread %ld\n", (*i)->Id()));
(*i)->AskToExit();
++i;
}
} else {
- OUT("WARNING:
RegistrarThreadManager::ShutdownThreads(): NULL mime_update_thread_shared_data "
- "pointer found in and removed from
RegistrarThreadManager::fThreads list\n");
+ OUT("WARNING:
RegistrarThreadManager::ShutdownThreads(): NULL"
+ " mime_update_thread_shared_data pointer found
in and removed"
+ " from RegistrarThreadManager::fThreads
list\n");
i = fThreads.erase(i);
}
}
-
+
/*! \todo We may want to iterate back through the list at this point,
snooze for a moment if find an unfinished thread, and kill it if
it still isn't finished by the time we're done snoozing.
*/
-
+
return B_OK;
}

@@ -195,21 +200,22 @@ RegistrarThreadManager::KillThreads()
for (i = fThreads.begin(); i != fThreads.end(); ) {
if (*i) {
if (!(*i)->IsFinished()) {
- DBG(OUT("RegistrarThreadManager::KillThreads():
Killing thread %ld\n",
- (*i)->Id()));
+ DBG(OUT("RegistrarThreadManager::KillThreads():
Killing thread"
+ " %ld\n", (*i)->Id()));
status_t err = kill_thread((*i)->Id());
if (err)
- OUT("WARNING:
RegistrarThreadManager::KillThreads(): kill_thread(%"
- B_PRId32 ") failed with error
code 0x%" B_PRIx32 "\n",
- (*i)->Id(), err);
- }
- DBG(OUT("RegistrarThreadManager::KillThreads():
Cleaning up thread %ld\n",
- (*i)->Id()));
+ OUT("WARNING:
RegistrarThreadManager::KillThreads():"
+ " kill_thread(%" B_PRId32 ")
failed with error code"
+ " 0x%" B_PRIx32 "\n",
(*i)->Id(), err);
+ }
+ DBG(OUT("RegistrarThreadManager::KillThreads():
Cleaning up thread"
+ " %ld\n", (*i)->Id()));
RemoveThread(i);
// adjusts i
} else {
- OUT("WARNING: RegistrarThreadManager::KillThreads():
NULL mime_update_thread_shared_data "
- "pointer found in and removed from
RegistrarThreadManager::fThreads list\n");
+ OUT("WARNING: RegistrarThreadManager::KillThreads():
NULL"
+ " mime_update_thread_shared_data pointer found
in and removed"
+ " from RegistrarThreadManager::fThreads
list\n");
i = fThreads.erase(i);
}
}
@@ -222,7 +228,7 @@ RegistrarThreadManager::KillThreads()
This is not necessarily a count of how many threads are actually
running,
as threads may remain in the thread list that are finished and waiting
to be cleaned up.
-
+
\return The number of threads currently under management
*/
uint
diff --git a/src/servers/registrar/mime/RegistrarThreadManager.h
b/src/servers/registrar/mime/RegistrarThreadManager.h
index 6dcc59c..7f395a1 100644
--- a/src/servers/registrar/mime/RegistrarThreadManager.h
+++ b/src/servers/registrar/mime/RegistrarThreadManager.h
@@ -1,10 +1,10 @@
//----------------------------------------------------------------------
-// This software is part of the OpenBeOS distribution and is covered
+// This software is part of the OpenBeOS distribution and is covered
// by the OpenBeOS license.
//---------------------------------------------------------------------
/*!
\file RegistrarThreadManager.h
- RegistrarThreadManager interface declaration
+ RegistrarThreadManager interface declaration
*/

#ifndef REGISTRAR_THREAD_MANAGER_H
@@ -21,7 +21,7 @@ class RegistrarThreadManager : public BHandler {
public:
RegistrarThreadManager();
~RegistrarThreadManager();
-
+
// BHandler virtuals
virtual void MessageReceived(BMessage* message);

@@ -30,14 +30,15 @@ public:
status_t CleanupThreads();
status_t ShutdownThreads();
status_t KillThreads();
-
+
uint ThreadCount() const;
-
+
static const int kThreadLimit = 12;
private:

- std::list<RegistrarThread*>::iterator&
RemoveThread(std::list<RegistrarThread*>::iterator &i);
-
+ std::list<RegistrarThread*>::iterator&
+ RemoveThread(std::list<RegistrarThread*>::iterator &i);
+
std::list<RegistrarThread*> fThreads;
int32 fThreadCount;
};

############################################################################

Revision: hrev49570
Commit: 462bfeede0cd123afe2e79d465876289e925ca53
URL: http://cgit.haiku-os.org/haiku/commit/?id=462bfeede0cd
Author: Michael Lotz <mmlr@xxxxxxxx>
Date: Sun Aug 23 09:48:24 2015 UTC

Ticket: https://dev.haiku-os.org/ticket/12237

registrar: Fix race condition on MimeUpdateThread termination.

When the MimeUpdateThread is done, it marks itself as finished and
notifies the thread manager to clean up finished threads. Since multiple
such threads might finish at the same time and trigger the cleanup
notification, other threads that already marked themselves finished but
haven't actually exited yet might already be deleted and removed. This
would then lead to a use-after-free when they subsequently tried to send
their own cleanup message.

To solve the race condition, the thread manager will now wait for the
thread to actually exit before cleaning it up.

The introduction of the launch_daemon has made this race condition more
likely due to more applications starting in parallel, each triggering a
CreateAppMetaMimeThread which is a subclass of MimeUpdateThread. This
commit might therefore fix #12237.

----------------------------------------------------------------------------

diff --git a/src/servers/registrar/mime/RegistrarThreadManager.cpp
b/src/servers/registrar/mime/RegistrarThreadManager.cpp
index 504c75f..9ce7690 100644
--- a/src/servers/registrar/mime/RegistrarThreadManager.cpp
+++ b/src/servers/registrar/mime/RegistrarThreadManager.cpp
@@ -243,6 +243,9 @@ RegistrarThreadManager::ThreadCount() const
std::list<RegistrarThread*>::iterator&
RegistrarThreadManager::RemoveThread(std::list<RegistrarThread*>::iterator &i)
{
+ status_t dummy;
+ wait_for_thread((*i)->Id(), &dummy);
+
delete *i;
atomic_add(&fThreadCount, -1);
return (i = fThreads.erase(i));


Other related posts:

  • » [haiku-commits] haiku: hrev49570 - src/servers/registrar/mime - mmlr