diff mbox

[0/4] take care of some coverity warnings

Message ID 20160415215911.GA24964@dub6
State Not Applicable
Headers show

Commit Message

Neels Hofmeyr April 15, 2016, 9:59 p.m. UTC 
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

[[[
]]]

and I would appreciate if someone else could verify that.

~Neels

Comments

Holger Freyther April 22, 2016, 1:45 p.m. UTC | #1 
> 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.
diff mbox

Patch

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);