From 7fe219f6810fe79cfc5a38d44fad90dc2444066e Mon Sep 17 00:00:00 2001 From: "andrew@webrtc.org" Date: Wed, 1 Feb 2012 02:40:37 +0000 Subject: [PATCH] Add some additional checks for corrupt payload. Investigation with corrupt payloads revealed a few places we could be using stronger checks. These are not foolproof by any means, but I figure the earlier we catch this the better. BUG=242 TEST=loopback call with a hacked ViE to insert corrupt payloads, and vie_auto_test without the hacks. Review URL: https://webrtc-codereview.appspot.com/369015 git-svn-id: http://webrtc.googlecode.com/svn/trunk@1585 4adac7df-926f-26a2-2b94-8c16560cd09d --- src/modules/rtp_rtcp/source/receiver_fec.cc | 13 +++++++++---- src/modules/rtp_rtcp/source/receiver_fec.h | 4 +++- src/modules/rtp_rtcp/source/rtp_receiver.cc | 7 +++++++ src/modules/rtp_rtcp/source/rtp_utility.cc | 6 ++++++ 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/modules/rtp_rtcp/source/receiver_fec.cc b/src/modules/rtp_rtcp/source/receiver_fec.cc index b2fe0b606..f677ffde7 100644 --- a/src/modules/rtp_rtcp/source/receiver_fec.cc +++ b/src/modules/rtp_rtcp/source/receiver_fec.cc @@ -8,16 +8,19 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include "receiver_fec.h" + #include -#include "receiver_fec.h" #include "rtp_receiver_video.h" #include "rtp_utility.h" +#include "trace.h" // RFC 5109 namespace webrtc { ReceiverFEC::ReceiverFEC(const WebRtc_Word32 id, RTPReceiverVideo* owner) - : _owner(owner), + : _id(id), + _owner(owner), _fec(new ForwardErrorCorrection(id)), _payloadTypeFEC(-1), _lastFECSeqNum(0), @@ -128,8 +131,10 @@ WebRtc_Word32 ReceiverFEC::AddReceivedFECPacket( timestampOffset += incomingRtpPacket[rtpHeader->header.headerLength+2]; timestampOffset = timestampOffset >> 2; if(timestampOffset != 0) { - // timestampOffset should be 0, however, this is a valid error case in - // the event of garbage payload. + // |timestampOffset| should be 0. However, it's possible this is the first + // location a corrupt payload can be caught, so don't assert. + WEBRTC_TRACE(kTraceWarning, kTraceRtpRtcp, _id, + "Corrupt payload found in %s", __FUNCTION__); delete receivedPacket->pkt; delete receivedPacket; return -1; diff --git a/src/modules/rtp_rtcp/source/receiver_fec.h b/src/modules/rtp_rtcp/source/receiver_fec.h index 069787ae1..74cbb9189 100644 --- a/src/modules/rtp_rtcp/source/receiver_fec.h +++ b/src/modules/rtp_rtcp/source/receiver_fec.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011 The WebRTC project authors. All Rights Reserved. + * Copyright (c) 2012 The WebRTC project authors. All Rights Reserved. * * Use of this source code is governed by a BSD-style license * that can be found in the LICENSE file in the root of the source @@ -44,6 +44,8 @@ public: private: int ParseAndReceivePacket(const ForwardErrorCorrection::Packet* packet); + + int _id; RTPReceiverVideo* _owner; ForwardErrorCorrection* _fec; std::list _receivedPacketList; diff --git a/src/modules/rtp_rtcp/source/rtp_receiver.cc b/src/modules/rtp_rtcp/source/rtp_receiver.cc index 9e6438bde..f42196623 100644 --- a/src/modules/rtp_rtcp/source/rtp_receiver.cc +++ b/src/modules/rtp_rtcp/source/rtp_receiver.cc @@ -1228,6 +1228,13 @@ WebRtc_Word32 RTPReceiver::CheckPayloadChanged( payloadType = firstPayloadByte & 0x7f; isRED = true; + if (REDPayloadType(payloadType)) { + // Invalid payload type, traced by caller. If we proceeded here, + // this would be set as |_lastReceivedPayloadType|, and we would no + // longer catch corrupt packets at this level. + return -1; + } + //when we receive RED we need to check the real payload type if (payloadType == _lastReceivedPayloadType) { if(_audio) diff --git a/src/modules/rtp_rtcp/source/rtp_utility.cc b/src/modules/rtp_rtcp/source/rtp_utility.cc index bddcdbdbe..664dd6c61 100644 --- a/src/modules/rtp_rtcp/source/rtp_utility.cc +++ b/src/modules/rtp_rtcp/source/rtp_utility.cc @@ -823,6 +823,12 @@ ModuleRTPUtility::RTPPayloadParser::ParseVP8(RTPPayload& parsedPacket) const vp8->beginningOfPartition = (*dataPtr & 0x10) ? true : false; // S bit vp8->partitionID = (*dataPtr & 0x0F); // PartID field + if (vp8->partitionID > 8) + { + // Weak check for corrupt data: PartID MUST NOT be larger than 8. + return false; + } + // Advance dataPtr and decrease remaining payload size dataPtr++; dataLength--;