5 Jul
2013
5 Jul
'13
7:43 a.m.
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?
5 Jul
5 Jul
7:34 p.m.
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
6 Jul
6 Jul
10:19 a.m.
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
10:05 a.m.
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.
11:42 a.m.
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;
+ }
8:43 p.m.
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.
7 Jul
7 Jul
7:44 p.m.
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
9:07 p.m.
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
9 Jul
9 Jul
6:53 p.m.
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
9:32 p.m.
Holger Hans Peter Freyther wrote:
On Sun, Jul 07, 2013 at 09:07:32PM +0200, Andreas
Eversberg wrote:
here it is. thanx for reviewing it.
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.
9:23 p.m.
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
6 Jul
6 Jul
11:42 a.m.
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);
dl->send_buffer = NULL;
+ }
Does msg also need to be set to NULL after the above msgb_free()?
//Peter
2:02 p.m.
Holger Hans Peter Freyther wrote:
That's a good point too! I was thinking about if the variable is
tested again later in the same function.
//Peter
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. :)
3954
days inactive
3955
days old
14 comments
3 participants
participants (3)
-
Andreas Eversberg -
Holger Hans Peter Freyther -
Peter Stuge