Bug 834100 - Null deref if you call addIceCandidate on an RTCPeerConnection before setting localDesc [@ fsmdef_ev_addcandidate]. r=abr

This commit is contained in:
Andrew Miller 2013-01-31 15:43:03 -05:00
parent 379a090a1d
commit 3b617bcb01
14 changed files with 155 additions and 29 deletions

View File

@ -552,7 +552,7 @@ PeerConnection.prototype = {
return Cr.NS_ERROR_NOT_IMPLEMENTED;
},
addIceCandidate: function(cand) {
addIceCandidate: function(cand, onSuccess, onError) {
if (!cand) {
throw new Error ("NULL candidate passed to addIceCandidate!");
}
@ -561,10 +561,13 @@ PeerConnection.prototype = {
throw new Error ("Invalid candidate passed to addIceCandidate!");
}
this._onAddIceCandidateSuccess = onSuccess;
this._onAddIceCandidateError = onError;
this._queueOrRun({
func: this._pc.addIceCandidate,
args: [cand.candidate, cand.sdpMid || "", cand.sdpMLineIndex],
wait: false
wait: true
});
},
@ -758,6 +761,22 @@ PeerConnectionObserver.prototype = {
this._dompc._executeNext();
},
onAddIceCandidateSuccess: function(code) {
this._dompc._pendingType = null;
if (this._dompc._onAddIceCandidateSuccess) {
this._dompc._onAddIceCandidateSuccess.onCallback(code);
}
this._dompc._executeNext();
},
onAddIceCandidateError: function(code) {
this._dompc._pendingType = null;
if (this._dompc._onAddIceCandidateError) {
this._dompc._onAddIceCandidateError.onCallback(code);
}
this._dompc._executeNext();
},
onStateChange: function(state) {
if (state != Ci.IPeerConnectionObserver.kIceState) {
return;

View File

@ -42,6 +42,8 @@ interface IPeerConnectionObserver : nsISupports
void onSetRemoteDescriptionSuccess(in unsigned long code);
void onSetLocalDescriptionError(in unsigned long code);
void onSetRemoteDescriptionError(in unsigned long code);
void onAddIceCandidateSuccess(in unsigned long code);
void onAddIceCandidateError(in unsigned long code);
/* Data channel callbacks */
void notifyDataChannel(in nsIDOMDataChannel channel);

View File

@ -59,7 +59,9 @@ interface nsIDOMRTCPeerConnection : nsISupports
[optional] in jsval constraints,
[optional] in bool restart);
void addIceCandidate(in nsIDOMRTCIceCandidate candidate);
void addIceCandidate(in nsIDOMRTCIceCandidate candidate,
[optional] in RTCPeerConnectionCallback successCallback,
[optional] in RTCPeerConnectionCallback failureCallback);
void addStream(in nsIDOMMediaStream stream,
[optional] in jsval constraints);

View File

@ -0,0 +1,24 @@
<html class="reftest-wait">
<head>
<script language="javascript">
function start() {
remotePC = new mozRTCPeerConnection();
var cand = new mozRTCIceCandidate(
{candidate: "1 1 UDP 1 127.0.0.1 34567 type host",
sdpMid: "helloworld",
sdbMid: "helloworld", // Mis-spelt attribute for bug 833948 compatibility.
sdpMLineIndex: 1
});
remotePC.addIceCandidate(cand);
remotePC.addIceCandidate(cand, function(sdp){}, finish);
}
function finish(arg) {
document.documentElement.removeAttribute("class");
}
</script>
</head>
<body onload="start()">
</body>
</html>

View File

@ -8,4 +8,5 @@ load 799419.html
load 801227.html
load 802982.html
load 812785.html
load 834100.html
load 822197.html

View File

@ -174,6 +174,14 @@ public:
mObserver->OnSetRemoteDescriptionError(mCode);
break;
case ADDICECANDIDATE:
mObserver->OnAddIceCandidateSuccess(mCode);
break;
case ADDICECANDIDATEERROR:
mObserver->OnAddIceCandidateError(mCode);
break;
case REMOTESTREAMADD:
{
nsDOMMediaStream* stream = nullptr;
@ -200,7 +208,6 @@ public:
}
case UPDATELOCALDESC:
case UPDATEREMOTEDESC:
/* No action necessary */
break;
@ -1183,7 +1190,7 @@ PeerConnectionImpl::onCallEvent(ccapi_call_event_e aCallEvent,
break;
case SETREMOTEDESC:
case UPDATEREMOTEDESC:
case ADDICECANDIDATE:
mRemoteSDP = aInfo->getSDP();
break;

View File

@ -1319,6 +1319,7 @@ static void ccappUpdateSessionData (session_update_t *sessUpd)
sessUpd->eventID == SET_REMOTE_DESC ||
sessUpd->eventID == UPDATE_LOCAL_DESC ||
sessUpd->eventID == UPDATE_REMOTE_DESC ||
sessUpd->eventID == ICE_CANDIDATE_ADD ||
sessUpd->eventID == REMOTE_STREAM_ADD ) {
CCAPP_DEBUG(DEB_F_PREFIX"CALL_SESSION_CREATED for session id 0x%x event is 0x%x \n",
@ -1361,6 +1362,7 @@ static void ccappUpdateSessionData (session_update_t *sessUpd)
sessUpd->eventID == SET_REMOTE_DESC ||
sessUpd->eventID == UPDATE_LOCAL_DESC ||
sessUpd->eventID == UPDATE_REMOTE_DESC ||
sessUpd->eventID == ICE_CANDIDATE_ADD ||
sessUpd->eventID == REMOTE_STREAM_ADD ) {
data->attr = sessUpd->update.ccSessionUpd.data.state_data.attr;
data->inst = sessUpd->update.ccSessionUpd.data.state_data.inst;
@ -1391,6 +1393,7 @@ static void ccappUpdateSessionData (session_update_t *sessUpd)
case SET_REMOTE_DESC:
case UPDATE_LOCAL_DESC:
case UPDATE_REMOTE_DESC:
case ICE_CANDIDATE_ADD:
data->sdp = sessUpd->update.ccSessionUpd.data.state_data.sdp;
/* Fall through to the next case... */
case REMOTE_STREAM_ADD:
@ -1728,6 +1731,7 @@ static void ccappUpdateSessionData (session_update_t *sessUpd)
case SET_REMOTE_DESC:
case UPDATE_LOCAL_DESC:
case UPDATE_REMOTE_DESC:
case ICE_CANDIDATE_ADD:
data->sdp = sessUpd->update.ccSessionUpd.data.state_data.sdp;
/* Fall through to the next case... */
case REMOTE_STREAM_ADD:

View File

@ -1670,25 +1670,20 @@ void ui_update_local_description(call_events event, line_t nLine, callid_t nCall
}
/**
* Let PeerConnection know about an updated remote session description
* Send data from addIceCandidate to the UI
*
* @return none
* @return none
*/
void ui_update_remote_description(call_events event, line_t nLine, callid_t nCallID,
uint16_t call_instance_id, string_t sdp)
void ui_ice_candidate_add(call_events event, line_t nLine, callid_t nCallID,
uint16_t call_instance_id, string_t sdp)
{
TNP_DEBUG(DEB_L_C_F_PREFIX"state=%d call_instance=%d\n",
DEB_L_C_F_PREFIX_ARGS(UI_API, nLine, nCallID, __FUNCTION__),
event, call_instance_id);
DEB_L_C_F_PREFIX_ARGS(UI_API, nLine, nCallID, __FUNCTION__), event, call_instance_id);
post_message_helper(UPDATE_REMOTE_DESC, event, nLine, nCallID,
call_instance_id, sdp, PC_OK);
return;
post_message_helper(ICE_CANDIDATE_ADD, event, nLine, nCallID, call_instance_id, sdp, PC_OK);
}
/**
* Send Remote Stream data to the UI
*

View File

@ -3617,15 +3617,29 @@ fsmdef_ev_addcandidate(sm_event_t *event) {
config_get_value(CFGID_SDPMODE, &sdpmode, sizeof(sdpmode));
if (sdpmode == FALSE) {
ui_ice_candidate_add(evAddIceCandidateError, line, call_id,
dcb->caller_id.call_instance_id, strlib_empty());
return (SM_RC_END);
}
if (dcb == NULL) {
FSM_DEBUG_SM(DEB_F_PREFIX"dcb is NULL.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
ui_ice_candidate_add(evAddIceCandidateError, line, call_id,
dcb->caller_id.call_instance_id, strlib_empty());
return SM_RC_CLEANUP;
}
if (!dcb->sdp) {
FSM_DEBUG_SM(DEB_F_PREFIX"dcb->sdp is NULL. Has the "
"remote description been set yet?\n",
DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
ui_ice_candidate_add(evAddIceCandidateError, line, call_id,
dcb->caller_id.call_instance_id, strlib_empty());
return SM_RC_END;
}
/* Perform level lookup based on mid value */
/* comment until mid is properly updated
cause = gsmsdp_find_level_from_mid(dcb, (const char *)msg->data.candidate.mid, &level);
@ -3665,10 +3679,12 @@ fsmdef_ev_addcandidate(sm_event_t *event) {
remote_sdp = sipsdp_write_to_buf(dcb->sdp->dest_sdp, &remote_sdp_len);
if (!remote_sdp) {
ui_ice_candidate_add(evAddIceCandidateError, line, call_id,
dcb->caller_id.call_instance_id, strlib_empty());
return (SM_RC_END);
}
ui_update_remote_description(evUpdateRemoteDesc, line, call_id,
ui_ice_candidate_add(evAddIceCandidate, line, call_id,
dcb->caller_id.call_instance_id, strlib_malloc(remote_sdp,-1));
free(remote_sdp);

View File

@ -222,7 +222,8 @@ typedef enum {
SET_REMOTE_DESC,
UPDATE_LOCAL_DESC,
UPDATE_REMOTE_DESC,
REMOTE_STREAM_ADD
REMOTE_STREAM_ADD,
ICE_CANDIDATE_ADD
} group_call_event_t;
/* File Player Session Events */

View File

@ -38,10 +38,11 @@ typedef enum {
evSetLocalDesc = SETLOCALDESC,
evSetRemoteDesc = SETREMOTEDESC,
evUpdateLocalDesc = UPDATELOCALDESC,
evUpdateRemoteDesc = UPDATEREMOTEDESC,
evSetLocalDescError = SETLOCALDESCERROR,
evSetRemoteDescError = SETREMOTEDESCERROR,
evOnRemoteStreamAdd = REMOTESTREAMADD,
evAddIceCandidate = ADDICECANDIDATE,
evAddIceCandidateError = ADDICECANDIDATEERROR,
evMaxEvent
} call_events;
@ -162,8 +163,9 @@ void ui_set_remote_description(call_events event, line_t nLine, callid_t nCallID
void ui_update_local_description(call_events event, line_t nLine, callid_t nCallID,
uint16_t call_instance_id, string_t sdp);
void ui_update_remote_description(call_events event, line_t nLine, callid_t nCallID,
uint16_t call_instance_id, string_t sdp);
void ui_ice_candidate_add(call_events event, line_t nLine, callid_t nCallID,
uint16_t call_instance_id, string_t sdp);
void ui_on_remote_stream_added(call_events event, line_t nLine, callid_t nCallID,
uint16_t call_instance_id, cc_media_remote_track_table_t media_tracks);

View File

@ -287,6 +287,8 @@ typedef enum {
SETLOCALDESCERROR,
SETREMOTEDESCERROR,
REMOTESTREAMADD,
ADDICECANDIDATE,
ADDICECANDIDATEERROR,
MAX_CALL_STATES
} cc_call_state_t;

View File

@ -138,9 +138,6 @@ std::string CC_SIPCCCallInfo::callStateToString (cc_call_state_t state)
case UPDATELOCALDESC:
statestr = "UPDATELOCALDESC";
break;
case UPDATEREMOTEDESC:
statestr = "UPDATEREMOTEDESC";
break;
case SETLOCALDESCERROR:
statestr = "SETLOCALDESCERROR";
break;
@ -150,6 +147,12 @@ std::string CC_SIPCCCallInfo::callStateToString (cc_call_state_t state)
case REMOTESTREAMADD:
statestr = "REMOTESTREAMADD";
break;
case ADDICECANDIDATE:
statestr = "ADDICECANDIDATE";
break;
case ADDICECANDIDATEERROR:
statestr = "ADDICECANDIDATEERROR";
break;
default:
break;
}

View File

@ -144,7 +144,7 @@ public:
};
TestObserver(sipcc::PeerConnectionImpl *peerConnection) :
state(stateNoResponse),
state(stateNoResponse), addIceSuccessCount(0),
onAddStreamCalled(false),
pc(peerConnection) {
}
@ -160,6 +160,7 @@ public:
char *lastString;
uint32_t lastStatusCode;
uint32_t lastStateType;
int addIceSuccessCount;
bool onAddStreamCalled;
private:
@ -354,6 +355,24 @@ TestObserver::FoundIceCandidate(const char* strCandidate)
return NS_OK;
}
NS_IMETHODIMP
TestObserver::OnAddIceCandidateSuccess(uint32_t code)
{
lastStatusCode = code;
state = stateSuccess;
addIceSuccessCount++;
return NS_OK;
}
NS_IMETHODIMP
TestObserver::OnAddIceCandidateError(uint32_t code)
{
lastStatusCode = code;
state = stateError;
cout << "onAddIceCandidateError = " << code << endl;
return NS_OK;
}
class ParsedSDP {
public:
//Line number with the corresponding SDP line.
@ -699,18 +718,24 @@ void CreateAnswer(sipcc::MediaConstraints& constraints, std::string offer,
}
void DoTrickleIce(ParsedSDP &sdp) {
int expectAddIce = 0;
pObserver->addIceSuccessCount = 0;
for (std::multimap<int, std::string>::iterator it = sdp.ice_candidates_.begin();
it != sdp.ice_candidates_.end(); ++it) {
if ((*it).first != 0) {
std::cerr << "Adding trickle ICE candidate " << (*it).second << std::endl;
ASSERT_TRUE(NS_SUCCEEDED(pc->AddIceCandidate((*it).second.c_str(), "", (*it).first)));
expectAddIce++;
}
}
ASSERT_TRUE_WAIT(pObserver->addIceSuccessCount == expectAddIce,
kDefaultTimeout);
}
void DoTrickleIceChrome(ParsedSDP &sdp) {
int expectAddIce = 0;
pObserver->addIceSuccessCount = 0;
for (std::multimap<int, std::string>::iterator it = sdp.ice_candidates_.begin();
it != sdp.ice_candidates_.end(); ++it) {
if ((*it).first != 0) {
@ -718,8 +743,11 @@ void CreateAnswer(sipcc::MediaConstraints& constraints, std::string offer,
std::cerr << "Adding trickle ICE candidate " << candidate << std::endl;
ASSERT_TRUE(NS_SUCCEEDED(pc->AddIceCandidate(candidate.c_str(), "", (*it).first)));
expectAddIce++;
}
}
ASSERT_TRUE_WAIT(pObserver->addIceSuccessCount == expectAddIce,
kDefaultTimeout);
}
@ -729,8 +757,16 @@ void CreateAnswer(sipcc::MediaConstraints& constraints, std::string offer,
return state == sipcc::PeerConnectionImpl::kIceConnected;
}
void AddIceCandidate(const char* candidate, const char* mid, unsigned short level) {
void AddIceCandidate(const char* candidate, const char* mid, unsigned short level,
bool expectSuccess) {
pObserver->state = TestObserver::stateNoResponse;
pc->AddIceCandidate(candidate, mid, level);
ASSERT_TRUE_WAIT(pObserver->state != TestObserver::stateNoResponse,
kDefaultTimeout);
ASSERT_TRUE(pObserver->state ==
expectSuccess ? TestObserver::stateSuccess :
TestObserver::stateError
);
}
int GetPacketsReceived(int stream) {
@ -1016,7 +1052,12 @@ public:
const char * candidate, const char * mid,
unsigned short level, uint32_t sdpCheck) {
a1_.CreateOffer(constraints, OFFER_AV, sdpCheck);
a1_.AddIceCandidate(candidate, mid, level);
a1_.AddIceCandidate(candidate, mid, level, true);
}
void AddIceCandidateEarly(const char * candidate, const char * mid,
unsigned short level) {
a1_.AddIceCandidate(candidate, mid, level, false);
}
protected:
@ -1342,6 +1383,13 @@ TEST_F(SignalingTest, CreateOfferAddCandidate)
SHOULD_SENDRECV_AV);
}
TEST_F(SignalingTest, AddIceCandidateEarly)
{
sipcc::MediaConstraints constraints;
AddIceCandidateEarly(strSampleCandidate.c_str(),
strSampleMid.c_str(), nSamplelevel);
}
// XXX adam@nostrum.com -- This test seems questionable; we need to think
// through what actually needs to be tested here.
TEST_F(SignalingTest, DISABLED_OfferAnswerReNegotiateOfferAnswerDontReceiveVideoNoVideoStream)