This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.
Pablo Neira Ayuso pablo at gnumonks.orgHi 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!