From: Pablo Neira Ayuso <pablo(a)gnumonks.org> Holger reported a leak in the ipaccess_drop path and a patch to fix this. This is a new version of the patch posted that also handle the case in which only one of the link (OML / RSL) is established and no ID_RESP was received. Based on patch of Holger Freyther. --- src/e1_input.c | 1 + src/input/hsl.c | 3 --- src/input/ipaccess.c | 10 ++++------ 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/e1_input.c b/src/e1_input.c index 957b74c..a85dd91 100644 --- a/src/e1_input.c +++ b/src/e1_input.c @@ -486,6 +486,7 @@ void e1inp_sign_link_destroy(struct e1inp_sign_link *link) if (link->ts->line->driver->close) link->ts->line->driver->close(link); + e1inp_line_put(link->ts->line); talloc_free(link); } diff --git a/src/input/hsl.c b/src/input/hsl.c index 3dcba1d..1a60c2b 100644 --- a/src/input/hsl.c +++ b/src/input/hsl.c @@ -83,9 +83,6 @@ static void hsl_drop(struct e1inp_line *line, struct osmo_fd *bfd) close(bfd->fd); bfd->fd = -1; } - /* put the virtual E1 line that we cloned for this socket, if - * it becomes unused, it gets released. */ - e1inp_line_put(line); } static int process_hsl_rsl(struct msgb *msg, struct e1inp_line *line, diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 76d1994..a3c3b70 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -237,9 +237,6 @@ static int ipaccess_drop(struct osmo_fd *bfd) int ret = 0; struct e1inp_line *line = bfd->data; - /* e1inp_sign_link_destroy releases the socket descriptors for us. */ - line->ops->sign_link_down(line); - /* Error case: we did not see any ID_RESP yet for this socket. */ if (bfd->fd != -1) { LOGP(DLINP, LOGL_ERROR, "Forcing socket shutdown with " @@ -249,9 +246,10 @@ static int ipaccess_drop(struct osmo_fd *bfd) bfd->fd = -1; ret = -ENOENT; } - /* put the virtual E1 line that we cloned for this socket, if - * it becomes unused, it gets released. */ - e1inp_line_put(line); + + /* e1inp_sign_link_destroy releases the socket descriptors for us. */ + line->ops->sign_link_down(line); + return ret; } -- 1.7.10.4

From: Pablo Neira Ayuso &lt;pablo(a)gnumonks.org&gt; If we hit any error, spot an error message containing the reason and close the links to start over. This patch has been tested by injecting errors in: * ipaccess_send, by randomly returning -1. * returning error from the ->sign_link_up callback, both from the OML and RSL links. * returning error from the ->sign_link callback, both for the OML and RSL links. With this patch, Valgrind shows no "definitely lost" memory blocks anymore (including the error path that has been tested) and the ipaccess driver behaves more robustly in case of errors. --- src/input/ipaccess.c | 199 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 141 insertions(+), 58 deletions(-) diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index a3c3b70..6475007 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -189,26 +189,39 @@ int ipaccess_send_id_req(int fd) /* base handling of the ip.access protocol */ int ipaccess_rcvmsg_base(struct msgb *msg, struct osmo_fd *bfd) { - int ipa_ccm = 0; uint8_t msg_type = *(msg->l2h); - int ret = 0; + int ret; switch (msg_type) { case IPAC_MSGT_PING: - ipa_ccm = 1; ret = ipaccess_send_pong(bfd->fd); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "Cannot send PING " + "message. Reason: %s\n", strerror(errno)); + break; + } + ret = 1; break; case IPAC_MSGT_PONG: DEBUGP(DLMI, "PONG!\n"); - ipa_ccm = 1; + ret = 1; break; case IPAC_MSGT_ID_ACK: DEBUGP(DLMI, "ID_ACK? -> ACK!\n"); - ipa_ccm = 1; ret = ipaccess_send_id_ack(bfd->fd); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "Cannot send ID_ACK " + "message. Reason: %s\n", strerror(errno)); + break; + } + ret = 1; + break; + default: + /* This is not an IPA PING, PONG or ID_ACK message */ + ret = 0; break; } - return ipa_ccm; + return ret; } /* base handling of the ip.access protocol */ @@ -221,6 +234,10 @@ int ipaccess_rcvmsg_bts_base(struct msgb *msg, switch (msg_type) { case IPAC_MSGT_PING: ret = ipaccess_send_pong(bfd->fd); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "Cannot send PONG " + "message. Reason: %s\n", strerror(errno)); + } break; case IPAC_MSGT_PONG: DEBUGP(DLMI, "PONG!\n"); @@ -229,12 +246,12 @@ int ipaccess_rcvmsg_bts_base(struct msgb *msg, DEBUGP(DLMI, "ID_ACK\n"); break; } - return 0; + return ret; } static int ipaccess_drop(struct osmo_fd *bfd) { - int ret = 0; + int ret = 1; struct e1inp_line *line = bfd->data; /* Error case: we did not see any ID_RESP yet for this socket. */ @@ -264,8 +281,23 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg, int len, ret; /* Handle IPA PING, PONG and ID_ACK messages. */ - if (ipaccess_rcvmsg_base(msg, bfd)) + ret = ipaccess_rcvmsg_base(msg, bfd); + switch(ret) { + case -1: + /* error in IPA control message handling */ + goto err; + case 1: + /* this is an IPA control message, skip further processing */ return 0; + case 0: + /* this is not an IPA control message, continue */ + break; + default: + LOGP(DLINP, LOGL_ERROR, "Unexpected return from " + "ipaccess_rcvmsg_base " + "(ret=%d)\n", ret); + goto err; + } switch (msg_type) { case IPAC_MSGT_ID_RESP: @@ -379,17 +411,16 @@ static int handle_ts1_read(struct osmo_fd *bfd) struct e1inp_sign_link *link; struct ipaccess_head *hh; struct msgb *msg; - int ret = 0, error; - - error = ipa_msg_recv(bfd->fd, &msg); - if (error < 0) - return error; - else if (error == 0) { - if (ipaccess_drop(bfd) >= 0) { - LOGP(DLINP, LOGL_NOTICE, "Sign link vanished, " - "dead socket\n"); - } - return error; + int ret; + + ret = ipa_msg_recv(bfd->fd, &msg); + if (ret < 0) { + LOGP(DLINP, LOGL_NOTICE, "Sign link problems, " + "closing socket. Reason: %s\n", strerror(errno)); + goto err; + } else if (ret == 0) { + LOGP(DLINP, LOGL_NOTICE, "Sign link vanished, dead socket\n"); + goto err; } DEBUGP(DLMI, "RX %u: %s\n", ts_nr, osmo_hexdump(msgb_l2(msg), msgb_l2len(msg))); @@ -397,7 +428,7 @@ static int handle_ts1_read(struct osmo_fd *bfd) if (hh->proto == IPAC_PROTO_IPACCESS) { ipaccess_rcvmsg(line, msg, bfd); msgb_free(msg); - return ret; + return 0; } /* BIG FAT WARNING: bfd might no longer exist here, since ipaccess_rcvmsg() * might have free'd it !!! */ @@ -406,8 +437,8 @@ static int handle_ts1_read(struct osmo_fd *bfd) if (!link) { LOGP(DLINP, LOGL_ERROR, "no matching signalling link for " "hh->proto=0x%02x\n", hh->proto); - msgb_free(msg); - return -EIO; + ret = -EINVAL; + goto err_msg; } msg->dst = link; @@ -416,20 +447,21 @@ static int handle_ts1_read(struct osmo_fd *bfd) LOGP(DLINP, LOGL_ERROR, "Fix your application, " "no action set for signalling messages.\n"); ret = -EINVAL; - goto err; + goto err_msg; } if (e1i_ts->line->ops->sign_link(msg) < 0) { LOGP(DLINP, LOGL_ERROR, "Bad signalling message," "sign_link returned error: %s\n", osmo_hexdump(msgb_l2(msg), msgb_l2len(msg))); ret = -EINVAL; + goto err; } - return ret; + + return 0; +err_msg: + msgb_free(msg); err: - osmo_fd_unregister(bfd); - close(bfd->fd); - bfd->fd = -1; - e1inp_line_put(line); + ipaccess_drop(bfd); return ret; } @@ -497,9 +529,9 @@ static int __handle_ts1_write(struct osmo_fd *bfd, struct e1inp_line *line) case E1INP_SIGN_RSL: break; default: - msgb_free(msg); bfd->when |= BSC_FD_WRITE; /* come back for more msg */ - return -EINVAL; + ret = -EINVAL; + goto out; } msg->l2h = msg->data; @@ -508,7 +540,11 @@ static int __handle_ts1_write(struct osmo_fd *bfd, struct e1inp_line *line) DEBUGP(DLMI, "TX %u: %s\n", ts_nr, osmo_hexdump(msg->l2h, msgb_l2len(msg))); ret = send(bfd->fd, msg->data, msg->len, 0); - msgb_free(msg); + if (ret != msg->len) { + LOGP(DLINP, LOGL_ERROR, "failed to send A-bis IPA signalling " + "message. Reason: %s\n", strerror(errno)); + goto err; + } /* set tx delay timer for next event */ e1i_ts->sign.tx_timer.cb = timeout_ts1_write; @@ -517,6 +553,12 @@ static int __handle_ts1_write(struct osmo_fd *bfd, struct e1inp_line *line) /* Reducing this might break the nanoBTS 900 init. */ osmo_timer_schedule(&e1i_ts->sign.tx_timer, 0, e1i_ts->sign.delay); +out: + msgb_free(msg); + return ret; +err: + ipaccess_drop(bfd); + msgb_free(msg); return ret; } @@ -571,7 +613,7 @@ static int ipaccess_bsc_oml_cb(struct ipa_server_link *link, int fd) line = e1inp_line_clone(tall_ipa_ctx, link->line); if (line == NULL) { LOGP(DLINP, LOGL_ERROR, "could not clone E1 line\n"); - return -1; + return -ENOMEM; } /* create virrtual E1 timeslots for signalling */ @@ -592,15 +634,25 @@ static int ipaccess_bsc_oml_cb(struct ipa_server_link *link, int fd) ret = osmo_fd_register(bfd); if (ret < 0) { LOGP(DLINP, LOGL_ERROR, "could not register FD\n"); - close(bfd->fd); - e1inp_line_put(line); - return ret; + goto err_line; } /* Request ID. FIXME: request LOCATION, HW/SW VErsion, Unit Name, Serno */ ret = ipaccess_send_id_req(bfd->fd); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "could not send ID REQ. Reason: %s\n", + strerror(errno)); + goto err_socket; + } + return ret; - return ret; +err_socket: + osmo_fd_unregister(bfd); + close(bfd->fd); + bfd->fd = -1; +err_line: + e1inp_line_put(line); + return ret; } static int ipaccess_bsc_rsl_cb(struct ipa_server_link *link, int fd) @@ -615,7 +667,7 @@ static int ipaccess_bsc_rsl_cb(struct ipa_server_link *link, int fd) line = e1inp_line_clone(tall_ipa_ctx, link->line); if (line == NULL) { LOGP(DLINP, LOGL_ERROR, "could not clone E1 line\n"); - return -1; + return -ENOMEM; } /* initialize the fds */ for (i = 0; i < ARRAY_SIZE(line->ts); ++i) @@ -636,14 +688,24 @@ static int ipaccess_bsc_rsl_cb(struct ipa_server_link *link, int fd) ret = osmo_fd_register(bfd); if (ret < 0) { LOGP(DLINP, LOGL_ERROR, "could not register FD\n"); - close(bfd->fd); - e1inp_line_put(line); - return ret; + goto err_line; } /* Request ID. FIXME: request LOCATION, HW/SW VErsion, Unit Name, Serno */ ret = ipaccess_send_id_req(bfd->fd); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "could not send ID REQ. Reason: %s\n", + strerror(errno)); + goto err_socket; + } + return ret; - return 0; +err_socket: + osmo_fd_unregister(bfd); + close(bfd->fd); + bfd->fd = -1; +err_line: + e1inp_line_put(line); + return ret; } static struct msgb * @@ -736,18 +798,21 @@ static int ipaccess_bts_cb(struct ipa_client_conn *link, struct msgb *msg) struct ipaccess_head *hh = (struct ipaccess_head *) msg->data; struct e1inp_ts *e1i_ts = NULL; struct e1inp_sign_link *sign_link; + struct msgb *rmsg; + int ret = 0; /* special handling for IPA CCM. */ if (hh->proto == IPAC_PROTO_IPACCESS) { uint8_t msg_type = *(msg->l2h); /* ping, pong and acknowledgment cases. */ - ipaccess_rcvmsg_bts_base(msg, link->ofd); + ret = ipaccess_rcvmsg_bts_base(msg, link->ofd); + if (ret < 0) + goto err; /* this is a request for identification from the BSC. */ if (msg_type == IPAC_MSGT_ID_GET) { struct e1inp_sign_link *sign_link; - struct msgb *rmsg; uint8_t *data = msgb_l2(msg); int len = msgb_l2len(msg); @@ -756,10 +821,8 @@ static int ipaccess_bts_cb(struct ipa_client_conn *link, struct msgb *msg) LOGP(DLINP, LOGL_ERROR, "Unable to set signal link, " "closing socket.\n"); - osmo_fd_unregister(link->ofd); - close(link->ofd->fd); - link->ofd->fd = -1; - return -EINVAL; + ret = -EINVAL; + goto err; } sign_link = link->line->ops->sign_link_up(msg, link->line, @@ -768,22 +831,32 @@ static int ipaccess_bts_cb(struct ipa_client_conn *link, struct msgb *msg) LOGP(DLINP, LOGL_ERROR, "Unable to set signal link, " "closing socket.\n"); - osmo_fd_unregister(link->ofd); - close(link->ofd->fd); - link->ofd->fd = -1; - return -EINVAL; + ret = -EINVAL; + goto err; } rmsg = ipa_bts_id_resp(link->line->ops->cfg.ipa.dev, data + 1, len - 1); - ipaccess_send(link->ofd->fd, rmsg->data, rmsg->len); + ret = ipaccess_send(link->ofd->fd, rmsg->data, + rmsg->len); + if (ret != rmsg->len) { + LOGP(DLINP, LOGL_ERROR, "cannot send ID_RESP " + "message. Reason: %s\n", strerror(errno)); + goto err_rmsg; + } msgb_free(rmsg); /* send ID_ACK. */ rmsg = ipa_bts_id_ack(); - ipaccess_send(link->ofd->fd, rmsg->data, rmsg->len); + ret = ipaccess_send(link->ofd->fd, rmsg->data, + rmsg->len); + if (ret != rmsg->len) { + LOGP(DLINP, LOGL_ERROR, "cannot send ID_ACK " + "message. Reason: %s\n", strerror(errno)); + goto err_rmsg; + } msgb_free(rmsg); } - return 0; + return ret; } else if (link->port == IPA_TCP_PORT_OML) e1i_ts = &link->line->ts[0]; else if (link->port == IPA_TCP_PORT_RSL) @@ -794,8 +867,8 @@ static int ipaccess_bts_cb(struct ipa_client_conn *link, struct msgb *msg) if (sign_link == NULL) { LOGP(DLINP, LOGL_ERROR, "no matching signalling link for " "hh->proto=0x%02x\n", hh->proto); - msgb_free(msg); - return -EIO; + ret = -EIO; + goto err; } msg->dst = sign_link; @@ -803,10 +876,20 @@ static int ipaccess_bts_cb(struct ipa_client_conn *link, struct msgb *msg) if (!link->line->ops->sign_link) { LOGP(DLINP, LOGL_ERROR, "Fix your application, " "no action set for signalling messages.\n"); - return -ENOENT; + ret = -ENOENT; + goto err; } link->line->ops->sign_link(msg); return 0; + +err_rmsg: + msgb_free(rmsg); +err: + osmo_fd_unregister(link->ofd); + close(link->ofd->fd); + link->ofd->fd = -1; + msgb_free(msg); + return ret; } struct ipaccess_line { -- 1.7.10.4

From: Pablo Neira Ayuso &lt;pablo(a)gnumonks.org&gt; --- src/input/lapd.c | 54 +++++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/input/lapd.c b/src/input/lapd.c index a7a86ce..5a475c4 100644 --- a/src/input/lapd.c +++ b/src/input/lapd.c @@ -71,39 +71,39 @@ #define LAPD_SET_K(n, o) {n,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o} const struct lapd_profile lapd_profile_isdn = { - LAPD_SET_K(7,7), - 3, - 260, - 3, - 1,0, - 1,0, - 2,0, - 10,0, - 0 + .k = LAPD_SET_K(7,7), + .n200 = 3, + .n201 = 260, + .n202 = 3, + .t200_sec = 1, .t200_usec = 0, + .t201_sec = 1, .t201_usec = 0, + .t202_sec = 2, .t202_usec = 0, + .t203_sec = 10, .t203_usec = 0, + .short_address = 0 }; const struct lapd_profile lapd_profile_abis = { - LAPD_SET_K(2,1), - 3, - 260, - 0, /* infinite */ - 0,240000, - 1,0, - 2,0, - 10,0, - 0 + .k = LAPD_SET_K(2,1), + .n200 = 3, + .n201 = 260, + .n202 = 0, /* infinite */ + .t200_sec = 0, .t200_usec = 240000, + .t201_sec = 1, .t201_usec = 0, + .t202_sec = 2, .t202_usec = 0, + .t203_sec = 10, .t203_usec = 0, + .short_address = 0 }; const struct lapd_profile lapd_profile_sat = { - LAPD_SET_K(15,15), - 5, - 260, - 5, - 2,400000, - 2,400000, - 2,400000, - 20,0, - 1 + .k = LAPD_SET_K(15,15), + .n200 = 5, + .n201 = 260, + .n202 = 5, + .t200_sec = 2, .t200_usec = 400000, + .t201_sec = 2, .t201_usec = 400000, + .t202_sec = 2, .t202_usec = 400000, + .t203_sec = 20, .t203_sec = 0, + .short_address = 1 }; typedef enum { -- 1.7.10.4

From: Pablo Neira Ayuso &lt;pablo(a)gnumonks.org&gt; This is a library, we leave up to the client code to decide when to finish the code execution. --- src/input/dahdi.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/input/dahdi.c b/src/input/dahdi.c index ff5cb80..8d3e060 100644 --- a/src/input/dahdi.c +++ b/src/input/dahdi.c @@ -425,14 +425,14 @@ struct e1inp_driver dahdi_driver = { .vty_show = &dahdi_vty_show, }; -void dahdi_set_bufinfo(int fd, int as_sigchan) +int dahdi_set_bufinfo(int fd, int as_sigchan) { struct dahdi_bufferinfo bi; int x = 0; if (ioctl(fd, DAHDI_GET_BUFINFO, &bi)) { LOGP(DLINP, LOGL_ERROR, "Error getting bufinfo\n"); - exit(-1); + return -EIO; } if (as_sigchan) { @@ -446,13 +446,13 @@ void dahdi_set_bufinfo(int fd, int as_sigchan) if (ioctl(fd, DAHDI_SET_BUFINFO, &bi)) { LOGP(DLINP, LOGL_ERROR, "Error setting bufinfo\n"); - exit(-1); + return -EIO; } if (!as_sigchan) { if (ioctl(fd, DAHDI_AUDIOMODE, &x)) { LOGP(DLINP, LOGL_ERROR, "Error setting bufinfo\n"); - exit(-1); + return -EIO; } } else { int one = 1; @@ -461,6 +461,7 @@ void dahdi_set_bufinfo(int fd, int as_sigchan) * as this command will fail if the slot _already_ was a * signalling slot before :( */ } + return 0; } static int dahdi_e1_setup(struct e1inp_line *line) @@ -522,10 +523,13 @@ static int dahdi_e1_setup(struct e1inp_line *line) LOGP(DLINP, LOGL_ERROR, "%s could not open %s %s\n", __func__, openstr, strerror(errno)); - exit(-1); + return -EIO; } bfd->when = BSC_FD_READ | BSC_FD_EXCEPT; - dahdi_set_bufinfo(bfd->fd, 1); + ret = dahdi_set_bufinfo(bfd->fd, 1); + if (ret < 0) + return ret; + if (!e1i_ts->lapd) e1i_ts->lapd = lapd_instance_alloc(1, dahdi_write_msg, bfd, e1inp_dlsap_up, @@ -543,9 +547,11 @@ static int dahdi_e1_setup(struct e1inp_line *line) LOGP(DLINP, LOGL_ERROR, "%s could not open %s %s\n", __func__, openstr, strerror(errno)); - exit(-1); + return -EIO; } - dahdi_set_bufinfo(bfd->fd, 0); + ret = dahdi_set_bufinfo(bfd->fd, 0); + if (ret < 0) + return -EIO; /* We never include the DAHDI B-Channel FD into the * writeset, since it doesn't support poll() based * write flow control */ -- 1.7.10.4

From: Pablo Neira Ayuso &lt;pablo(a)gnumonks.org&gt; We have to read from the eventfd, otherwise select keeps returning the file descriptor as ready to read. --- tests/e1inp_ipa_bts_test.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/e1inp_ipa_bts_test.c b/tests/e1inp_ipa_bts_test.c index bf46e9e..1b9a2db 100644 --- a/tests/e1inp_ipa_bts_test.c +++ b/tests/e1inp_ipa_bts_test.c @@ -205,9 +205,14 @@ static int abis_nm_sw_act_req(struct e1inp_sign_link *sign_link, static int test_bts_gsm_12_21_cb(struct osmo_fd *ofd, unsigned int what) { - int ret; + int ret, event_type; struct ipaccess_unit *unit = ofd->data; + if (read(eventfds[0], &event_type, sizeof(unsigned int)) < 0) { + LOGP(DBTSTEST, LOGL_ERROR, "error receiving event\n"); + return 0; + } + switch(bts_state) { case BTS_TEST_OML_SIGN_LINK_DOWN: /* Do nothing until OML link becomes ready. */ -- 1.7.10.4

From: Pablo Neira Ayuso &lt;pablo(a)gnumonks.org&gt; Fix a leak in the tests: The ->sign_link callback is reponsible for releasing the msgb. --- tests/e1inp_ipa_bsc_test.c | 1 + tests/e1inp_ipa_bts_test.c | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/e1inp_ipa_bsc_test.c b/tests/e1inp_ipa_bsc_test.c index b447893..b2fb11c 100644 --- a/tests/e1inp_ipa_bsc_test.c +++ b/tests/e1inp_ipa_bsc_test.c @@ -191,6 +191,7 @@ static int sign_link(struct msgb *msg) LOGP(DBSCTEST, LOGL_ERROR, "Unknown signallin message.\n"); break; } + msgb_free(msg); return ret; } diff --git a/tests/e1inp_ipa_bts_test.c b/tests/e1inp_ipa_bts_test.c index f885c5c..31aac95 100644 --- a/tests/e1inp_ipa_bts_test.c +++ b/tests/e1inp_ipa_bts_test.c @@ -151,6 +151,7 @@ static int sign_link(struct msgb *msg) LOGP(DBTSTEST, LOGL_ERROR, "Unknown signalling message.\n"); break; } + msgb_free(msg); return ret; } -- 1.7.10.4