From: Pablo Neira Ayuso pablo@gnumonks.org
Hi,
This is the second version of the libosmo-abis patchset. Mostly aiming to improve robustness and fix leaks in error paths.
I have tried to address all Holger comments, I leave two things for my TODO list:
1) automate testing via automated testing while compilation 2) check the bfd->fd = -1 to make sure we need it in all cases
I have also intensively tested (although it was quite manual) this changes with the IPA BSC/BTS tests, running valgrind to check for leaks.
If you find any issue or you think it's OK, please ACK/NACK.
Thanks.
Pablo Neira Ayuso (13): ipa: fix missing set of write_cb for IPA client connection ipaccess: fix leak of e1inp_line tests: e1inp_ipa_bsc_test: fix crash ipaccess: improve error handling tests: e1inp_ipa_bts_test: fix compilation warnings lapd: use C99 structure initialization for profile templates input: dahdi: use logging facilities instead of fprintf and stderr input: dahdi: replace exit by return input: add generic PCAP interface for LAPD tests: fix CPU suckup e1inp_ipa_bts_test after test finish tests: e1inp_ipa_*_test: add signal handling for process termination tests: e1inp_ipa_*_test: fix leak of msgb in ->sign_link path ipaccess: fix leak of IPA control messages in the BTS side
include/Makefile.am | 2 +- include/osmocom/abis/lapd.h | 1 + include/osmocom/abis/lapd_pcap.h | 11 ++ src/Makefile.am | 1 + src/e1_input.c | 1 + src/input/dahdi.c | 54 ++++++---- src/input/hsl.c | 3 - src/input/ipa.c | 3 + src/input/ipaccess.c | 210 ++++++++++++++++++++++++++------------ src/input/lapd.c | 66 +++++++----- src/input/lapd_pcap.c | 159 +++++++++++++++++++++++++++++ tests/e1inp_ipa_bsc_test.c | 26 ++++- tests/e1inp_ipa_bts_test.c | 31 +++++- 13 files changed, 443 insertions(+), 125 deletions(-) create mode 100644 include/osmocom/abis/lapd_pcap.h create mode 100644 src/input/lapd_pcap.c
From: Pablo Neira Ayuso pablo@gnumonks.org
Now the BTS and BSC tests work again. --- src/input/ipa.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/input/ipa.c b/src/input/ipa.c index a887959..3c6a507 100644 --- a/src/input/ipa.c +++ b/src/input/ipa.c @@ -249,6 +249,9 @@ ipa_client_conn_create(void *ctx, struct e1inp_ts *ts, /* default to generic write callback if not set. */ if (write_cb == NULL) ipa_link->write_cb = ipa_client_write_default_cb; + else + ipa_link->write_cb = write_cb; + if (ts) ipa_link->line = ts->line; ipa_link->data = data;
From: Pablo Neira Ayuso pablo@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; }
From: Pablo Neira Ayuso pablo@gnumonks.org
The sequence to trigger the crash was:
1) establish a full A-bis IPA link (both OML and RSL correctly established and ID_RESP received from BTS). 2) nc 127.0.0.1 3002 # establish OML link only
I forgot to set to NULL the oml and rsl links we're using. Otherwise, the test calls e1inp_sign_link_destroy(rsl) which does not exists. --- tests/e1inp_ipa_bsc_test.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tests/e1inp_ipa_bsc_test.c b/tests/e1inp_ipa_bsc_test.c index 424c87b..a0e5653 100644 --- a/tests/e1inp_ipa_bsc_test.c +++ b/tests/e1inp_ipa_bsc_test.c @@ -62,10 +62,14 @@ sign_link_up(void *dev, struct e1inp_line *line, enum e1inp_sign_type type) static void sign_link_down(struct e1inp_line *line) { LOGP(DBSCTEST, LOGL_NOTICE, "signal link has been closed\n"); - if (oml_sign_link) + if (oml_sign_link) { e1inp_sign_link_destroy(oml_sign_link); - if (rsl_sign_link) + oml_sign_link = NULL; + } + if (rsl_sign_link) { e1inp_sign_link_destroy(rsl_sign_link); + rsl_sign_link = NULL; + } }
static void fill_om_hdr(struct abis_om_hdr *oh, uint8_t len)
From: Pablo Neira Ayuso pablo@gnumonks.org
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 {
On Fri, Aug 24, 2012 at 12:44:53AM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
@@ -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;
return ret;+err_socket:
- osmo_fd_unregister(bfd);
- close(bfd->fd);
- bfd->fd = -1;
+err_line:
- e1inp_line_put(line);
- return ret;
}
in this case the socket was closed and remains open now? It needs to jump under the osmo_fd_unregister line.
/* 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;
same as above.
On Fri, Aug 24, 2012 at 08:45:26AM +0200, Holger Hans Peter Freyther wrote:
On Fri, Aug 24, 2012 at 12:44:53AM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
@@ -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;
return ret;+err_socket:
- osmo_fd_unregister(bfd);
- close(bfd->fd);
- bfd->fd = -1;
+err_line:
- e1inp_line_put(line);
- return ret;
}
in this case the socket was closed and remains open now? It needs to jump under the osmo_fd_unregister line.
/* 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;same as above.
Yes, this has to be:
err_socket: osmo_fd_unregister(bfd); err_line: close(bfd->fd); bfd->fd = -1; e1inp_line_put(line); return ret;
Thanks Holger. New patch attached.
On Fri, Aug 24, 2012 at 12:09:08PM +0200, Pablo Neira Ayuso wrote:
@@ -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;
ah nice, potential memleak fix. :)
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;
bad. but not from your code. sign_link does delete the msgb.. dumping it will work most of the time but is a "read after free".
Acked-by: Holger Freyther holger@freyther.de
as far as I can see.. the error paths have no leaks
On Wed, Aug 29, 2012 at 09:30:58PM +0200, Holger Hans Peter Freyther wrote:
On Fri, Aug 24, 2012 at 12:09:08PM +0200, Pablo Neira Ayuso wrote:
@@ -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;ah nice, potential memleak fix. :)
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;bad. but not from your code. sign_link does delete the msgb.. dumping it will work most of the time but is a "read after free".
Indeed. abis_[rsl|oml]_rcvmsg are usually called in the path of ->sign_link, and they are in charge of releasing the msgb.
I'm going to remove that dump and send a patch to add it to openBSC's abis_[rsl|oml]_rcvmsg (before msgb is released).
Acked-by: Holger Freyther holger@freyther.de
as far as I can see.. the error paths have no leaks
Thanks for your review Holger. Will fix the thing above and push the patchset.
On Thu, Aug 30, 2012 at 04:42:21AM +0200, Pablo Neira Ayuso wrote: [...]
Acked-by: Holger Freyther holger@freyther.de
as far as I can see.. the error paths have no leaks
Thanks for your review Holger. Will fix the thing above and push the patchset.
JFYI: pushed to master now.
From: Pablo Neira Ayuso pablo@gnumonks.org
CC e1inp_ipa_bts_test.o e1inp_ipa_bts_test.c: In function ‘sign_link_up’: e1inp_ipa_bts_test.c:47:8: warning: variable ‘dst’ set but not used [-Wunused-but-set-variable] e1inp_ipa_bts_test.c: In function ‘test_bts_gsm_12_21_cb’: e1inp_ipa_bts_test.c:211:6: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable] CCLD e1inp_ipa_bts_test --- tests/e1inp_ipa_bts_test.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tests/e1inp_ipa_bts_test.c b/tests/e1inp_ipa_bts_test.c index 3549661..bf46e9e 100644 --- a/tests/e1inp_ipa_bts_test.c +++ b/tests/e1inp_ipa_bts_test.c @@ -44,7 +44,6 @@ static struct e1inp_sign_link * sign_link_up(void *unit, struct e1inp_line *line, enum e1inp_sign_type type) { struct e1inp_sign_link *sign_link = NULL; - void *dst = NULL;
switch(type) { case E1INP_SIGN_OML: @@ -58,7 +57,6 @@ sign_link_up(void *unit, struct e1inp_line *line, enum e1inp_sign_type type) LOGP(DBTSTEST, LOGL_ERROR, "cannot create OML sign link\n"); } - dst = oml_sign_link; if (oml_sign_link) { unsigned int event_type = 0;
@@ -84,7 +82,6 @@ sign_link_up(void *unit, struct e1inp_line *line, enum e1inp_sign_type type) LOGP(DBTSTEST, LOGL_ERROR, "cannot create RSL sign link\n"); } - dst = rsl_sign_link; break; default: return NULL; @@ -221,6 +218,10 @@ static int test_bts_gsm_12_21_cb(struct osmo_fd *ofd, unsigned int what) unit->bts_id, unit->trx_id, 0, NULL, 0); + if (ret < 0) { + LOGP(DBTSTEST, LOGL_ERROR, "cannot send SW ACT REQ\n"); + break; + } bts_state = BTS_TEST_OML_WAIT_SW_ACT_ACK; break; case BTS_TEST_OML_WAIT_SW_ACT_ACK:
From: Pablo Neira Ayuso pablo@gnumonks.org
--- 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 {
From: Pablo Neira Ayuso pablo@gnumonks.org
--- src/input/dahdi.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/src/input/dahdi.c b/src/input/dahdi.c index 0cefa6c..ff5cb80 100644 --- a/src/input/dahdi.c +++ b/src/input/dahdi.c @@ -196,7 +196,7 @@ static int ts_want_write(struct e1inp_ts *e1i_ts) * writeset, since it doesn't support poll() based * write flow control */ if (e1i_ts->type == E1INP_TS_TYPE_TRAU) { - fprintf(stderr, "Trying to write TRAU ts\n"); + LOGP(DLINP, LOGL_DEBUG, "Trying to write TRAU ts\n"); return 0; }
@@ -302,7 +302,7 @@ static int handle_tsX_write(struct osmo_fd *bfd) ret = subchan_mux_out(mx, tx_buf, D_BCHAN_TX_GRAN);
if (ret != D_BCHAN_TX_GRAN) { - fprintf(stderr, "Huh, got ret of %d\n", ret); + LOGP(DLINP, LOGL_DEBUG, "Huh, got ret of %d\n", ret); if (ret < 0) return ret; } @@ -316,8 +316,8 @@ static int handle_tsX_write(struct osmo_fd *bfd)
ret = write(bfd->fd, tx_buf, ret); if (ret < D_BCHAN_TX_GRAN) - fprintf(stderr, "send returns %d instead of %d\n", ret, - D_BCHAN_TX_GRAN); + LOGP(DLINP, LOGL_DEBUG, "send returns %d instead of %d\n", + ret, D_BCHAN_TX_GRAN);
return ret; } @@ -337,7 +337,8 @@ static int handle_tsX_read(struct osmo_fd *bfd)
ret = read(bfd->fd, msg->data, D_TSX_ALLOC_SIZE); if (ret < 0 || ret != D_TSX_ALLOC_SIZE) { - fprintf(stderr, "read error %d %s\n", ret, strerror(errno)); + LOGP(DLINP, LOGL_DEBUG, "read error %d %s\n", + ret, strerror(errno)); return ret; }
@@ -388,7 +389,8 @@ static int dahdi_fd_cb(struct osmo_fd *bfd, unsigned int what) * write flow control */ break; default: - fprintf(stderr, "unknown E1 TS type %u\n", e1i_ts->type); + LOGP(DLINP, LOGL_NOTICE, + "unknown E1 TS type %u\n", e1i_ts->type); break; }
@@ -429,7 +431,7 @@ void dahdi_set_bufinfo(int fd, int as_sigchan) int x = 0;
if (ioctl(fd, DAHDI_GET_BUFINFO, &bi)) { - fprintf(stderr, "Error getting bufinfo\n"); + LOGP(DLINP, LOGL_ERROR, "Error getting bufinfo\n"); exit(-1); }
@@ -443,13 +445,13 @@ void dahdi_set_bufinfo(int fd, int as_sigchan) }
if (ioctl(fd, DAHDI_SET_BUFINFO, &bi)) { - fprintf(stderr, "Error setting bufinfo\n"); + LOGP(DLINP, LOGL_ERROR, "Error setting bufinfo\n"); exit(-1); }
if (!as_sigchan) { if (ioctl(fd, DAHDI_AUDIOMODE, &x)) { - fprintf(stderr, "Error setting bufinfo\n"); + LOGP(DLINP, LOGL_ERROR, "Error setting bufinfo\n"); exit(-1); } } else { @@ -517,7 +519,8 @@ static int dahdi_e1_setup(struct e1inp_line *line) if (!bfd->fd) bfd->fd = open(openstr, O_RDWR | O_NONBLOCK); if (bfd->fd == -1) { - fprintf(stderr, "%s could not open %s %s\n", + LOGP(DLINP, LOGL_ERROR, + "%s could not open %s %s\n", __func__, openstr, strerror(errno)); exit(-1); } @@ -537,7 +540,8 @@ static int dahdi_e1_setup(struct e1inp_line *line) if (!bfd->fd) bfd->fd = open(openstr, O_RDWR | O_NONBLOCK); if (bfd->fd == -1) { - fprintf(stderr, "%s could not open %s %s\n", + LOGP(DLINP, LOGL_ERROR, + "%s could not open %s %s\n", __func__, openstr, strerror(errno)); exit(-1); } @@ -550,14 +554,16 @@ static int dahdi_e1_setup(struct e1inp_line *line) }
if (bfd->fd < 0) { - fprintf(stderr, "%s could not open %s %s\n", + LOGP(DLINP, LOGL_ERROR, + "%s could not open %s %s\n", __func__, openstr, strerror(errno)); return bfd->fd; }
ret = osmo_fd_register(bfd); if (ret < 0) { - fprintf(stderr, "could not register FD: %s\n", + LOGP(DLINP, LOGL_ERROR, + "could not register FD: %s\n", strerror(ret)); return ret; }
From: Pablo Neira Ayuso pablo@gnumonks.org
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 */
From: Pablo Neira Ayuso pablo@gnumonks.org
This patch allows you to create PCAP traces between the BSC and BTS including the LAPD frames. Useful for debugging communication issues.
So far, it was only possible to create layer 3 traces containing OML/RSL. LAPD traces can be also interesting to debug communication issues between the BSC and the BTS.
To enable PCAP of LAPD, you only have to invoke:
li->pcap_fd = osmo_pcap_lapd_open("/tmp/file.pcap", 0600);
By default, li->pcap_fd is set to -1 which means disabled.
openBSC needs a patch to replace all usage of e1_set_pcap_fd by osmo_pcap_lapd_open. --- include/Makefile.am | 2 +- include/osmocom/abis/lapd.h | 1 + include/osmocom/abis/lapd_pcap.h | 11 +++ src/Makefile.am | 1 + src/input/lapd.c | 12 +++ src/input/lapd_pcap.c | 159 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 185 insertions(+), 1 deletion(-) create mode 100644 include/osmocom/abis/lapd_pcap.h create mode 100644 src/input/lapd_pcap.c
diff --git a/include/Makefile.am b/include/Makefile.am index 61e6cf5..16fa506 100644 --- a/include/Makefile.am +++ b/include/Makefile.am @@ -3,4 +3,4 @@ noinst_HEADERS=mISDNif.h internal.h nobase_include_HEADERS = osmocom/abis/ipa.h osmocom/abis/trau_frame.h \ osmocom/abis/ipa_proxy.h osmocom/abis/ipaccess.h osmocom/abis/abis.h \ osmocom/abis/subchan_demux.h osmocom/abis/e1_input.h \ - osmocom/abis/lapd.h osmocom/trau/osmo_ortp.h + osmocom/abis/lapd.h osmocom/abis/lapd_pcap.h osmocom/trau/osmo_ortp.h diff --git a/include/osmocom/abis/lapd.h b/include/osmocom/abis/lapd.h index 847a597..2457cde 100644 --- a/include/osmocom/abis/lapd.h +++ b/include/osmocom/abis/lapd.h @@ -35,6 +35,7 @@ struct lapd_instance { struct lapd_profile profile; /* must be a copy */
struct llist_head tei_list; /* list of TEI in this LAPD instance */ + int pcap_fd; /* PCAP file descriptor */ };
enum lapd_recv_errors { diff --git a/include/osmocom/abis/lapd_pcap.h b/include/osmocom/abis/lapd_pcap.h new file mode 100644 index 0000000..1c0d555 --- /dev/null +++ b/include/osmocom/abis/lapd_pcap.h @@ -0,0 +1,11 @@ +#ifndef _LAPD_PCAP_H_ +#define _LAPD_PCAP_H_ + +#define OSMO_LAPD_PCAP_INPUT 0 +#define OSMO_LAPD_PCAP_OUTPUT 1 + +int osmo_pcap_lapd_open(char *filename, mode_t mode); +int osmo_pcap_lapd_write(int fd, int direction, struct msgb *msg); +int osmo_pcap_lapd_close(int fd); + +#endif diff --git a/src/Makefile.am b/src/Makefile.am index cf58aaa..01f0913 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -23,6 +23,7 @@ libosmoabis_la_SOURCES = init.c \ input/ipa.c \ input/ipaccess.c \ input/lapd.c \ + input/lapd_pcap.c \ input/misdn.c \ input/rs232.c
diff --git a/src/input/lapd.c b/src/input/lapd.c index 5a475c4..bced94a 100644 --- a/src/input/lapd.c +++ b/src/input/lapd.c @@ -36,6 +36,7 @@ #include <osmocom/core/msgb.h> #include <osmocom/core/timer.h> #include <osmocom/abis/lapd.h> +#include <osmocom/abis/lapd_pcap.h>
#define LAPD_ADDR2(sapi, cr) ((((sapi) & 0x3f) << 2) | (((cr) & 0x1) << 1)) #define LAPD_ADDR3(tei) ((((tei) & 0x7f) << 1) | 0x1) @@ -311,6 +312,10 @@ static int lapd_tei_receive(struct lapd_instance *li, uint8_t *data, int len) msg = msgb_alloc_headroom(56, 56, "DL EST"); msg->l2h = msgb_push(msg, 8); memcpy(msg->l2h, resp, 8); + + /* write to PCAP file, if enabled. */ + osmo_pcap_lapd_write(li->pcap_fd, OSMO_LAPD_PCAP_OUTPUT, msg); + LOGP(DLLAPD, LOGL_DEBUG, "TX: %s\n", osmo_hexdump(msg->data, msg->len)); li->transmit_cb(msg, li->transmit_cbdata); @@ -336,6 +341,9 @@ int lapd_receive(struct lapd_instance *li, struct msgb *msg, int *error) struct lapd_sap *sap; struct lapd_tei *teip;
+ /* write to PCAP file, if enabled. */ + osmo_pcap_lapd_write(li->pcap_fd, OSMO_LAPD_PCAP_INPUT, msg); + LOGP(DLLAPD, LOGL_DEBUG, "RX: %s\n", osmo_hexdump(msg->data, msg->len)); if (msg->len < 2) { LOGP(DLLAPD, LOGL_ERROR, "LAPD frame receive len %d < 2, " @@ -585,6 +593,9 @@ static int send_ph_data_req(struct lapd_msg_ctx *lctx, struct msgb *msg) else msg->l2h[1] = LAPD_ADDR3(lctx->tei);
+ /* write to PCAP file, if enabled. */ + osmo_pcap_lapd_write(li->pcap_fd, OSMO_LAPD_PCAP_OUTPUT, msg); + /* forward frame to L1 */ LOGP(DLLAPD, LOGL_DEBUG, "TX: %s\n", osmo_hexdump(msg->data, msg->len)); li->transmit_cb(msg, li->transmit_cbdata); @@ -645,6 +656,7 @@ struct lapd_instance *lapd_instance_alloc(int network_side, li->transmit_cbdata = tx_cbdata; li->receive_cb = rx_cb; li->receive_cbdata = rx_cbdata; + li->pcap_fd = -1; memcpy(&li->profile, profile, sizeof(li->profile));
INIT_LLIST_HEAD(&li->tei_list); diff --git a/src/input/lapd_pcap.c b/src/input/lapd_pcap.c new file mode 100644 index 0000000..c83bc60 --- /dev/null +++ b/src/input/lapd_pcap.c @@ -0,0 +1,159 @@ +/* (C) 2008-2012 by Harald Welte laforge@gnumonks.org + * + * All Rights Reserved + * + * Author: Harald Welte laforge@gnumonks.org + * Pablo Neira Ayuso pablo@gnumonks.org + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see http://www.gnu.org/licenses/. + * + */ + +#include <stdio.h> +#include <stdint.h> +#include <stddef.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h> +#include <sys/time.h> +#include <arpa/inet.h> +#include <string.h> +#include <errno.h> + +#include <osmocom/core/msgb.h> +#include <osmocom/core/logging.h> +#include <osmocom/core/utils.h> + +#include <osmocom/abis/lapd_pcap.h> + +/* + * pcap writing of the mlapd load + * pcap format is from http://wiki.wireshark.org/Development/LibpcapFileFormat + */ +#define DLT_LINUX_LAPD 177 + +struct pcap_hdr { + uint32_t magic_number; + uint16_t version_major; + uint16_t version_minor; + int32_t thiszone; + uint32_t sigfigs; + uint32_t snaplen; + uint32_t network; +} __attribute__((packed)); + +struct pcap_rechdr { + uint32_t ts_sec; + uint32_t ts_usec; + uint32_t incl_len; + uint32_t orig_len; +} __attribute__((packed)); + +struct pcap_lapdhdr { + uint16_t pkttype; + uint16_t hatype; + uint16_t halen; + uint64_t addr; + int16_t protocol; +} __attribute__((packed)); + +osmo_static_assert(offsetof(struct pcap_lapdhdr, hatype) == 2, hatype_offset); +osmo_static_assert(offsetof(struct pcap_lapdhdr, halen) == 4, halen_offset); +osmo_static_assert(offsetof(struct pcap_lapdhdr, addr) == 6, addr_offset); +osmo_static_assert(offsetof(struct pcap_lapdhdr, protocol) == 14, proto_offset); +osmo_static_assert(sizeof(struct pcap_lapdhdr) == 16, lapd_header_size); + +int osmo_pcap_lapd_open(char *filename, mode_t mode) +{ + int fd; + struct pcap_hdr pcap_header = { + .magic_number = 0xa1b2c3d4, + .version_major = 2, + .version_minor = 4, + .thiszone = 0, + .sigfigs = 0, + .snaplen = 65535, + .network = DLT_LINUX_LAPD, + }; + + LOGP(DLLAPD, LOGL_NOTICE, "opening LAPD pcap file `%s'\n", filename); + + fd = open(filename, O_WRONLY|O_TRUNC|O_CREAT, mode); + if (fd < 0) { + LOGP(DLLAPD, LOGL_ERROR, "failed to open PCAP file: %s\n", + strerror(errno)); + return -1; + } + if (write(fd, &pcap_header, sizeof(pcap_header)) + != sizeof(pcap_header)) { + LOGP(DLLAPD, LOGL_ERROR, "cannot write PCAP header: %s\n", + strerror(errno)); + close(fd); + return -1; + } + return fd; +} + +/* This currently only works for the D-Channel */ +int osmo_pcap_lapd_write(int fd, int direction, struct msgb *msg) +{ + int numbytes = 0; + struct timeval tv; + struct pcap_rechdr pcap_rechdr; + struct pcap_lapdhdr header; + char buf[sizeof(struct pcap_rechdr) + + sizeof(struct pcap_lapdhdr) + msg->len]; + + /* PCAP file has not been opened, skip. */ + if (fd < 0) + return 0; + + pcap_rechdr.ts_sec = 0; + pcap_rechdr.ts_usec = 0; + pcap_rechdr.incl_len = msg->len + sizeof(struct pcap_lapdhdr); + pcap_rechdr.orig_len = msg->len + sizeof(struct pcap_lapdhdr); + + header.pkttype = 4; + header.hatype = 0; + header.halen = 0; + header.addr = direction == OSMO_LAPD_PCAP_OUTPUT ? 0x0 : 0x1; + header.protocol = ntohs(48); + + gettimeofday(&tv, NULL); + pcap_rechdr.ts_sec = tv.tv_sec; + pcap_rechdr.ts_usec = tv.tv_usec; + + memcpy(buf + numbytes, &pcap_rechdr, sizeof(pcap_rechdr)); + numbytes += sizeof(pcap_rechdr); + + memcpy(buf + numbytes, &header, sizeof(header)); + numbytes += sizeof(header); + + memcpy(buf + numbytes, msg->data, msg->len); + numbytes += msg->len; + + if (write(fd, buf, numbytes) != numbytes) { + LOGP(DLLAPD, LOGL_ERROR, "cannot write packet to PCAP: %s\n", + strerror(errno)); + return -1; + } + return numbytes; +} + +int osmo_pcap_lapd_close(int fd) +{ + LOGP(DLLAPD, LOGL_NOTICE, "closing LAPD pcap file\n"); + return close(fd); +}
From: Pablo Neira Ayuso pablo@gnumonks.org
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. */
From: Pablo Neira Ayuso pablo@gnumonks.org
This patch adds signal handling to release memory in the exit path of the tests. This is good to check what memory we are leaking in the exist path of the tests. --- tests/e1inp_ipa_bsc_test.c | 17 +++++++++++++++-- tests/e1inp_ipa_bts_test.c | 16 +++++++++++++++- 2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/tests/e1inp_ipa_bsc_test.c b/tests/e1inp_ipa_bsc_test.c index a0e5653..b447893 100644 --- a/tests/e1inp_ipa_bsc_test.c +++ b/tests/e1inp_ipa_bsc_test.c @@ -1,4 +1,5 @@ #include <stdio.h> +#include <signal.h> #include <osmocom/core/talloc.h> #include <osmocom/abis/abis.h> #include <osmocom/abis/e1_input.h> @@ -199,10 +200,16 @@ const struct log_info bsc_test_log_info = { .num_cat = ARRAY_SIZE(bsc_test_cat), };
-int main(void) +static struct e1inp_line *line; + +static void sighandler(int foo) { - struct e1inp_line *line; + e1inp_line_put(line); + exit(EXIT_SUCCESS); +}
+int main(void) +{ tall_test = talloc_named_const(NULL, 1, "e1inp_test"); libosmo_abis_init(tall_test);
@@ -220,6 +227,12 @@ int main(void) .sign_link = sign_link, };
+ if (signal(SIGINT, sighandler) == SIG_ERR || + signal(SIGTERM, sighandler) == SIG_ERR) { + perror("Cannot set sighandler"); + exit(EXIT_FAILURE); + } + #define LINENR 0
line = e1inp_line_create(LINENR, "ipa"); diff --git a/tests/e1inp_ipa_bts_test.c b/tests/e1inp_ipa_bts_test.c index 1b9a2db..f885c5c 100644 --- a/tests/e1inp_ipa_bts_test.c +++ b/tests/e1inp_ipa_bts_test.c @@ -1,4 +1,5 @@ #include <stdio.h> +#include <signal.h> #include <osmocom/core/talloc.h> #include <string.h> #include <unistd.h> @@ -236,6 +237,14 @@ static int test_bts_gsm_12_21_cb(struct osmo_fd *ofd, unsigned int what) return 0; }
+static struct e1inp_line *line; + +static void sighandler(int foo) +{ + e1inp_line_put(line); + exit(EXIT_SUCCESS); +} + int main(void) { struct ipaccess_unit bts_dev_info = { @@ -250,7 +259,6 @@ int main(void) .location2 = "testBTS", .serno = "", }; - struct e1inp_line *line;
tall_test = talloc_named_const(NULL, 1, "e1inp_test"); libosmo_abis_init(tall_test); @@ -272,6 +280,12 @@ int main(void)
#define LINENR 0
+ if (signal(SIGINT, sighandler) == SIG_ERR || + signal(SIGTERM, sighandler) == SIG_ERR) { + perror("Cannot set sighandler"); + exit(EXIT_FAILURE); + } + line = e1inp_line_create(LINENR, "ipa"); if (line == NULL) { LOGP(DBTSTEST, LOGL_ERROR, "problem enabling E1 line\n");
From: Pablo Neira Ayuso pablo@gnumonks.org
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; }
From: Pablo Neira Ayuso pablo@gnumonks.org
--- src/input/ipaccess.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 6475007..265c982 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -856,6 +856,7 @@ static int ipaccess_bts_cb(struct ipa_client_conn *link, struct msgb *msg) } msgb_free(rmsg); } + msgb_free(msg); return ret; } else if (link->port == IPA_TCP_PORT_OML) e1i_ts = &link->line->ts[0];
On Fri, Aug 24, 2012 at 12:44:49AM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Hi,
I would like to have another look at some of the patches. I am not sure if it helps you in a way. I think I only want to have another look at patch 04 (error handling) and 13 (msgbg leak fix). Iff it helps feel free to push all but the above two patches.
holger
On Fri, Aug 24, 2012 at 08:12:18PM +0200, Holger Hans Peter Freyther wrote:
On Fri, Aug 24, 2012 at 12:44:49AM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Hi,
I would like to have another look at some of the patches. I am not sure if it helps you in a way. I think I only want to have another look at patch 04 (error handling) and 13 (msgbg leak fix). Iff it helps feel free to push all but the above two patches.
No need to rush, I can wait for your review.
Then, I can send another version of the patchset.
Let me know.
-
Holger Hans Peter Freyther -
Pablo Neira Ayuso -
pablo＠gnumonks.org