From fc4e1e3b667932879b55323970ac7e007900fa1c Mon Sep 17 00:00:00 2001 From: Stefan Hacker Date: Tue, 5 Nov 2013 21:29:20 +0100 Subject: [PATCH] Fix recorder crash due to use after free on user object. The recorder used a ClientUser object pointer attached to the RecordBuffer to be able to access a users session id and username . If the user joined a server, made a noise and immediately left the server before the RecordBuffer from the noise was processed the referenced ClientUser was already deleted which crashed the client. This fix retools RecordInfo and RecordBuffer to carry the information previously needed from the ClientUser so that inside of the recorder thread no dependencies to the ClientUser exist. --- src/mumble/VoiceRecorder.cpp | 52 +++++++++++++++++++----------------- src/mumble/VoiceRecorder.h | 29 ++++++++++++-------- 2 files changed, 45 insertions(+), 36 deletions(-) diff --git a/src/mumble/VoiceRecorder.cpp b/src/mumble/VoiceRecorder.cpp index ad3f1bafbcb..c1d776c2432 100644 --- a/src/mumble/VoiceRecorder.cpp +++ b/src/mumble/VoiceRecorder.cpp @@ -42,12 +42,12 @@ #include "../Timer.h" VoiceRecorder::RecordBuffer::RecordBuffer( - const ClientUser *cu, + int userIndex, boost::shared_array buffer, int samples, quint64 absoluteStartSample) - : cuUser(cu) + : recordInfoIndex(userIndex) , fBuffer(buffer) , iSamples(samples) , absoluteStartSample(absoluteStartSample) { @@ -55,8 +55,9 @@ VoiceRecorder::RecordBuffer::RecordBuffer( // Nothing } -VoiceRecorder::RecordInfo::RecordInfo(quint64 lastWrittenAbsoluteSample) - : sf(NULL) +VoiceRecorder::RecordInfo::RecordInfo(quint64 lastWrittenAbsoluteSample, const QString& userName) + : userName(userName) + , sf(NULL) , lastWrittenAbsoluteSample(lastWrittenAbsoluteSample) { } @@ -121,17 +122,12 @@ QString VoiceRecorder::sanitizeFilenameOrPathComponent(const QString &str) const return res; } -QString VoiceRecorder::expandTemplateVariables(const QString &path, boost::shared_ptr rb) const { +QString VoiceRecorder::expandTemplateVariables(const QString &path, const QString& userName) const { // Split path into components QString res; QStringList comp = path.split(QLatin1Char('/')); Q_ASSERT(!comp.isEmpty()); - QString username(QLatin1String("Mixdown")); - // In mixdown mode |cuUser| is always NULL. - if (rb->cuUser) - username = rb->cuUser->qsName; - // Create a readable representation of the start date. QString date(qdtRecordingStart.date().toString(Qt::ISODate)); QString time(qdtRecordingStart.time().toString(QLatin1String("hh-mm-ss"))); @@ -150,7 +146,7 @@ QString VoiceRecorder::expandTemplateVariables(const QString &path, boost::share // %time Inserts the current time // %host Inserts the hostname QHash vars; - vars.insert(QLatin1String("user"), username); + vars.insert(QLatin1String("user"), userName); vars.insert(QLatin1String("date"), date); vars.insert(QLatin1String("time"), time); vars.insert(QLatin1String("host"), hostname); @@ -197,6 +193,12 @@ QString VoiceRecorder::expandTemplateVariables(const QString &path, boost::share return res; } +int VoiceRecorder::indexForUser(const ClientUser *clientUser) const { + Q_ASSERT(!bMixDown || clientUser == NULL); + + return (bMixDown) ? 0 : clientUser->uiSession; +} + void VoiceRecorder::run() { Q_ASSERT(!bRecording); Q_ASSERT(iSampleRate != 0); @@ -272,15 +274,13 @@ void VoiceRecorder::run() { QMutexLocker l(&qmBufferLock); rb = qlRecordBuffer.takeFirst(); } - - // Use 0 as the |index| if multi channel recording is disabled. - int index = bMixDown ? 0 : rb->cuUser->uiSession; - Q_ASSERT(qhRecordInfo.contains(index)); - + // Create the file for this RecordInfo instance if it's not yet open. - boost::shared_ptr ri = qhRecordInfo.value(index); + + Q_ASSERT(qhRecordInfo.contains(rb->recordInfoIndex)); + boost::shared_ptr ri = qhRecordInfo.value(rb->recordInfoIndex); if (ri->sf == NULL) { - QString filename = expandTemplateVariables(qsFileName, rb); + QString filename = expandTemplateVariables(qsFileName, ri->userName); // Try to find a unique filename. { @@ -320,8 +320,7 @@ void VoiceRecorder::run() { } // Store the username in the title attribute of the file (if supported by the format). - if (rb->cuUser) - sf_set_string(ri->sf, SF_STR_TITLE, qPrintable(rb->cuUser->qsName)); + sf_set_string(ri->sf, SF_STR_TITLE, qPrintable(ri->userName)); } // Calculate the difference between the time of the current buffer and the time where we last wrote audio data for that user. @@ -357,13 +356,16 @@ void VoiceRecorder::stop() { qwcSleep.wakeAll(); } -void VoiceRecorder::addBuffer(const ClientUser *cu, boost::shared_array buffer, int samples, quint64 absoluteSampleCount) { - Q_ASSERT(!bMixDown || cu == NULL); +void VoiceRecorder::addBuffer(const ClientUser *clientUser, boost::shared_array buffer, int samples, quint64 absoluteSampleCount) { + Q_ASSERT(!bMixDown || clientUser == NULL); // Create a new RecordInfo object if this is a new user. - int index = bMixDown ? 0 : cu->uiSession; + const int index = indexForUser(clientUser); + if (!qhRecordInfo.contains(index)) { - boost::shared_ptr ri = boost::make_shared(m_firstSampleAbsolute); + boost::shared_ptr ri = boost::make_shared( + m_firstSampleAbsolute, bMixDown ? QLatin1String("Mixdown") + : clientUser->qsName); qhRecordInfo.insert(index, ri); } @@ -371,7 +373,7 @@ void VoiceRecorder::addBuffer(const ClientUser *cu, boost::shared_array b { // Save the buffer in |qlRecordBuffer|. QMutexLocker l(&qmBufferLock); - boost::shared_ptr rb = boost::make_shared(cu, buffer, samples, absoluteSampleCount); + boost::shared_ptr rb = boost::make_shared(index, buffer, samples, absoluteSampleCount); qlRecordBuffer << rb; } diff --git a/src/mumble/VoiceRecorder.h b/src/mumble/VoiceRecorder.h index b9fe1f25524..cecdb4d6f5b 100644 --- a/src/mumble/VoiceRecorder.h +++ b/src/mumble/VoiceRecorder.h @@ -81,13 +81,13 @@ class VoiceRecorder : public QThread { // Stores information about a recording buffer. struct RecordBuffer { // Constructs a new RecordBuffer object. - explicit RecordBuffer(const ClientUser *cu, - boost::shared_array buffer, - int samples, - quint64 absoluteStartSample); + RecordBuffer(int recordInfoIndex, + boost::shared_array buffer, + int samples, + quint64 absoluteStartSample); - // The user to which this buffer belongs. - const ClientUser *cuUser; + /// Hashmap index for the user + const int recordInfoIndex; // The buffer. boost::shared_array fBuffer; @@ -101,14 +101,18 @@ class VoiceRecorder : public QThread { // Keep the recording state for one user. struct RecordInfo { - explicit RecordInfo(quint64 lastWrittenAbsoluteSample); + RecordInfo(quint64 lastWrittenAbsoluteSample, const QString& userName); ~RecordInfo(); - // libsndfile's handle. + /// Name of the user being recorded + const QString userName; + + /// libsndfile's handle. SNDFILE *sf; /// The last absolute sample we wrote for this users quint64 lastWrittenAbsoluteSample; + }; // Hash which maps the |uiSession| of all users for which we have to keep a recording state to the corresponding RecordInfo object. @@ -154,9 +158,12 @@ class VoiceRecorder : public QThread { // Removes invalid characters in a path component. QString sanitizeFilenameOrPathComponent(const QString &str) const; - // Expands the template variables in |path| using the information contained in |rb|. - QString expandTemplateVariables(const QString &path, boost::shared_ptr rb) const; + /// Expands the template variables in |path| for the given |userName|. + QString expandTemplateVariables(const QString &path, const QString& userName) const; + /// Returns the RecordInfo hashmap index for the given user + int indexForUser(const ClientUser *clientUser) const; + public: // Error enum enum Error { Unspecified, CreateDirectoryFailed, CreateFileFailed, InvalidSampleRate }; @@ -173,7 +180,7 @@ class VoiceRecorder : public QThread { /// Adds an audio buffer which contains |samples| audio samples to the recorder. /// The audio data will be aligned using the given |absoluteSampleCount| - void addBuffer(const ClientUser *cu, boost::shared_array buffer, int samples, quint64 absoluteSampleCount); + void addBuffer(const ClientUser *clientUser, boost::shared_array buffer, int samples, quint64 absoluteSampleCount); // Sets the sample rate of the recorder. The sample rate can't change while the recoder is active. void setSampleRate(int sampleRate);