Change in osmo-msc[master]: call_leg: local_bridge: Avoid null pointer access if CN-side not ready
pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-msc/+/26917 )
Change subject: call_leg: local_bridge: Avoid null pointer access if CN-side not ready ......................................................................
call_leg: local_bridge: Avoid null pointer access if CN-side not ready
This happens if for instance an HNBGW drops the RAB-AssignmentRequest and does nothing with it.
call_leg.c:348:15: runtime error: member access within null pointer of type 'struct rtp_stream'
Related: OS#5401 Change-Id: I67d2d5b2dd3b367c34f929d63c056306ec001431 --- M src/libmsc/call_leg.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/17/26917/1
diff --git a/src/libmsc/call_leg.c b/src/libmsc/call_leg.c index 6242a80..e890f75 100644 --- a/src/libmsc/call_leg.c +++ b/src/libmsc/call_leg.c @@ -344,6 +344,9 @@ } codec = cl1->rtp[RTP_TO_RAN]->codec;
+ if (!cl1->rtp[RTP_TO_CN] || !cl2->rtp[RTP_TO_CN]) + return -ENOTCONN; + call_leg_ensure_ci(cl1, RTP_TO_CN, call_id1, trans1, &codec, &cl2->rtp[RTP_TO_CN]->local); call_leg_ensure_ci(cl2, RTP_TO_CN, call_id2, trans2,
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/26917 )
Change subject: call_leg: local_bridge: Avoid null pointer access if CN-side not ready ......................................................................
Patch Set 1:
This patch promptly terminates the call if any of the 2 CN-sides are not already ready when the bridging is requested.
Another version of the patch which also would fix the issue would be:
""" - call_leg_ensure_ci(cl1, RTP_TO_CN, call_id1, trans1, - &codec, &cl2->rtp[RTP_TO_CN]->local); - call_leg_ensure_ci(cl2, RTP_TO_CN, call_id2, trans2, - &codec, &cl1->rtp[RTP_TO_CN]->local); + call_leg_ensure_ci(cl1, RTP_TO_CN, call_id1, trans1, &codec, + cl2->rtp[RTP_TO_CN] ? &cl2->rtp[RTP_TO_CN]->local : NULL); + call_leg_ensure_ci(cl2, RTP_TO_CN, call_id2, trans2, &codec, + cl1->rtp[RTP_TO_CN] ? &cl1->rtp[RTP_TO_CN]->local : NULL) """
In the version presented above, the call would continue for a few more secs, until timer X2 triggers in rtp_stream FSM.
The question here is whether in normal conditions, it can be that the RAB-ASsReq is delayed so much that it arrives after the bridging is done. In that case, maybe this version above is better since then it may end up working correctly...
Neels or others, any comments on preferred way to fix it?
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/26917 )
Change subject: call_leg: local_bridge: Avoid null pointer access if CN-side not ready ......................................................................
Patch Set 1:
The question here is whether in normal conditions, it can be that the RAB-ASsReq is delayed so much that it arrives after the bridging is done. In that case, maybe this version above is better since then it may end up working correctly...
well, the RAB assignment might take quite a long time e.g. in a satellite back-haul scenario, where the hNB is 300ms RTT away.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/26917 )
Change subject: call_leg: local_bridge: Avoid null pointer access if CN-side not ready ......................................................................
Patch Set 1: Code-Review+1
I'm wondering why call_leg_local_bridge() is even called before all sides are ready. I think placing a safeguard here to avoid a crash is a good thing (fix DoS).
But if the RAB Assignment didn't complete, it should not even be reaching this stage, right?
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/26917 )
Change subject: call_leg: local_bridge: Avoid null pointer access if CN-side not ready ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+1
I'm wondering why call_leg_local_bridge() is even called before all sides are ready. I think placing a safeguard here to avoid a crash is a good thing (fix DoS).
But if the RAB Assignment didn't complete, it should not even be reaching this stage, right?
Agree, I also find this a bit weird that it reaches this point. I guess on the 3g side (MO side) the call proceeds due to CM Service Request, and is not currently waiting for RAB Assignment-Response?
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/26917 )
Change subject: call_leg: local_bridge: Avoid null pointer access if CN-side not ready ......................................................................
Patch Set 1: Verified+1 Code-Review+1
I have observed the problem also. This patch prevents the crash.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/26917 )
Change subject: call_leg: local_bridge: Avoid null pointer access if CN-side not ready ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review+1
I'm wondering why call_leg_local_bridge() is even called before all sides are ready. I think placing a safeguard here to avoid a crash is a good thing (fix DoS).
But if the RAB Assignment didn't complete, it should not even be reaching this stage, right?
Agree, I also find this a bit weird that it reaches this point. I guess on the 3g side (MO side) the call proceeds due to CM Service Request, and is not currently waiting for RAB Assignment-Response?
I think the point is that the RAB-AssignmentRequest does complete, but the RAB-AssignmentResponse wasn't forwarded by the hnbgw for some reason. The call then proceeds. (Connect, Connect Acknowledge...) Thats at least how I can trigger the problem. I just uncomment the line where the modified RAB-AssignmentResponse is forwarded.
After all the HNB sends the RAB-AssignmentResponse and then the Connect. Due to the MGW handling it is now possible that the Connect overtakes the RAB-AssignmentResponse, if the operations on the MGW take very long. This is probably not a real world problem but maybe we have to synchronize the Connect message. I am not sure. At least from the trace I can see there are about 5 sec. between RAB-AssignmentResponse and Connect.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/26917 )
Change subject: call_leg: local_bridge: Avoid null pointer access if CN-side not ready ......................................................................
Patch Set 1: Code-Review+2
pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/26917 )
Change subject: call_leg: local_bridge: Avoid null pointer access if CN-side not ready ......................................................................
call_leg: local_bridge: Avoid null pointer access if CN-side not ready
This happens if for instance an HNBGW drops the RAB-AssignmentRequest and does nothing with it.
call_leg.c:348:15: runtime error: member access within null pointer of type 'struct rtp_stream'
Related: OS#5401 Change-Id: I67d2d5b2dd3b367c34f929d63c056306ec001431 --- M src/libmsc/call_leg.c 1 file changed, 3 insertions(+), 0 deletions(-)
Approvals: Jenkins Builder: Verified dexter: Looks good to me, but someone else must approve; Verified neels: Looks good to me, but someone else must approve pespin: Looks good to me, approved
diff --git a/src/libmsc/call_leg.c b/src/libmsc/call_leg.c index 6242a80..e890f75 100644 --- a/src/libmsc/call_leg.c +++ b/src/libmsc/call_leg.c @@ -344,6 +344,9 @@ } codec = cl1->rtp[RTP_TO_RAN]->codec;
+ if (!cl1->rtp[RTP_TO_CN] || !cl2->rtp[RTP_TO_CN]) + return -ENOTCONN; + call_leg_ensure_ci(cl1, RTP_TO_CN, call_id1, trans1, &codec, &cl2->rtp[RTP_TO_CN]->local); call_leg_ensure_ci(cl2, RTP_TO_CN, call_id2, trans2,
-
dexter -
laforge
-
neels
-
pespin