5 Jul
2013
5 Jul
'13
3:55 p.m.
Hi Holger,
On Fri, Jul 05, 2013 at 07:29:15AM +0200, Holger Hans Peter Freyther wrote:
On Fri, Jul 05, 2013 at 12:23:54AM +0200, Pablo Neira
Ayuso wrote:
Looks good, thanks.
I'll proceed to remove HSL support from
libosmo-abis as well.
thanks, I already removed it (as part of looking through the
coverity issues). If there was an issue with my removal please
poke me. In coverity we have four more issues, it would be nice
if you
could have a look at the following:
src/input/ipaccess.c:
static int ipaccess_bts_cb(struct ipa_client_conn *link, struct msgb *msg)
...
862 } else if (link->port == IPA_TCP_PORT_OML)
863 e1i_ts = &link->line->ts[0];
4. Condition "link->port == 3003", taking false branch
864 else if (link->port == IPA_TCP_PORT_RSL)
865 e1i_ts = &link->line->ts[1];
866
867 /* look up for some existing signaling link. */
CID 1042352 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)
5. var_deref_model: Passing null pointer "e1i_ts" to function
"e1inp_lookup_sign_link(struct e1inp_ts *, uint8_t, uint8_t)", which
dereferences it. [show details]
868 sign_link = e1inp_lookup_sign_link(e1i_ts, hh->proto, 0);
port can probably only be IPA_TCP_PORT_OML|IPA_TCP_PORT_RSL. Can
you confirm this is a false positive? Shall we add an else branch
and add a OSMO_ASSERT(false) there?
This should not ever happen. Added an assertion as you suggested and
pushed the patch.
src/input/ipa.c:
static int ipa_server_fd_cb(struct osmo_fd *ofd, unsigned int what)
...
4. Condition "link->accept_cb", taking true branch
324 if (link->accept_cb)
325 link->accept_cb(link, ret);
326
CID 1040692 (#1 of 1): Resource leak (RESOURCE_LEAK)
5. leaked_handle: Handle variable "ret" going out of scope leaks the handle.
327 return 0;
Shall we have an else close(ret)? or should we enforce the presence
of a accept_cb when the socket is bound?
Should not happen either, but added the close as you suggested.
src/input/misdn.c:
static int _mi_e1_line_update(struct e1inp_line *line)
...
649 ret = mi_e1_setup(line, 1);
12. Condition "ret", taking false branch
650 if (ret)
CID 1040691 (#3 of 4): Resource leak (RESOURCE_LEAK) [select issue]
651 return ret;
652
CID 1040691 (#4 of 4): Resource leak (RESOURCE_LEAK)
13. leaked_handle: Handle variable "sk" going out of scope leaks the handle.
653 return 0;
I would add a close(sk); in the code but I was not sure if mISDN requires
this code to be open?
I don't have any msidn card. It seems we don't have any ->close
callback in the line set to close that socket, but I prefer to leave
as is by now until I/someone else can confirm this.
src/input/ipaccess.c:
static struct msgb *
713ipa_bts_id_resp(struct ipaccess_unit *dev, uint8_t *data, int len)
751 case IPAC_IDTAG_SWVERSION:
CID 1040690 (#4 of 5): Copy into fixed size buffer (STRING_OVERFLOW)
15. fixed_size_dest: You might overrun the 64 byte fixed-size string "str" by
copying "dev->swversion" without checking the length.
16. parameter_as_source: Note: This defect has an elevated risk because the source
argument is a parameter of the current function.
752 strcpy(str, dev->swversion);
753 break;
this appears to be a genuine issue.
Those strings are set in the configuration path, I have fix it, no
such an "elevated risk" as coverity spotted.
Also pushed a couple of patches to fix compilation warnings.
I have this warning as well:
e1_input.c: In function 'write_pcap_packet':
e1_input.c:163:6: warning: variable 'ret' set but not used
[-Wunused-but-set-variable]
I can fix it by returning error in abis_sendmsg and e1inp_rx_ts if it
cannot write the traces to the pcap file (should only affect
isdn-based BTSes) including some log message.
Let me know if you have any issue with those.
Thanks!