[Live-devel] [PATCH] fix remaining use-after-free resulting from unscheduleDelayedTask
Helmut Grohne
helmut.grohne at intenta.de
Thu Jan 9 03:36:30 PST 2020
When scheduling a delayed task, one receives a TaskToken. This token can
be used lateron for cancelling the task. Or the task could be run in the
interim and the token could be reissued for a different task. If the
task is unscheduled at this point, this consitutues a use-after-free
scenario. According to
http://lists.live555.com/pipermail/live-devel/2016-August/020225.html
one is not supposed to unschedule such a task.
Accordingly, we update all task handler functions to clear their
corresponding TaskToken once they are run. A subsequent call to
unscheduleDelayedTask will harmlessly unschedule the NULL token.
Note that clearing a token after unscheduleDelayedTask is not necessary.
---
.../WindowsAudioInputDevice_common.cpp | 3 ++-
liveMedia/GenericMediaServer.cpp | 1 +
liveMedia/ProxyServerMediaSession.cpp | 22 +++++++++++++------
liveMedia/SIPClient.cpp | 3 +++
liveMedia/T140TextRTPSink.cpp | 2 +-
liveMedia/include/ProxyServerMediaSession.hh | 1 +
testProgs/playCommon.cpp | 4 ++++
7 files changed, 27 insertions(+), 9 deletions(-)
This is reviving a pretty old thread. Yet, the problem reported there
persists and deserves a fix. This followup is meant to close the matter
for good.
Helmut
diff --git a/WindowsAudioInputDevice/WindowsAudioInputDevice_common.cpp b/WindowsAudioInputDevice/WindowsAudioInputDevice_common.cpp
index 451ef4e7..6534c43e 100644
--- a/WindowsAudioInputDevice/WindowsAudioInputDevice_common.cpp
+++ b/WindowsAudioInputDevice/WindowsAudioInputDevice_common.cpp
@@ -65,7 +65,7 @@ void WindowsAudioInputDevice_common::doGetNextFrame() {
void WindowsAudioInputDevice_common::doStopGettingFrames() {
// Turn off the audio poller:
- envir().taskScheduler().unscheduleDelayedTask(nextTask()); nextTask() = NULL;
+ envir().taskScheduler().unscheduleDelayedTask(nextTask());
}
double WindowsAudioInputDevice_common::getAverageLevel() const {
@@ -96,6 +96,7 @@ double WindowsAudioInputDevice_common::getAverageLevel() const {
void WindowsAudioInputDevice_common::audioReadyPoller(void* clientData) {
WindowsAudioInputDevice_common* inputDevice = (WindowsAudioInputDevice_common*)clientData;
+ inputDevice->nextTask() = NULL;
inputDevice->audioReadyPoller1();
}
diff --git a/liveMedia/GenericMediaServer.cpp b/liveMedia/GenericMediaServer.cpp
index 7e970f28..33eab479 100644
--- a/liveMedia/GenericMediaServer.cpp
+++ b/liveMedia/GenericMediaServer.cpp
@@ -303,6 +303,7 @@ void GenericMediaServer::ClientSession::noteClientLiveness(ClientSession* client
}
void GenericMediaServer::ClientSession::livenessTimeoutTask(ClientSession* clientSession) {
+ clientSession->fLivenessCheckTask = NULL;
// If this gets called, the client session is assumed to have timed out, so delete it:
#ifdef DEBUG
char const* streamName
diff --git a/liveMedia/ProxyServerMediaSession.cpp b/liveMedia/ProxyServerMediaSession.cpp
index 512e4b47..eb6cda15 100644
--- a/liveMedia/ProxyServerMediaSession.cpp
+++ b/liveMedia/ProxyServerMediaSession.cpp
@@ -115,7 +115,7 @@ ProxyServerMediaSession
tunnelOverHTTPPortNum,
verbosityLevel > 0 ? verbosityLevel-1 : verbosityLevel,
socketNumToServer);
- ProxyRTSPClient::sendDESCRIBE(fProxyRTSPClient);
+ fProxyRTSPClient->sendDESCRIBE();
}
ProxyServerMediaSession::~ProxyServerMediaSession() {
@@ -258,10 +258,10 @@ ProxyRTSPClient::ProxyRTSPClient(ProxyServerMediaSession& ourServerMediaSession,
}
void ProxyRTSPClient::reset() {
- envir().taskScheduler().unscheduleDelayedTask(fLivenessCommandTask); fLivenessCommandTask = NULL;
- envir().taskScheduler().unscheduleDelayedTask(fDESCRIBECommandTask); fDESCRIBECommandTask = NULL;
- envir().taskScheduler().unscheduleDelayedTask(fSubsessionTimerTask); fSubsessionTimerTask = NULL;
- envir().taskScheduler().unscheduleDelayedTask(fResetTask); fResetTask = NULL;
+ envir().taskScheduler().unscheduleDelayedTask(fLivenessCommandTask);
+ envir().taskScheduler().unscheduleDelayedTask(fDESCRIBECommandTask);
+ envir().taskScheduler().unscheduleDelayedTask(fSubsessionTimerTask);
+ envir().taskScheduler().unscheduleDelayedTask(fResetTask);
fSetupQueueHead = fSetupQueueTail = NULL;
fNumSetupsDone = 0;
@@ -419,6 +419,7 @@ void ProxyRTSPClient::scheduleLivenessCommand() {
void ProxyRTSPClient::sendLivenessCommand(void* clientData) {
ProxyRTSPClient* rtspClient = (ProxyRTSPClient*)clientData;
+ rtspClient->fLivenessCommandTask = NULL;
// Note. By default, we do not send "GET_PARAMETER" as our 'liveness notification' command, even if the server previously
// indicated (in its response to our earlier "OPTIONS" command) that it supported "GET_PARAMETER". This is because
@@ -444,6 +445,7 @@ void ProxyRTSPClient::scheduleReset() {
}
void ProxyRTSPClient::doReset() {
+ fResetTask = NULL;
if (fVerbosityLevel > 0) {
envir() << *this << "::doReset\n";
}
@@ -452,7 +454,7 @@ void ProxyRTSPClient::doReset() {
fOurServerMediaSession.resetDESCRIBEState();
setBaseURL(fOurURL); // because we'll be sending an initial "DESCRIBE" all over again
- sendDESCRIBE(this);
+ sendDESCRIBE();
}
void ProxyRTSPClient::doReset(void* clientData) {
@@ -478,7 +480,12 @@ void ProxyRTSPClient::scheduleDESCRIBECommand() {
void ProxyRTSPClient::sendDESCRIBE(void* clientData) {
ProxyRTSPClient* rtspClient = (ProxyRTSPClient*)clientData;
- if (rtspClient != NULL) rtspClient->sendDescribeCommand(::continueAfterDESCRIBE, rtspClient->auth());
+ rtspClient->fDESCRIBECommandTask = NULL;
+ rtspClient->sendDESCRIBE();
+}
+
+void ProxyRTSPClient::sendDESCRIBE() {
+ sendDescribeCommand(::continueAfterDESCRIBE, auth());
}
void ProxyRTSPClient::subsessionTimeout(void* clientData) {
@@ -486,6 +493,7 @@ void ProxyRTSPClient::subsessionTimeout(void* clientData) {
}
void ProxyRTSPClient::handleSubsessionTimeout() {
+ fSubsessionTimerTask = NULL;
// We still have one or more subsessions ('tracks') left to "SETUP". But we can't wait any longer for them. Send a "PLAY" now:
MediaSession* sess = fOurServerMediaSession.fClientMediaSession;
if (sess != NULL) sendPlayCommand(*sess, ::continueAfterPLAY, -1.0f, -1.0f, 1.0f, fOurAuthenticator);
diff --git a/liveMedia/SIPClient.cpp b/liveMedia/SIPClient.cpp
index 72e922f3..cab24161 100644
--- a/liveMedia/SIPClient.cpp
+++ b/liveMedia/SIPClient.cpp
@@ -339,6 +339,7 @@ unsigned const timerDFires = 0xDDDDDDDD;
void SIPClient::timerAHandler(void* clientData) {
SIPClient* client = (SIPClient*)clientData;
+ client->fTimerA = NULL;
if (client->fVerbosityLevel >= 1) {
client->envir() << "RETRANSMISSION " << ++client->fTimerACount
<< ", after " << client->fTimerALen/1000000.0
@@ -349,6 +350,7 @@ void SIPClient::timerAHandler(void* clientData) {
void SIPClient::timerBHandler(void* clientData) {
SIPClient* client = (SIPClient*)clientData;
+ client->fTimerB = NULL;
if (client->fVerbosityLevel >= 1) {
client->envir() << "RETRANSMISSION TIMEOUT, after "
<< 64*client->fT1/1000000.0 << " seconds\n";
@@ -359,6 +361,7 @@ void SIPClient::timerBHandler(void* clientData) {
void SIPClient::timerDHandler(void* clientData) {
SIPClient* client = (SIPClient*)clientData;
+ client->fTimerD = NULL;
if (client->fVerbosityLevel >= 1) {
client->envir() << "TIMER D EXPIRED\n";
}
diff --git a/liveMedia/T140TextRTPSink.cpp b/liveMedia/T140TextRTPSink.cpp
index 6d9212bd..84554fba 100644
--- a/liveMedia/T140TextRTPSink.cpp
+++ b/liveMedia/T140TextRTPSink.cpp
@@ -143,6 +143,7 @@ void T140IdleFilter::handleIdleTimeout(void* clientData) {
}
void T140IdleFilter::handleIdleTimeout() {
+ fIdleTimerTask = NULL;
// No data has arrived from the upstream source within our specified 'idle period' (after data was requested from downstream).
// Send an empty 'idle' frame to our downstream "T140TextRTPSink". (This will cause an empty RTP packet to get sent.)
deliverEmptyFrame();
@@ -178,7 +179,6 @@ void T140IdleFilter::onSourceClosure(void* clientData) {
void T140IdleFilter::onSourceClosure() {
envir().taskScheduler().unscheduleDelayedTask(fIdleTimerTask);
- fIdleTimerTask = NULL;
handleClosure();
}
diff --git a/liveMedia/include/ProxyServerMediaSession.hh b/liveMedia/include/ProxyServerMediaSession.hh
index 902dfda1..ea7b4f70 100644
--- a/liveMedia/include/ProxyServerMediaSession.hh
+++ b/liveMedia/include/ProxyServerMediaSession.hh
@@ -65,6 +65,7 @@ private:
void scheduleDESCRIBECommand();
static void sendDESCRIBE(void* clientData);
+ void sendDESCRIBE();
static void subsessionTimeout(void* clientData);
void handleSubsessionTimeout();
diff --git a/testProgs/playCommon.cpp b/testProgs/playCommon.cpp
index 707a4a24..47dbc3ac 100644
--- a/testProgs/playCommon.cpp
+++ b/testProgs/playCommon.cpp
@@ -1163,6 +1163,7 @@ void sessionTimerHandler(void* /*clientData*/) {
}
void periodicFileOutputTimerHandler(void* /*clientData*/) {
+ periodicFileOutputTask = NULL;
fileOutputSecondsSoFar += fileOutputInterval;
// First, close the existing output files:
@@ -1435,6 +1436,7 @@ void signalHandlerShutdown(int /*sig*/) {
}
void checkForPacketArrival(void* /*clientData*/) {
+ arrivalCheckTimerTask = NULL;
if (!notifyOnPacketArrival) return; // we're not checking
// Check each subsession, to see whether it has received data packets:
@@ -1493,6 +1495,7 @@ void checkForPacketArrival(void* /*clientData*/) {
}
void checkInterPacketGaps(void* /*clientData*/) {
+ interPacketGapCheckTimerTask = NULL;
if (interPacketGapMaxTime == 0) return; // we're not checking
// Check each subsession, counting up how many packets have been received:
@@ -1522,6 +1525,7 @@ void checkInterPacketGaps(void* /*clientData*/) {
}
void checkSessionTimeoutBrokenServer(void* /*clientData*/) {
+ sessionTimeoutBrokenServerTask = NULL;
if (!sendKeepAlivesToBrokenServers) return; // we're not checking
// Send an "OPTIONS" request, starting with the second call
--
2.20.1
More information about the live-devel
mailing list