Skip to content
Permalink
Browse files

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.
  • Loading branch information...
hacst committed Nov 5, 2013
1 parent 9a47e05 commit fc4e1e3b667932879b55323970ac7e007900fa1c
Showing with 45 additions and 36 deletions.
  1. +27 −25 src/mumble/VoiceRecorder.cpp
  2. +18 −11 src/mumble/VoiceRecorder.h
@@ -42,21 +42,22 @@
#include "../Timer.h"

VoiceRecorder::RecordBuffer::RecordBuffer(
const ClientUser *cu,
int userIndex,
boost::shared_array<float> buffer,
int samples,
quint64 absoluteStartSample)

: cuUser(cu)
: recordInfoIndex(userIndex)
, fBuffer(buffer)
, iSamples(samples)
, absoluteStartSample(absoluteStartSample) {

// 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<RecordBuffer> 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<const QString, QString> 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<RecordInfo> ri = qhRecordInfo.value(index);

Q_ASSERT(qhRecordInfo.contains(rb->recordInfoIndex));
boost::shared_ptr<RecordInfo> 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,21 +356,24 @@ void VoiceRecorder::stop() {
qwcSleep.wakeAll();
}

void VoiceRecorder::addBuffer(const ClientUser *cu, boost::shared_array<float> buffer, int samples, quint64 absoluteSampleCount) {
Q_ASSERT(!bMixDown || cu == NULL);
void VoiceRecorder::addBuffer(const ClientUser *clientUser, boost::shared_array<float> 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<RecordInfo> ri = boost::make_shared<RecordInfo>(m_firstSampleAbsolute);
boost::shared_ptr<RecordInfo> ri = boost::make_shared<RecordInfo>(
m_firstSampleAbsolute, bMixDown ? QLatin1String("Mixdown")
: clientUser->qsName);

qhRecordInfo.insert(index, ri);
}

{
// Save the buffer in |qlRecordBuffer|.
QMutexLocker l(&qmBufferLock);
boost::shared_ptr<RecordBuffer> rb = boost::make_shared<RecordBuffer>(cu, buffer, samples, absoluteSampleCount);
boost::shared_ptr<RecordBuffer> rb = boost::make_shared<RecordBuffer>(index, buffer, samples, absoluteSampleCount);
qlRecordBuffer << rb;
}

@@ -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<float> buffer,
int samples,
quint64 absoluteStartSample);
RecordBuffer(int recordInfoIndex,
boost::shared_array<float> 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<float> 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<RecordBuffer> 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<float> buffer, int samples, quint64 absoluteSampleCount);
void addBuffer(const ClientUser *clientUser, boost::shared_array<float> 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);

0 comments on commit fc4e1e3

Please sign in to comment.
You can’t perform that action at this time.