Fixing a few NULL dereference warnings found by the Iu coverity check. They are not related apart from that.
Neels Hofmeyr (4): gprs_gmm: ensure llme present upon Attach Req (CID #57686) gtphub_unmap_header_tei(): don't dereference unmapped_tei arg if not present (CID #57687) bsc_nat: forward_sccp_to_msc(): assert con presence (CID #57872) gbproxy_test: assert msg allocation (CID #57873)
openbsc/src/gprs/gprs_gmm.c | 4 ++++ openbsc/src/gprs/gtphub.c | 10 ++++++---- openbsc/src/osmo-bsc_nat/bsc_nat.c | 1 + openbsc/tests/gbproxy/gbproxy_test.c | 1 + 4 files changed, 12 insertions(+), 4 deletions(-)
--- openbsc/src/gprs/gprs_gmm.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/openbsc/src/gprs/gprs_gmm.c b/openbsc/src/gprs/gprs_gmm.c index 5f0a5fd..438e047 100644 --- a/openbsc/src/gprs/gprs_gmm.c +++ b/openbsc/src/gprs/gprs_gmm.c @@ -838,6 +838,10 @@ static int gsm48_rx_gmm_att_req(struct sgsn_mm_ctx *ctx, struct msgb *msg, int rc;
LOGMMCTXP(LOGL_INFO, ctx, "-> GMM ATTACH REQUEST "); + if (!llme) { + LOGMMCTXP(LOGL_ERROR, ctx, "No LLME for GMM ATTACH REQUEST"); + return -EINVAL; + }
/* As per TS 04.08 Chapter 4.7.1.4, the attach request arrives either * with a foreign TLLI (P-TMSI that was allocated to the MS before),
On 14 Apr 2016, at 15:21, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
openbsc/src/gprs/gprs_gmm.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/openbsc/src/gprs/gprs_gmm.c b/openbsc/src/gprs/gprs_gmm.c index 5f0a5fd..438e047 100644 --- a/openbsc/src/gprs/gprs_gmm.c +++ b/openbsc/src/gprs/gprs_gmm.c @@ -838,6 +838,10 @@ static int gsm48_rx_gmm_att_req(struct sgsn_mm_ctx *ctx, struct msgb *msg, int rc;
LOGMMCTXP(LOGL_INFO, ctx, "-> GMM ATTACH REQUEST ");
- if (!llme) {
same as with my other mail. The lower layer should create a llme on the fly. So please have a look at the coverity user interface to see _where_ the llme is NULL and then we can have a look at that issue.
holger
On Fri, Apr 22, 2016 at 03:47:29PM +0200, Holger Freyther wrote:
On 14 Apr 2016, at 15:21, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
openbsc/src/gprs/gprs_gmm.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/openbsc/src/gprs/gprs_gmm.c b/openbsc/src/gprs/gprs_gmm.c index 5f0a5fd..438e047 100644 --- a/openbsc/src/gprs/gprs_gmm.c +++ b/openbsc/src/gprs/gprs_gmm.c @@ -838,6 +838,10 @@ static int gsm48_rx_gmm_att_req(struct sgsn_mm_ctx *ctx, struct msgb *msg, int rc;
LOGMMCTXP(LOGL_INFO, ctx, "-> GMM ATTACH REQUEST ");
- if (!llme) {
same as with my other mail. The lower layer should create a llme on the fly. So please have a look at the coverity user interface to see _where_ the llme is NULL and then we can have a look at that issue.
There is, obviously, an explicit NULL on the sysmocom/iu branch, where no LLME is applicable.
openbsc/openbsc/src/gprs/gprs_gmm.c:2462: rc = gsm0408_rcv_gmm(mmctx, msg, NULL);
Coverity concerns itself about a
"Comparing llme to null implies that llme might be null."
so no actual NULL is reported. (though coverity could have reported above explicit NULL which I found manually)
Actually, the llme NULL check in gsm0408_rcv_gmm() is introduced in the sysmocom/iu branch along with mentioned explicit NULL:
1628 if (llme && !mmctx && 1629 gh->msg_type != GSM48_MT_GMM_ATTACH_REQ && 1630 gh->msg_type != GSM48_MT_GMM_RA_UPD_REQ) { 1631 LOGP(DMM, LOGL_NOTICE, "Cannot handle GMM for unknown MM CTX\n");
So my suggestion above to clean up llme only when it's not NULL does still make a lot of sense to me.
Since I'm not familiar with GRPS code, maybe Daniel could have a quick look? I'm having a déjà vu, didn't I write this before? Would be nice if I could stop banging my head against your wall now ;)
~Neels
On 25 Apr 2016, at 12:51, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
There is, obviously, an explicit NULL on the sysmocom/iu branch, where no LLME is applicable.
openbsc/openbsc/src/gprs/gprs_gmm.c:2462: rc = gsm0408_rcv_gmm(mmctx, msg, NULL);
when looking at the patch I thought you would be on master.
So my suggestion above to clean up llme only when it's not NULL does still make a lot of sense to me.
Since I'm not familiar with GRPS code, maybe Daniel could have a quick look? I'm having a déjà vu, didn't I write this before? Would be nice if I could stop banging my head against your wall now ;)
I would need to look at the code but if the GMM code is used on top of two different layers, then it should not look too much at the LLME? Specially as with send/receive it might not use the same LLME anyway?
So yes, for your Iu branch add the NULL check, once we merge to master we need to see if there is valuable information inside the llme that we need to store somewhere else/or just log it.
holger
On Mon, Apr 25, 2016 at 08:35:39PM +0200, Holger Freyther wrote:
On 25 Apr 2016, at 12:51, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
There is, obviously, an explicit NULL on the sysmocom/iu branch, where no LLME is applicable.
openbsc/openbsc/src/gprs/gprs_gmm.c:2462: rc = gsm0408_rcv_gmm(mmctx, msg, NULL);
when looking at the patch I thought you would be on master.
IIUC the coverity build uses the iu branches?
But right, I submitted the patch against master, while on the iu branches I would normally just commit. So, a mixup from my side, sorry!
I'm still glad you found this issue; I do in general highly esteem your reviews, pretty much always of pinnacle quality in an amazingly short time. Kudos!
So yes, for your Iu branch add the NULL check, once we merge to master we need to see if there is valuable information inside the llme that we need to store somewhere else/or just log it.
My guess is that for Iu subscriptions, there will simply be no LLME, and there won't be information in an LLME that is NULL ;) But agreed.
The merge-Iu-to-master review will be ... extreme, probably. Daniel and I should at some point take a look at refactoring the commit sequence to group by semantics and iron out trial-and-error commits.
But it won't happen without an A-interface, I guess, so it is quite far down the line at this point (unless we keep NITB somehow, which would probably also be a bit of work).
~Neels
Hi Neels,
I'm a bit troubled by the following statements:
On Thu, Apr 28, 2016 at 01:27:11PM +0200, Neels Hofmeyr wrote:
The merge-Iu-to-master review will be ... extreme, probably. Daniel and I should at some point take a look at refactoring the commit sequence to group by semantics and iron out trial-and-error commits.
But it won't happen without an A-interface, I guess, so it is quite far down the line at this point (unless we keep NITB somehow, which would probably also be a bit of work).
It was clearly not the intention to keep the code out of master for more than the time it takes to implement those features.
So yes, very clearly, for the CS side the NITB has to stay intact after a merge of the Iu, until the A interface is implemented. So please don't think of this as something "far down the line".
For the PS side, I don't see any reason at all why a merge to master should be postponed much longer (after it works end-to-end, with GSUP and external HLR). The newly-introduced libiu should be easy to add without any risk to break existing code. And the decisions about Gb vs. IuPS are all made at runtime when looking at the respective subscriber / mm_context.
Regards, Harald
On Thu, Apr 28, 2016 at 02:52:12PM +0200, Harald Welte wrote:
It was clearly not the intention to keep the code out of master for more than the time it takes to implement those features.
So yes, very clearly, for the CS side the NITB has to stay intact after a merge of the Iu, until the A interface is implemented. So please don't think of this as something "far down the line".
This is a very high level decision.
So far the explicit goal was to replace CSCN's libbsc with a proper A interface. So instead of placing junctures to decide whether to use BSC land structures (like the bts pointer) or not, I removed BSC land references.
The A-interface aim is more neat in the sense that we will have explicit junctures that can decide whether to send a given message to A or IuCS. This obviously removes some NITB direct-BSC-access features.
OTOH, the NITB-with-IuCS would be more like the tightly connected MSC+BSC with an IuCS side channel "stuck to its side". It's then questionnable whether to remove the direct-BSC-access features at all.
Implementing the A-interface is clearly more work before being able to merge. But if we are going to sacrifice NITB for an A-interface anyway, it would be overall less work to keep NITB and IuCS separate until A is done.
Keeping the NITB requires some un-rewiring, talking about the changes to gsm_subscriber_connection and reverting a few clear cuts from the previous code paths that I so far have in the iu branch. Shouldn't be too much work really.
However, to me, keeping the NITB along with IuCS does sound like a complete change of direction compared to the previous instructions you gave me, i.e. to go for an A. Either way is fine, but let's be clear on which is the goal!
Thinkable would also be to have all three: IuCS, NITB and a future A interface. That is to a degree sacrificing the neat clarity of separating MSC and BSC for good, and it would be the non-UNIX style "Eierlegende Wollmilchsau" [1].
These are my thoughts on CS so far, I am happy to go whichever way the community prefers. Let's establish consensus on the goal/roadmap as soon as possible.
My wish to review the branch diff so far is still valid and probably goes hand in hand with this decision. It might be good if I have a look at the key junctures in the code paths and send a report/overview to this list?
For the PS side, I don't see any reason at all why a merge to master should be postponed much longer.
Yes, of course depending on Daniel's opinion, this is a completely different thing and merging is less of a complex decision.
~Neels
--- openbsc/src/gprs/gtphub.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/openbsc/src/gprs/gtphub.c b/openbsc/src/gprs/gtphub.c index e8bd3ae..58300ea 100644 --- a/openbsc/src/gprs/gtphub.c +++ b/openbsc/src/gprs/gtphub.c @@ -1434,14 +1434,16 @@ static int gtphub_unmap_header_tei(struct gtphub_peer_port **to_port_p, p->header_tei_rx, gtphub_port_str(from_port)); return -1; } - OSMO_ASSERT(*unmapped_from_tun); + + if (unmapped_from_tun) { + OSMO_ASSERT(*unmapped_from_tun); + LOG(LOGL_DEBUG, "Unmapped TEI coming from: %s\n", + gtphub_tunnel_str(*unmapped_from_tun)); + }
uint32_t unmapped_tei = to->tei_orig; set_tei(p, unmapped_tei);
- LOG(LOGL_DEBUG, "Unmapped TEI coming from: %s\n", - gtphub_tunnel_str(*unmapped_from_tun)); - /* May be NULL for an invalidated tunnel. */ *to_port_p = to->peer;
--- openbsc/src/osmo-bsc_nat/bsc_nat.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c index cacb919..b065769 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c @@ -1152,6 +1152,7 @@ static int forward_sccp_to_msc(struct bsc_connection *bsc, struct msgb *msg) if (!create_sccp_src_ref(bsc, parsed)) goto exit2; con = patch_sccp_src_ref_to_msc(msg, parsed, bsc); + OSMO_ASSERT(con); con->msc_con = bsc->nat->msc_con; con_msc = con->msc_con; con->filter_state.con_type = con_type;
--- openbsc/tests/gbproxy/gbproxy_test.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/openbsc/tests/gbproxy/gbproxy_test.c b/openbsc/tests/gbproxy/gbproxy_test.c index 0ba827f..96a68b0 100644 --- a/openbsc/tests/gbproxy/gbproxy_test.c +++ b/openbsc/tests/gbproxy/gbproxy_test.c @@ -1293,6 +1293,7 @@ static int gprs_process_message(struct gprs_ns_inst *nsi, const char *text, stru }
msg = gprs_ns_msgb_alloc(); + OSMO_ASSERT(msg); memmove(msg->data, data, data_len); msg->l2h = msg->data; msgb_put(msg, data_len);
On 14 Apr 2016, at 09:21, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
Fixing a few NULL dereference warnings found by the Iu coverity check. They are not related apart from that.
I just looked at the gprs_gmm patch and stopped reading. Your commit message should at least have some of the context of coverity.
So when is llme NULL? Is it allowed to be NULL? Does it make sense? We don't want to blindly do these things but understand the code around and see if the tool is right or wrong. And if we disagree maybe change the flow of code or add an assert.
holger
On Thu, Apr 14, 2016 at 09:37:52AM -0400, Holger Freyther wrote:
On 14 Apr 2016, at 09:21, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
Fixing a few NULL dereference warnings found by the Iu coverity check. They are not related apart from that.
I just looked at the gprs_gmm patch and stopped reading. Your commit message should at least have some of the context of coverity.
So when is llme NULL? Is it allowed to be NULL? Does it make sense? We don't want to blindly do these things but understand the code around and see if the tool is right or wrong. And if we disagree maybe change the flow of code or add an assert.
Spot on, I don't actually understand the llme one. All I know is that the calling function gsm0408_rcv_gmm() has a condition "if (llme..." up at the top. Let's drop that one unless someone else has the time to look at it.
The others I do understand though. Do read on ;)
~Neels
On Fri, Apr 15, 2016 at 11:01:04PM +0200, Neels Hofmeyr wrote:
On Thu, Apr 14, 2016 at 09:37:52AM -0400, Holger Freyther wrote:
On 14 Apr 2016, at 09:21, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
Fixing a few NULL dereference warnings found by the Iu coverity check. They are not related apart from that.
I just looked at the gprs_gmm patch and stopped reading. Your commit message should at least have some of the context of coverity.
So when is llme NULL? Is it allowed to be NULL? Does it make sense? We don't want to blindly do these things but understand the code around and see if the tool is right or wrong. And if we disagree maybe change the flow of code or add an assert.
Spot on, I don't actually understand the llme one. All I know is that the calling function gsm0408_rcv_gmm() has a condition "if (llme..." up at the top. Let's drop that one unless someone else has the time to look at it.
I just noticed that on the Iu branch and for Iu connections, llme is explicitly passed as NULL, so my patch would break things for Iu. Good thing you spotted that it's fishy.
The patch should probably be
[[[ diff --git a/openbsc/src/gprs/gprs_gmm.c b/openbsc/src/gprs/gprs_gmm.c index f510e64..f8d75d5 100644 --- a/openbsc/src/gprs/gprs_gmm.c +++ b/openbsc/src/gprs/gprs_gmm.c @@ -1185,7 +1185,7 @@ rejected: rc = gsm48_tx_gmm_att_rej_oldmsg(msg, reject_cause); if (ctx) mm_ctx_cleanup_free(ctx, "GPRS ATTACH REJ"); - else + else if (llme) /* TLLI unassignment */ gprs_llgmm_assign(llme, llme->tlli, 0xffffffff, GPRS_ALGO_GEA0, NULL);
]]]
and I would appreciate if someone else could verify that.
~Neels
On 15 Apr 2016, at 23:59, Neels Hofmeyr nhofmeyr@sysmocom.de wrote:
Neels,
if (ctx) mm_ctx_cleanup_free(ctx, "GPRS ATTACH REJ");
else
else if (llme) /* TLLI unassignment */ gprs_llgmm_assign(llme, llme->tlli, 0xffffffff, GPRS_ALGO_GEA0, NULL);
I don't think this is the right thing to do. There should always be a llme here. Please have a look at the coverity report it will tell where thos llme has been NULL.
On Thu, Apr 14, 2016 at 03:21:29PM +0200, Neels Hofmeyr wrote:
Fixing a few NULL dereference warnings found by the Iu coverity check. They are not related apart from that.
Neels Hofmeyr (4): gprs_gmm: ensure llme present upon Attach Req (CID #57686)
I think there was some discussion about whether llme could ever be NULL in this case anyway?
gtphub_unmap_header_tei(): don't dereference unmapped_tei arg if not present (CID #57687) bsc_nat: forward_sccp_to_msc(): assert con presence (CID #57872) gbproxy_test: assert msg allocation (CID #57873)
all three above have been merged now.
-
Harald Welte -
Holger Freyther -
Neels Hofmeyr