From: Pablo Neira Ayuso pablo@gnumonks.org
This patchset is the third in the series to cleanup code in ipaccess tools and drivers.
They have been tested by bootstraping the nanoBTS successfully in a small setup with two laptops, one acting as proxy and another with osmo-nitb.
They are available in the pablo/cleanups branch in openbsc git tree.
BTW: I'm going to stop a bit with cleanups to add a VTY interface for ipaccess-proxy, as Harald suggested.
Please, merge it! Thanks.
Pablo Neira Ayuso (7): libabis: ipaccess: more robust ipaccess_idtag_name() src: more robust ipaccess_idtag_parse() libabis: ipaccess: use log instead of debug under errors in ipaccess_send() ipaccess-find: get rid of ipac_idtag_name() ipaccess-proxy: more robust option parsing and checking ipaccess-proxy: remove leftover option `--udp' ipaccess-proxy: get rid of make_gprs_sock()
openbsc/include/openbsc/ipaccess.h | 2 +- openbsc/src/ipaccess/Makefile.am | 6 +++ openbsc/src/ipaccess/ipaccess-find.c | 22 +------------ openbsc/src/ipaccess/ipaccess-proxy.c | 56 +++++++++++++-------------------- openbsc/src/libabis/input/ipaccess.c | 18 ++++++---- openbsc/src/osmo-bsc_nat/bsc_nat.c | 8 ++++- openbsc/src/osmo-bsc_nat/bsc_ussd.c | 8 ++++- 7 files changed, 55 insertions(+), 65 deletions(-)
From: Pablo Neira Ayuso pablo@gnumonks.org
ipaccess_idtag_name() now takes a uint8_t as parameter which is the length of the type tag.
This patch was suggested by Zecke. --- openbsc/include/openbsc/ipaccess.h | 2 +- openbsc/src/libabis/input/ipaccess.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/openbsc/include/openbsc/ipaccess.h b/openbsc/include/openbsc/ipaccess.h index 00642ff..0edd81d 100644 --- a/openbsc/include/openbsc/ipaccess.h +++ b/openbsc/include/openbsc/ipaccess.h @@ -79,7 +79,7 @@ int ipaccess_send_pong(int fd); int ipaccess_send_id_ack(int fd); int ipaccess_send_id_req(int fd);
-const char *ipaccess_idtag_name(int tag); +const char *ipaccess_idtag_name(uint8_t tag); int ipaccess_idtag_parse(struct tlv_parsed *dec, unsigned char *buf, int len); int ipaccess_parse_unitid(const char *str, u_int16_t *site_id, u_int16_t *bts_id, u_int16_t *trx_id);
diff --git a/openbsc/src/libabis/input/ipaccess.c b/openbsc/src/libabis/input/ipaccess.c index e1e314b..c4c13d7 100644 --- a/openbsc/src/libabis/input/ipaccess.c +++ b/openbsc/src/libabis/input/ipaccess.c @@ -100,7 +100,7 @@ static const char *idtag_names[] = { [IPAC_IDTAG_UNIT] = "Unit_ID", };
-const char *ipaccess_idtag_name(int tag) +const char *ipaccess_idtag_name(uint8_t tag) { if (tag >= ARRAY_SIZE(idtag_names)) return "unknown";
From: Pablo Neira Ayuso pablo@gnumonks.org
Now ipaccess_idtag_parse() returns -EINVAL instead of -1. We also check for the return value of this function in every invocation to skip further processing in case of messages with malformed TLVs.
This idea was suggested by Zecke. --- openbsc/src/libabis/input/ipaccess.c | 14 +++++++++----- openbsc/src/osmo-bsc_nat/bsc_nat.c | 8 +++++++- openbsc/src/osmo-bsc_nat/bsc_ussd.c | 8 +++++++- 3 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/openbsc/src/libabis/input/ipaccess.c b/openbsc/src/libabis/input/ipaccess.c index c4c13d7..ba641a8 100644 --- a/openbsc/src/libabis/input/ipaccess.c +++ b/openbsc/src/libabis/input/ipaccess.c @@ -123,7 +123,7 @@ int ipaccess_idtag_parse(struct tlv_parsed *dec, unsigned char *buf, int len)
if (t_len > len + 1) { LOGP(DMI, LOGL_ERROR, "The tag does not fit: %d\n", t_len); - return -1; + return -EINVAL; }
DEBUGPC(DMI, "%s='%s' ", ipaccess_idtag_name(t_tag), cur); @@ -251,7 +251,7 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg, u_int16_t site_id = 0, bts_id = 0, trx_id = 0; struct gsm_bts *bts; char *unitid; - int len; + int len, ret;
/* handle base messages */ ipaccess_rcvmsg_base(msg, bfd); @@ -260,10 +260,14 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg, case IPAC_MSGT_ID_RESP: DEBUGP(DMI, "ID_RESP "); /* parse tags, search for Unit ID */ - ipaccess_idtag_parse(&tlvp, (u_int8_t *)msg->l2h + 2, - msgb_l2len(msg)-2); + ret = ipaccess_idtag_parse(&tlvp, (u_int8_t *)msg->l2h + 2, + msgb_l2len(msg)-2); DEBUGP(DMI, "\n"); - + if (ret < 0) { + LOGP(DINP, LOGL_ERROR, "ignoring IPA response message " + "with malformed TLVs\n"); + return ret; + } if (!TLVP_PRESENT(&tlvp, IPAC_IDTAG_UNIT)) break;
diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c index 7586294..8c164a2 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c @@ -1019,9 +1019,15 @@ exit: /* do we know who is handling this? */ if (msg->l2h[0] == IPAC_MSGT_ID_RESP) { struct tlv_parsed tvp; - ipaccess_idtag_parse(&tvp, + int ret; + ret = ipaccess_idtag_parse(&tvp, (unsigned char *) msg->l2h + 2, msgb_l2len(msg) - 2); + if (ret < 0) { + LOGP(DNAT, LOGL_ERROR, "ignoring IPA response " + "message with malformed TLVs\n"); + return ret; + } if (TLVP_PRESENT(&tvp, IPAC_IDTAG_UNITNAME)) ipaccess_auth_bsc(&tvp, bsc); } diff --git a/openbsc/src/osmo-bsc_nat/bsc_ussd.c b/openbsc/src/osmo-bsc_nat/bsc_ussd.c index ff1d27a..4beef7b 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_ussd.c +++ b/openbsc/src/osmo-bsc_nat/bsc_ussd.c @@ -123,9 +123,15 @@ static int ussd_read_cb(struct bsc_fd *bfd) if (hh->proto == IPAC_PROTO_IPACCESS) { if (msg->l2h[0] == IPAC_MSGT_ID_RESP) { struct tlv_parsed tvp; - ipaccess_idtag_parse(&tvp, + int ret; + ret = ipaccess_idtag_parse(&tvp, (unsigned char *) msg->l2h + 2, msgb_l2len(msg) - 2); + if (ret < 0) { + LOGP(DNAT, LOGL_ERROR, "ignoring IPA response " + "message with malformed TLVs\n"); + return ret; + } if (TLVP_PRESENT(&tvp, IPAC_IDTAG_UNITNAME)) ussd_auth_con(&tvp, conn); }
On 04/11/2011 05:04 PM, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Thanks, IIRC there is another issue here. We assume that the strings on the wire contain a '\0', e.g. in the printf statements we rely on this. So we could either put this in there or do something else.
thanks holger
Hi Holger,
On 11/04/11 17:52, Holger Hans Peter Freyther wrote:
On 04/11/2011 05:04 PM, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Thanks, IIRC there is another issue here. We assume that the strings on the wire contain a '\0', e.g. in the printf statements we rely on this. So we could either put this in there or do something else.
Can I assume that all IPAC_IDTAG_* are all strings?
I mean all of these:
[IPAC_IDTAG_SERNR] = "Serial_Number", [IPAC_IDTAG_UNITNAME] = "Unit_Name", [IPAC_IDTAG_LOCATION1] = "Location_1", [IPAC_IDTAG_LOCATION2] = "Location_2", [IPAC_IDTAG_EQUIPVERS] = "Equipment_Version", [IPAC_IDTAG_SWVERSION] = "Software_Version", [IPAC_IDTAG_IPADDR] = "IP_Address", [IPAC_IDTAG_MACADDR] = "MAC_Address", [IPAC_IDTAG_UNIT] = "Unit_ID",
If so, I'll send a patch to check for the trailing \0 to:
dec->lv[t_tag].val = cur;
Sorry, I didn't get you in the first hand, I'm not ignoring your requests :-).
Let me know if this is not what you mean.
On 04/12/2011 07:55 PM, Pablo Neira Ayuso wrote:
Thanks, IIRC there is another issue here. We assume that the strings on the wire contain a '\0', e.g. in the printf statements we rely on this. So we could either put this in there or do something else.
Can I assume that all IPAC_IDTAG_* are all strings?
I'm sorry. I do not know.
If so, I'll send a patch to check for the trailing \0 to:
dec->lv[t_tag].val = cur;
Sorry, I didn't get you in the first hand, I'm not ignoring your requests :-).
Let me know if this is not what you mean.
It would be good to find out which of these tags have the null termination and which not.
holger
From: Pablo Neira Ayuso pablo@gnumonks.org
This patch was suggested by Zecke. --- openbsc/src/libabis/input/ipaccess.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/openbsc/src/libabis/input/ipaccess.c b/openbsc/src/libabis/input/ipaccess.c index ba641a8..cba03a0 100644 --- a/openbsc/src/libabis/input/ipaccess.c +++ b/openbsc/src/libabis/input/ipaccess.c @@ -200,7 +200,7 @@ static int ipaccess_send(int fd, const void *msg, size_t msglen) if (ret < 0) return ret; if (ret < msglen) { - DEBUGP(DINP, "ipaccess_send: short write\n"); + LOGP(DINP, LOGL_ERROR, "ipaccess_send: short write\n"); return -EIO; } return ret;
From: Pablo Neira Ayuso pablo@gnumonks.org
Use generic ipaccess_idtag_name() available in libabis instead. --- openbsc/src/ipaccess/Makefile.am | 6 ++++++ openbsc/src/ipaccess/ipaccess-find.c | 22 +--------------------- 2 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/openbsc/src/ipaccess/Makefile.am b/openbsc/src/ipaccess/Makefile.am index cff3505..4fe1e37 100644 --- a/openbsc/src/ipaccess/Makefile.am +++ b/openbsc/src/ipaccess/Makefile.am @@ -4,6 +4,12 @@ AM_LDFLAGS = $(LIBOSMOCORE_LIBS) $(LIBOSMOGSM_LIBS) $(COVERAGE_LDFLAGS)
bin_PROGRAMS = ipaccess-find ipaccess-config ipaccess-proxy
+ipaccess_find_LDADD = $(top_builddir)/src/libbsc/libbsc.a \ + $(top_builddir)/src/libmsc/libmsc.a \ + $(top_builddir)/src/libabis/libabis.a \ + $(top_builddir)/src/libbsc/libbsc.a \ + $(top_builddir)/src/libtrau/libtrau.a \ + $(top_builddir)/src/libcommon/libcommon.a ipaccess_find_SOURCES = ipaccess-find.c
ipaccess_config_SOURCES = ipaccess-config.c ipaccess-firmware.c network_listen.c diff --git a/openbsc/src/ipaccess/ipaccess-find.c b/openbsc/src/ipaccess/ipaccess-find.c index a273609..987e35c 100644 --- a/openbsc/src/ipaccess/ipaccess-find.c +++ b/openbsc/src/ipaccess/ipaccess-find.c @@ -32,26 +32,6 @@ #include <openbsc/ipaccess.h> #include <openbsc/gsm_data.h>
-static const char *idtag_names[] = { - [IPAC_IDTAG_SERNR] = "Serial Number", - [IPAC_IDTAG_UNITNAME] = "Unit Name", - [IPAC_IDTAG_LOCATION1] = "Location 1", - [IPAC_IDTAG_LOCATION2] = "Location 2", - [IPAC_IDTAG_EQUIPVERS] = "Equipment Version", - [IPAC_IDTAG_SWVERSION] = "Software Version", - [IPAC_IDTAG_IPADDR] = "IP Address", - [IPAC_IDTAG_MACADDR] = "MAC Address", - [IPAC_IDTAG_UNIT] = "Unit ID", -}; - -static const char *ipac_idtag_name(int tag) -{ - if (tag >= ARRAY_SIZE(idtag_names)) - return "unknown"; - - return idtag_names[tag]; -} - static int udp_sock(const char *ifname) { int fd, rc, bc = 1; @@ -133,7 +113,7 @@ static int parse_response(unsigned char *buf, int len) t_len = *cur++; t_tag = *cur++; - printf("%s='%s' ", ipac_idtag_name(t_tag), cur); + printf("%s='%s' ", ipaccess_idtag_name(t_tag), cur);
cur += t_len; }
From: Pablo Neira Ayuso pablo@gnumonks.org
With this patch, ipaccess-proxy makes more robust option checking:
$ ./ipaccess-proxy -l 1.1.1.1 -b 2.2.2.2 -e ERROR: missing mandatory argument for `-e' option
And we return to shell to enforce the user to try again with the appropriate invocation.
Before this patch, the default getopt_long() error handling was enabled which displayed this message:
./ipaccess-proxy: option requires an argument -- 'e'
and ipaccess-proxy continued working.
This is generic enough to cover other option that require mandatory arguments like `--bsc' and `--listen'. --- openbsc/src/ipaccess/ipaccess-proxy.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/openbsc/src/ipaccess/ipaccess-proxy.c b/openbsc/src/ipaccess/ipaccess-proxy.c index 55658cf..dd0a739 100644 --- a/openbsc/src/ipaccess/ipaccess-proxy.c +++ b/openbsc/src/ipaccess/ipaccess-proxy.c @@ -1133,6 +1133,9 @@ static void handle_options(int argc, char** argv) { int options_mask = 0;
+ /* disable explicit missing arguments error output from getopt_long */ + opterr = 0; + while (1) { int option_index = 0, c; static struct option long_options[] = { @@ -1176,6 +1179,18 @@ static void handle_options(int argc, char** argv) case 'e': log_set_log_level(stderr_target, atoi(optarg)); break; + case '?': + if (optopt) { + printf("ERROR: missing mandatory argument " + "for `%s' option\n", argv[optind-1]); + } else { + printf("ERROR: unknown option `%s'\n", + argv[optind-1]); + } + print_usage(); + print_help(); + exit(EXIT_FAILURE); + break; default: /* ignore */ break;
From: Pablo Neira Ayuso pablo@gnumonks.org
It seems it remains unimplemented, remove it. --- openbsc/src/ipaccess/ipaccess-proxy.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/openbsc/src/ipaccess/ipaccess-proxy.c b/openbsc/src/ipaccess/ipaccess-proxy.c index dd0a739..4dd6ee5 100644 --- a/openbsc/src/ipaccess/ipaccess-proxy.c +++ b/openbsc/src/ipaccess/ipaccess-proxy.c @@ -1145,7 +1145,6 @@ static void handle_options(int argc, char** argv) {"log-level", 1, 0, 'e'}, {"listen", 1, 0, 'l'}, {"bsc", 1, 0, 'b'}, - {"udp", 1, 0, 'u'}, {0, 0, 0, 0} };
From: Pablo Neira Ayuso pablo@gnumonks.org
It is similar to make_sock() available in openbsc/libcommon. --- openbsc/src/ipaccess/ipaccess-proxy.c | 40 +++++--------------------------- 1 files changed, 7 insertions(+), 33 deletions(-)
diff --git a/openbsc/src/ipaccess/ipaccess-proxy.c b/openbsc/src/ipaccess/ipaccess-proxy.c index 4dd6ee5..5254959 100644 --- a/openbsc/src/ipaccess/ipaccess-proxy.c +++ b/openbsc/src/ipaccess/ipaccess-proxy.c @@ -126,7 +126,6 @@ static char *listen_ipaddr; static char *bsc_ipaddr; static char *gprs_ns_ipaddr;
-static int make_gprs_sock(struct bsc_fd *bfd, int (*cb)(struct bsc_fd*,unsigned int), void *); static int gprs_ns_cb(struct bsc_fd *bfd, unsigned int what);
#define PROXY_ALLOC_SIZE 1200 @@ -389,7 +388,13 @@ static int ipbc_alloc_connect(struct ipa_proxy_conn *ipc, struct bsc_fd *bfd, if (gprs_ns_ipaddr) { struct sockaddr_in sock; socklen_t len = sizeof(sock); - ret = make_gprs_sock(&ipbc->gprs_ns_fd, gprs_ns_cb, ipbc); + struct in_addr addr; + uint32_t ip; + + inet_aton(listen_ipaddr, &addr); + ip = ntohl(addr.s_addr); /* make_sock() needs host byte order */ + ret = make_sock(&ipbc->gprs_ns_fd, IPPROTO_UDP, ip, 0, 0, + gprs_ns_cb, ipbc); if (ret < 0) { LOGP(DINP, LOGL_ERROR, "Creating the GPRS socket failed.\n"); goto err_udp_bsc; @@ -982,37 +987,6 @@ static int gprs_ns_cb(struct bsc_fd *bfd, unsigned int what) return 0; }
-static int make_gprs_sock(struct bsc_fd *bfd, int (*cb)(struct bsc_fd*,unsigned int), void *data) -{ - struct sockaddr_in addr; - int ret; - - bfd->fd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); - bfd->cb = cb; - bfd->data = data; - bfd->when = BSC_FD_READ; - - memset(&addr, 0, sizeof(addr)); - addr.sin_family = AF_INET; - addr.sin_port = 0; - inet_aton(listen_ipaddr, &addr.sin_addr); - - ret = bind(bfd->fd, (struct sockaddr *) &addr, sizeof(addr)); - if (ret < 0) { - LOGP(DINP, LOGL_ERROR, - "Could not bind n socket for IP %s with error: %s.\n", - listen_ipaddr, strerror(errno)); - return -EIO; - } - - ret = bsc_register_fd(bfd); - if (ret < 0) { - perror("register_listen_fd"); - return ret; - } - return 0; -} - /* Actively connect to a BSC. */ static struct ipa_proxy_conn *connect_bsc(struct sockaddr_in *sa, int priv_nr, void *data) {
Hi Pablo,
On Mon, Apr 11, 2011 at 05:04:23PM +0200, pablo@gnumonks.org wrote:
They are available in the pablo/cleanups branch in openbsc git tree.
BTW: I'm going to stop a bit with cleanups to add a VTY interface for ipaccess-proxy, as Harald suggested.
Please, merge it!
thanks, merged!
On 11/04/11 19:14, Harald Welte wrote:
Hi Pablo,
On Mon, Apr 11, 2011 at 05:04:23PM +0200, pablo@gnumonks.org wrote:
They are available in the pablo/cleanups branch in openbsc git tree.
BTW: I'm going to stop a bit with cleanups to add a VTY interface for ipaccess-proxy, as Harald suggested.
Please, merge it!
thanks, merged!
Hm, I don't see them in the master branch, still in your local tree?
On Tue, Apr 12, 2011 at 07:56:29PM +0200, Pablo Neira Ayuso wrote:
On 11/04/11 19:14, Harald Welte wrote:
Hi Pablo,
On Mon, Apr 11, 2011 at 05:04:23PM +0200, pablo@gnumonks.org wrote:
They are available in the pablo/cleanups branch in openbsc git tree.
BTW: I'm going to stop a bit with cleanups to add a VTY interface for ipaccess-proxy, as Harald suggested.
Please, merge it!
thanks, merged!
Hm, I don't see them in the master branch, still in your local tree?
yes, sorry. fixed now.
-
Harald Welte -
Holger Hans Peter Freyther -
Pablo Neira Ayuso -
pablo＠gnumonks.org