Holger Hans Peter Freyther wrote:
> Dear Andreas,
>
> the coverity prevent utility has found three inconsistencies in the
> lapd_core.c and the gsm_04_08.c and I think you are suited the best
> to comment on how to resolve them.
>
>
> src/gsm/lapd_core.c
> static int lapd_res_req(struct osmo_dlsap_prim *dp, struct lapd_msg_ctx *lctx)
> ...
> 1943 LOGP(DLLAPD, LOGL_INFO, "perform re-establishment (SABM)
> length=%d\n",
> 1944 msg->len);
> ...
>
this is really wrong. msg may be null. at least it depends on the upper
layer how to provide msg (NULL or 0-length), see patch.
>
> CID 1040665 (#1 of 1): Dereference before null check (REVERSE_INULL)
> check_after_deref: Null-checking "msg" suggests that it may be null, but it
> has already been dereferenced on all paths leading to the check.
> 1956 if (msg && msg->len)
>
> so we already dereferenced msg to print the length. So at this point,
> is it legetimate to a have NULL msgb or shall the null check be removed?
>
>
also if msg exists with 0 lenght, it will not be used, so it must be
freed, see patch.
> src/libmsc/gsm_04_08.c
> static int gsm48_cc_rx_connect(struct gsm_trans *trans, struct msgb *msg)
> ...
> 2090 if (trans->subscr) {
> 2091 connect.fields |= MNCC_F_CONNECTED;
> 2092 strncpy(connect.connected.number,
> trans->subscr->extension,
> 2093 sizeof(connect.connected.number)-1);
> 2094 strncpy(connect.imsi, trans->subscr->imsi,
> 2095 sizeof(connect.imsi)-1);
> 2096 }
> ...
>
> CID 1040740 (#1 of 1): Dereference after null check (FORWARD_NULL)
> 6. var_deref_op: Dereferencing null pointer "trans->subscr".
> 2117 osmo_counter_inc(trans->subscr->net->stats.call.mt_connect);
> 2118
> 2119 return mncc_recvmsg(trans->subscr->net, trans, MNCC_SETUP_CNF,
> &connect);
>
> trans->subscr is conditionally dereferenced and then unconditionally,
> what is correct?
>
i think we can remove the check for trans->subscr, since all rx
functions assume that it is set. instead it makes sense to add a sanity
check (trans->subscr must be set) to gsm0408_rcv_cc before calling the
rx function.
>
> static int gsm48_cc_rx_setup(struct gsm_trans *trans, struct msgb *msg)
> ...
> 1761 if (trans->subscr) {
> 1762 setup.fields |= MNCC_F_CALLING;
> 1763 strncpy(setup.calling.number, trans->subscr->extension,
> 1764 sizeof(setup.calling.number)-1);
> 1765 strncpy(setup.imsi, trans->subscr->imsi,
> 1766 sizeof(setup.imsi)-1);
> 1767 }
> ...
> CID 1040739 (#1 of 1): Dereference after null check (FORWARD_NULL)
> 14. var_deref_model: Passing null pointer "trans->subscr" to function
> "subscr_name(struct gsm_subscriber *)", which dereferences it. [show details]
> 1813 LOGP(DCC, LOGL_INFO, "Subscriber %s (%s) sends SETUP to %s\n",
> 1814 subscr_name(trans->subscr), trans->subscr->extension,
> 1815 setup.called.number);
> 1816
> 1817 osmo_counter_inc(trans->subscr->net->stats.call.mo_setup);
>
> Same thing as above. Can the null check be removed? Would you have the
> time to do that?
>
same as a bove.
diff --git a/src/gsm/lapd_core.c b/src/gsm/lapd_core.c
index 68b5e78..f7cefbe 100644
--- a/src/gsm/lapd_core.c
+++ b/src/gsm/lapd_core.c
@@ -1950,7 +1950,7 @@ static int lapd_res_req(struct osmo_dlsap_prim *dp,
struct lapd_msg_ctx *lctx)
struct lapd_msg_ctx nctx;
LOGP(DLLAPD, LOGL_INFO, "perform re-establishment (SABM) length=%d\n",
- msg->len);
+ (msg) ? msg->len : 0);
/* be sure that history is empty */
lapd_dl_flush_hist(dl);
@@ -1962,11 +1962,14 @@ static int lapd_res_req(struct osmo_dlsap_prim *dp,
struct lapd_msg_ctx *lctx)
if (dl->send_buffer)
msgb_free(dl->send_buffer);
dl->send_out = 0;
- if (msg && msg->len)
+ if (msg && msg->len) {
/* Write data into the send buffer, to be sent first */
dl->send_buffer = msg;
- else
+ } else {
+ if (msg)
+ msgb_free(msg);
dl->send_buffer = NULL;
+ }
/* Discard partly received L3 message */
if (dl->rcv_buffer) {
