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:
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.
Looks good, thanks.
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!