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); ... 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?
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?
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?
On Fri, Jul 05, 2013 at 07:43:15AM +0200, Holger Hans Peter Freyther wrote:
Dear Andreas,
Dear Andreas,
we would also like to have your input on a mISDN file descriptor leak in the libosmo-abis code:
src/input/misdn.c static int _mi_e1_line_update(struct e1inp_line *line)
this function is not closing the 'sk' filedescriptor which is of type sk = socket(PF_ISDN, SOCK_RAW, ISDN_P_BASE);
do we need to keep this one open or is this is a bug? Pablo and me currently do not have a mISDN setup, so we can not easily answer this question right now.
Do you see anything wrong with this patch:
diff --git a/src/input/misdn.c b/src/input/misdn.c index a72a21e..5966817 100644 --- a/src/input/misdn.c +++ b/src/input/misdn.c @@ -640,6 +640,7 @@ static int _mi_e1_line_update(struct e1inp_line *line) fprintf(stdout, " nrbchan: %d\n", devinfo.nrbchan); fprintf(stdout, " name: %s\n", devinfo.name); #endif + close(sk);
if (!(devinfo.Dprotocols & (1 << ISDN_P_NT_E1))) { fprintf(stderr, "error: card is not of type E1 (NT-mode)\n");
thanks holger
Holger Hans Peter Freyther wrote:
src/input/misdn.c static int _mi_e1_line_update(struct e1inp_line *line)
this function is not closing the 'sk' filedescriptor which is of type sk = socket(PF_ISDN, SOCK_RAW, ISDN_P_BASE);
do we need to keep this one open or is this is a bug? Pablo and me currently do not have a mISDN setup, so we can not easily answer this question right now.
hi holger,
the socket is only used to get information about isdn interfaces, so it shall be closed, if not used anymore. your patch looks good to me.
best regards,
andreas
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.
On Sat, Jul 06, 2013 at 10:05:00AM +0200, Andreas Eversberg wrote:
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.
but we have not hit this case yet (e.g. no re-establishment occured right now). Do you have an idea of why this doesn't crash right now?
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.
okay. I will take care of that.
also if msg exists with 0 lenght, it will not be used, so it must be freed, see patch.
do you think you could extend the LAPD testcase for the case that would have crashed/leaked right now? msgb_free(NULL) is well defined, this means you do not need to have a NULL check there.
LOGP(DLLAPD, LOGL_INFO, "perform re-establishment (SABM) length=%d\n",
msg->len);
(msg) ? msg->len : 0);
why the '(' and ')'?
- } else {
if (msg)msgb_free(msg);
msgb_free(msg)
dl->send_buffer = NULL;
- }
Holger Hans Peter Freyther wrote:
but we have not hit this case yet (e.g. no re-establishment occured right now). Do you have an idea of why this doesn't crash right now?
this is because openbsc does not do re-establishment in case of a data link failure. osmocombb uses resume for assignment (which causes the same function as re-establishment), but for this case, lapdm.c will provide a zero-length msg.
On Sat, Jul 06, 2013 at 08:43:52PM +0200, Andreas Eversberg wrote:
this is because openbsc does not do re-establishment in case of a data link failure. osmocombb uses resume for assignment (which causes the same function as re-establishment), but for this case, lapdm.c will provide a zero-length msg.
Hi,
so maybe it simplifies the code just to assume/assert that the msgb is present in the primitive? As you freed the msgb, this means that OsmocomBB currently leaks this msgb during the handling of assignment?
thanks holger
Holger Hans Peter Freyther wrote:
so maybe it simplifies the code just to assume/assert that the msgb is present in the primitive? As you freed the msgb, this means that OsmocomBB currently leaks this msgb during the handling of assignment?
hi holger,
lapdm.c takes the re-establishment message and forwards it to lapd_core.c, so we can assume that msg is set. i case there is data in the re-establishment msg, it is moved into send_buffer. in case of no data it must be freed. (currently i don't know why resume/re-establishment requires content in SABM message.)
regards,
andreas
On Sun, Jul 07, 2013 at 09:07:32PM +0200, Andreas Eversberg wrote:
lapdm.c takes the re-establishment message and forwards it to lapd_core.c, so we can assume that msg is set. i case there is data in the re-establishment msg, it is moved into send_buffer. in case of no data it must be freed. (currently i don't know why resume/re-establishment requires content in SABM message.)
okay, please send your final patch for this issue to the mailinglist for review.
thanks holger
Holger Hans Peter Freyther wrote:
On Sun, Jul 07, 2013 at 09:07:32PM +0200, Andreas Eversberg wrote:
lapdm.c takes the re-establishment message and forwards it to lapd_core.c, so we can assume that msg is set. i case there is data in the re-establishment msg, it is moved into send_buffer. in case of no data it must be freed. (currently i don't know why resume/re-establishment requires content in SABM message.)
okay, please send your final patch for this issue to the mailinglist for review.
here it is. thanx for reviewing it.
Holger Hans Peter Freyther wrote:
the patch didn't include Peter's feedback. I have now added the above for the reasons mentioned during the review of your patch.
hi holger,
hmm. i just don't see why to set msg to NULL. it is not used afterwards. later in the code it is overwritten anyway.
regards,
andreas
Andreas Eversberg wrote:
+++ 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);- }
Does msg also need to be set to NULL after the above msgb_free()?
//Peter
Holger Hans Peter Freyther wrote:
Does msg also need to be set to NULL after the above msgb_free()?
I think it is good practice to set it to NULL to get a clean segfault in case we add more code. :)
That's a good point too! I was thinking about if the variable is tested again later in the same function.
//Peter
-
Andreas Eversberg -
Holger Hans Peter Freyther -
Peter Stuge