[PATCH 1/6] msgb: Add msgb_resize_area and msgb_copy
These functions originate from openbsc/src/gprs but are generic msgb helper functions. msgb_copy: This function allocates a new msgb, copies the data buffer of msg, and adjusts the pointers (incl. l1h-l4h) accordingly. msgb_resize_area: This resizes a sub area of the msgb data and adjusts the pointers (incl. l1h-l4h) accordingly. Sponsored-by: On-Waves ehf --- include/osmocom/core/msgb.h | 3 ++ src/msgb.c | 88 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/include/osmocom/core/msgb.h b/include/osmocom/core/msgb.h index 644a639..21e363a 100644 --- a/include/osmocom/core/msgb.h +++ b/include/osmocom/core/msgb.h @@ -74,6 +74,9 @@ extern struct msgb *msgb_dequeue(struct llist_head *queue); extern void msgb_reset(struct msgb *m); uint16_t msgb_length(const struct msgb *msg); extern const char *msgb_hexdump(const struct msgb *msg); +extern int msgb_resize_area(struct msgb *msg, uint8_t *area, + size_t old_size, size_t new_size); +extern struct msgb *msgb_copy(const struct msgb *msg, const char *name); #ifdef MSGB_DEBUG #include <osmocom/core/panic.h> diff --git a/src/msgb.c b/src/msgb.c index b2fe1d2..a257479 100644 --- a/src/msgb.c +++ b/src/msgb.c @@ -153,6 +153,94 @@ void msgb_set_talloc_ctx(void *ctx) tall_msgb_ctx = ctx; } +/*! \brief Copy an msgb. + * + * This function allocates a new msgb, copies the data buffer of msg, + * and adjusts the pointers (incl l1h-l4h) accordingly. The cb part + * is not copied. + * \param[in] msg The old msgb object + * \param[in] name Human-readable name to be associated with msgb + */ +struct msgb *msgb_copy(const struct msgb *msg, const char *name) +{ + struct msgb *new_msg; + + new_msg = msgb_alloc(msg->data_len, name); + if (!new_msg) + return NULL; + + /* copy data */ + memcpy(new_msg->_data, msg->_data, new_msg->data_len); + + /* copy header */ + new_msg->len = msg->len; + new_msg->data += msg->data - msg->_data; + new_msg->head += msg->head - msg->_data; + new_msg->tail += msg->tail - msg->_data; + + if (msg->l1h) + new_msg->l1h = new_msg->_data + (msg->l1h - msg->_data); + if (msg->l2h) + new_msg->l2h = new_msg->_data + (msg->l2h - msg->_data); + if (msg->l3h) + new_msg->l3h = new_msg->_data + (msg->l3h - msg->_data); + if (msg->l4h) + new_msg->l4h = new_msg->_data + (msg->l4h - msg->_data); + + return new_msg; +} + +/*! \brief Resize an area within an msgb + * + * This resizes a sub area of the msgb data and adjusts the pointers (incl + * l1h-l4h) accordingly. The cb part is not updated. If the area is extended, + * the contents of the extension is undefined. The complete sub area must be a + * part of [data,tail]. + * + * \param[inout] msg The msgb object + * \param[in] area A pointer to the sub-area + * \param[in] old_size The old size of the sub-area + * \param[in] new_size The new size of the sub-area + * \returns 0 on success, -1 if there is not enough space to extend the area + */ +int msgb_resize_area(struct msgb *msg, uint8_t *area, + size_t old_size, size_t new_size) +{ + int rc; + uint8_t *rest = area + old_size; + int rest_len = msg->len - old_size - (area - msg->data); + int delta_size = (int)new_size - (int)old_size; + + if (area < msg->data || rest > msg->tail) + MSGB_ABORT(msg, "Sub area is not fully contained in the msg data\n"); + + if (delta_size == 0) + return 0; + + if (delta_size > 0) { + rc = msgb_trim(msg, msg->len + delta_size); + if (rc < 0) + return rc; + } + + memmove(area + new_size, area + old_size, rest_len); + + if (msg->l1h >= rest) + msg->l1h += delta_size; + if (msg->l2h >= rest) + msg->l2h += delta_size; + if (msg->l3h >= rest) + msg->l3h += delta_size; + if (msg->l4h >= rest) + msg->l4h += delta_size; + + if (delta_size < 0) + msgb_trim(msg, msg->len + delta_size); + + return 0; +} + + /*! \brief Return a (static) buffer containing a hexdump of the msg * \param[in] msg message buffer * \returns a pointer to a static char array -- 1.9.1
This patch makes msgb_hexdump accept out of range lXh pointers and shows info about them instead of aborting the dump entirely. Sponsored-by: On-Waves ehf --- src/msgb.c | 46 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/src/msgb.c b/src/msgb.c index a257479..d896b53 100644 --- a/src/msgb.c +++ b/src/msgb.c @@ -263,10 +263,25 @@ const char *msgb_hexdump(const struct msgb *msg) if (!lxhs[i]) continue; - if (lxhs[i] < msg->data) - goto out_of_range; + if (lxhs[i] < msg->head) + continue; + if (lxhs[i] > msg->head + msg->data_len) + continue; if (lxhs[i] > msg->tail) - goto out_of_range; + continue; + if (lxhs[i] < msg->data || lxhs[i] > msg->tail) { + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "(L%d=data%+d) ", + i+1, lxhs[i] - msg->data); + buf_offs += nchars; + continue; + } + if (lxhs[i] < start) { + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "(L%d%+d) ", i+1, start - lxhs[i]); + buf_offs += nchars; + continue; + } nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, "%s[L%d]> ", osmo_hexdump(start, lxhs[i] - start), @@ -282,11 +297,28 @@ const char *msgb_hexdump(const struct msgb *msg) if (nchars < 0 || nchars + buf_offs >= sizeof(buf)) return "ERROR"; - return buf; + buf_offs += nchars; + + for (i = 0; i < ARRAY_SIZE(lxhs); i++) { + if (!lxhs[i]) + continue; + + if (lxhs[i] < msg->head || lxhs[i] > msg->head + msg->data_len) { + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "(L%d out of range) ", i+1); + } else if (lxhs[i] <= msg->data + msg->data_len && + lxhs[i] > msg->tail) { + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "(L%d=tail%+d) ", + i+1, lxhs[i] - msg->tail); + } else + continue; + + if (nchars < 0 || nchars + buf_offs >= sizeof(buf)) + return "ERROR"; + buf_offs += nchars; + } -out_of_range: - nchars = snprintf(buf, sizeof(buf) - buf_offs, - "!!! L%d out of range", i+1); return buf; } -- 1.9.1
This adds a function that verifies whether a mgsb is consistent. Sponsored-by: On-Waves ehf --- include/osmocom/core/msgb.h | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/include/osmocom/core/msgb.h b/include/osmocom/core/msgb.h index 21e363a..2bf4ac5 100644 --- a/include/osmocom/core/msgb.h +++ b/include/osmocom/core/msgb.h @@ -77,6 +77,7 @@ extern const char *msgb_hexdump(const struct msgb *msg); extern int msgb_resize_area(struct msgb *msg, uint8_t *area, size_t old_size, size_t new_size); extern struct msgb *msgb_copy(const struct msgb *msg, const char *name); +static int msgb_test_invariant(const struct msgb *msg) __attribute__((pure)); #ifdef MSGB_DEBUG #include <osmocom/core/panic.h> @@ -412,6 +413,45 @@ static inline struct msgb *msgb_alloc_headroom(int size, int headroom, return msg; } +/*! \brief Check a message buffer for consistency + * \param[in] msg message buffer + * \returns 0 (false) if inconsistent, != 0 (true) otherwise + */ +static inline int msgb_test_invariant(const struct msgb *msg) +{ + const unsigned char *lbound; + if (!msg || !msg->data || !msg->tail || + (msg->data + msg->len != msg->tail) || + (msg->data < msg->head) || + (msg->tail > msg->head + msg->data_len)) + return 0; + + lbound = msg->head; + + if (msg->l1h) { + if (msg->l1h < lbound) + return 0; + lbound = msg->l1h; + } + if (msg->l2h) { + if (msg->l2h < lbound) + return 0; + lbound = msg->l2h; + } + if (msg->l3h) { + if (msg->l3h < lbound) + return 0; + lbound = msg->l3h; + } + if (msg->l4h) { + if (msg->l4h < lbound) + return 0; + lbound = msg->l4h; + } + + return lbound <= msg->head + msg->data_len; +} + /* non inline functions to ease binding */ uint8_t *msgb_data(const struct msgb *msg); -- 1.9.1
This tests several API functions of the msgb by checking the invariant and by dumping resulting message buffers as hex. Sponsored-by: On-Waves ehf Conflicts: tests/Makefile.am --- tests/Makefile.am | 9 +++- tests/msgb/msgb_test.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/msgb/msgb_test.ok | 21 +++++++++ tests/testsuite.at | 6 +++ 4 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 tests/msgb/msgb_test.c create mode 100644 tests/msgb/msgb_test.ok diff --git a/tests/Makefile.am b/tests/Makefile.am index 4442355..edf25a0 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -10,7 +10,8 @@ check_PROGRAMS = timer/timer_test sms/sms_test ussd/ussd_test \ logging/logging_test fr/fr_test \ loggingrb/loggingrb_test strrb/strrb_test \ vty/vty_test comp128/comp128_test utils/utils_test \ - smscb/gsm0341_test stats/stats_test + smscb/gsm0341_test stats/stats_test \ + msgb/msgb_test if ENABLE_MSGFILE check_PROGRAMS += msgfile/msgfile_test @@ -52,6 +53,9 @@ gprs_gprs_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gs lapd_lapd_test_SOURCES = lapd/lapd_test.c lapd_lapd_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gsm/libosmogsm.la +msgb_msgb_test_SOURCES = msgb/msgb_test.c +msgb_msgb_test_LDADD = $(top_builddir)/src/libosmocore.la + msgfile_msgfile_test_SOURCES = msgfile/msgfile_test.c msgfile_msgfile_test_LDADD = $(top_builddir)/src/libosmocore.la @@ -127,7 +131,8 @@ EXTRA_DIST = testsuite.at $(srcdir)/package.m4 $(TESTSUITE) \ fr/fr_test.ok loggingrb/logging_test.ok \ loggingrb/logging_test.err strrb/strrb_test.ok \ vty/vty_test.ok comp128/comp128_test.ok \ - utils/utils_test.ok stats/stats_test.ok + utils/utils_test.ok stats/stats_test.ok \ + msgb/msgb_test.ok DISTCLEANFILES = atconfig diff --git a/tests/msgb/msgb_test.c b/tests/msgb/msgb_test.c new file mode 100644 index 0000000..412e8bb --- /dev/null +++ b/tests/msgb/msgb_test.c @@ -0,0 +1,120 @@ +/* + * (C) 2014 by On-Waves + * All Rights Reserved + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 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 General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + */ + +#include <stdlib.h> +#include <osmocom/core/application.h> +#include <osmocom/core/logging.h> +#include <osmocom/core/utils.h> +#include <osmocom/core/msgb.h> + +#include <errno.h> + +#include <string.h> + +#define CHECK_RC(rc) \ + if (rc != 0) { \ + printf("Operation failed rc=%d on %s:%d\n", rc, __FILE__, __LINE__); \ + abort(); \ + } + +static void test_msgb_api() +{ + struct msgb *msg = msgb_alloc_headroom(4096, 128, "data"); + unsigned char *cptr = NULL; + int rc; + + printf("Testing the msgb API\n"); + + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + cptr = msg->l1h = msgb_put(msg, 4); + printf("put(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + cptr = msg->l2h = msgb_put(msg, 4); + printf("put(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + cptr = msg->l3h = msgb_put(msg, 4); + printf("put(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + cptr = msg->l4h = msgb_put(msg, 4); + printf("put(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + OSMO_ASSERT(msgb_length(msg) == 16); + cptr = msgb_push(msg, 4); + printf("push(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + OSMO_ASSERT(msgb_length(msg) == 20); + rc = msgb_trim(msg, 16); + printf("trim(16) -> %d\n", rc); + CHECK_RC(rc); + OSMO_ASSERT(msgb_test_invariant(msg)); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_length(msg) == 16); + + cptr = msgb_get(msg, 4); + printf("get(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + OSMO_ASSERT(msgb_length(msg) == 12); + + printf("Test msgb_hexdump\n"); + msg->l1h = msg->head; + printf("Buffer: %s\n", msgb_hexdump(msg)); + msg->l3h = msg->data; + printf("Buffer: %s\n", msgb_hexdump(msg)); + msg->l3h = msg->head - 1; + printf("Buffer: %s\n", msgb_hexdump(msg)); + +#if 0 +extern void msgb_reset(struct msgb *m); +#define msgb_l1(m) ((void *)(MSGB_CHECK2(m)->l1h)) +static inline unsigned int msgb_l1len(const struct msgb *msgb) + static inline int msgb_tailroom(const struct msgb *msgb) + static inline int msgb_headroom(const struct msgb *msgb) + static inline unsigned char *msgb_put(struct msgb *msgb, unsigned int len) + static inline unsigned char *msgb_get(struct msgb *msgb, unsigned int len) + static inline unsigned char *msgb_push(struct msgb *msgb, unsigned int len) + static inline unsigned char *msgb_pull(struct msgb *msgb, unsigned int len) + + static inline unsigned char *msgb_pull_to_l3(struct msgb *msg) + static inline int msgb_trim(struct msgb *msg, int len) + static inline int msgb_l3trim(struct msgb *msg, int l3len) + uint8_t *msgb_data(const struct msgb *msg); + return; +#endif +} + +static struct log_info info = {}; + +int main(int argc, char **argv) +{ + osmo_init_logging(&info); + + test_msgb_api(); + + printf("Success.\n"); + + return 0; +} diff --git a/tests/msgb/msgb_test.ok b/tests/msgb/msgb_test.ok new file mode 100644 index 0000000..f8de0cd --- /dev/null +++ b/tests/msgb/msgb_test.ok @@ -0,0 +1,21 @@ +Testing the msgb API +Buffer: +put(4) -> data+0 +Buffer: [L1]> 00 00 00 00 +put(4) -> data+4 +Buffer: [L1]> 00 00 00 00 [L2]> 00 00 00 00 +put(4) -> data+8 +Buffer: [L1]> 00 00 00 00 [L2]> 00 00 00 00 [L3]> 00 00 00 00 +put(4) -> data+12 +Buffer: [L1]> 00 00 00 00 [L2]> 00 00 00 00 [L3]> 00 00 00 00 [L4]> 00 00 00 00 +push(4) -> data+0 +Buffer: 00 00 00 00 [L1]> 00 00 00 00 [L2]> 00 00 00 00 [L3]> 00 00 00 00 [L4]> 00 00 00 00 +trim(16) -> 0 +Buffer: 00 00 00 00 [L1]> 00 00 00 00 [L2]> 00 00 00 00 [L3]> 00 00 00 00 [L4]> +get(4) -> data+12 +Buffer: 00 00 00 00 [L1]> 00 00 00 00 [L2]> 00 00 00 00 [L3]> (L4=tail+4) +Test msgb_hexdump +Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> 00 00 00 00 [L3]> (L4=tail+4) +Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> (L3+8) 00 00 00 00 (L4=tail+4) +Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> 00 00 00 00 (L3 out of range) (L4=tail+4) +Success. diff --git a/tests/testsuite.at b/tests/testsuite.at index 85c3e8b..12199e3 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -27,6 +27,12 @@ cat $abs_srcdir/conv/conv_test.ok > expout AT_CHECK([$abs_top_builddir/tests/conv/conv_test], [0], [expout]) AT_CLEANUP +AT_SETUP([msgb]) +AT_KEYWORDS([msgb]) +cat $abs_srcdir/msgb/msgb_test.ok > expout +AT_CHECK([$abs_top_builddir/tests/msgb/msgb_test], [0], [expout]) +AT_CLEANUP + if ENABLE_MSGFILE AT_SETUP([msgfile]) AT_KEYWORDS([msgfile]) -- 1.9.1
Sponsored-by: On-Waves ehf --- tests/msgb/msgb_test.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/msgb/msgb_test.ok | 2 ++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/tests/msgb/msgb_test.c b/tests/msgb/msgb_test.c index 412e8bb..08d9857 100644 --- a/tests/msgb/msgb_test.c +++ b/tests/msgb/msgb_test.c @@ -87,6 +87,7 @@ static void test_msgb_api() msg->l3h = msg->head - 1; printf("Buffer: %s\n", msgb_hexdump(msg)); + #if 0 extern void msgb_reset(struct msgb *m); #define msgb_l1(m) ((void *)(MSGB_CHECK2(m)->l1h)) @@ -97,15 +98,61 @@ static inline unsigned int msgb_l1len(const struct msgb *msgb) static inline unsigned char *msgb_get(struct msgb *msgb, unsigned int len) static inline unsigned char *msgb_push(struct msgb *msgb, unsigned int len) static inline unsigned char *msgb_pull(struct msgb *msgb, unsigned int len) - + static inline unsigned char *msgb_pull_to_l3(struct msgb *msg) static inline int msgb_trim(struct msgb *msg, int len) static inline int msgb_l3trim(struct msgb *msg, int l3len) uint8_t *msgb_data(const struct msgb *msg); - return; +return; #endif } +static void test_msgb_copy() +{ + struct msgb *msg = msgb_alloc_headroom(4096, 128, "data"); + struct msgb *msg2; + + printf("Testing msgb_copy\n"); + + msg->l1h = msgb_put(msg, 20); + msg->l2h = msgb_put(msg, 20); + msg->l3h = msgb_put(msg, 20); + msg->l4h = msgb_put(msg, 20); + + msg2 = msgb_copy(msg, "copy"); + + OSMO_ASSERT(msgb_length(msg) == msgb_length(msg2)); + OSMO_ASSERT(msgb_l1len(msg) == msgb_l1len(msg2)); + OSMO_ASSERT(msgb_l2len(msg) == msgb_l2len(msg2)); + OSMO_ASSERT(msgb_l3len(msg) == msgb_l3len(msg2)); + OSMO_ASSERT(msg->tail - msg->l4h == msg2->tail - msg2->l4h); +} + +static void test_msgb_resize_area() +{ + struct msgb *msg = msgb_alloc_headroom(4096, 128, "data"); + int rc; + + printf("Testing msgb_resize_area\n"); + + msg->l1h = msgb_put(msg, 20); + msg->l2h = msgb_put(msg, 20); + msg->l3h = msgb_put(msg, 20); + msg->l4h = msgb_put(msg, 20); + + rc = msgb_resize_area(msg, msg->l2h, 20, 20 + 30); + + OSMO_ASSERT(rc >= 0); + OSMO_ASSERT(msgb_length(msg) == 80 + 30); + OSMO_ASSERT(msgb_l1len(msg) == 80 + 30); + OSMO_ASSERT(msgb_l2len(msg) == 60 + 30); + OSMO_ASSERT(msgb_l3len(msg) == 40); + OSMO_ASSERT(msg->tail - msg->l4h == 20); + + rc = msgb_resize_area(msg, msg->l2h, 50, 8000); + OSMO_ASSERT(rc == -1); +} + static struct log_info info = {}; int main(int argc, char **argv) @@ -113,6 +160,8 @@ int main(int argc, char **argv) osmo_init_logging(&info); test_msgb_api(); + test_msgb_copy(); + test_msgb_resize_area(); printf("Success.\n"); diff --git a/tests/msgb/msgb_test.ok b/tests/msgb/msgb_test.ok index f8de0cd..3ace5be 100644 --- a/tests/msgb/msgb_test.ok +++ b/tests/msgb/msgb_test.ok @@ -18,4 +18,6 @@ Test msgb_hexdump Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> 00 00 00 00 [L3]> (L4=tail+4) Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> (L3+8) 00 00 00 00 (L4=tail+4) Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> 00 00 00 00 (L3 out of range) (L4=tail+4) +Testing msgb_copy +Testing msgb_resize_area Success. -- 1.9.1
On Tue, Nov 17, 2015 at 10:37:48AM +0100, Jacob Erlbeck wrote:
diff --git a/tests/msgb/msgb_test.c b/tests/msgb/msgb_test.c index 412e8bb..08d9857 100644 --- a/tests/msgb/msgb_test.c +++ b/tests/msgb/msgb_test.c @@ -87,6 +87,7 @@ static void test_msgb_api() msg->l3h = msg->head - 1; printf("Buffer: %s\n", msgb_hexdump(msg));
+ #if 0 extern void msgb_reset(struct msgb *m); #define msgb_l1(m) ((void *)(MSGB_CHECK2(m)->l1h)) @@ -97,15 +98,61 @@ static inline unsigned int msgb_l1len(const struct msgb *msgb) static inline unsigned char *msgb_get(struct msgb *msgb, unsigned int len) static inline unsigned char *msgb_push(struct msgb *msgb, unsigned int len) static inline unsigned char *msgb_pull(struct msgb *msgb, unsigned int len) - + static inline unsigned char *msgb_pull_to_l3(struct msgb *msg) static inline int msgb_trim(struct msgb *msg, int len) static inline int msgb_l3trim(struct msgb *msg, int l3len) uint8_t *msgb_data(const struct msgb *msg); - return; +return; #endif }
^ seems like those whitespace changes came in by accident... ~Neels
On 23.11.2015 10:43, Neels Hofmeyr wrote:
On Tue, Nov 17, 2015 at 10:37:48AM +0100, Jacob Erlbeck wrote:
diff --git a/tests/msgb/msgb_test.c b/tests/msgb/msgb_test.c index 412e8bb..08d9857 100644 --- a/tests/msgb/msgb_test.c +++ b/tests/msgb/msgb_test.c @@ -87,6 +87,7 @@ static void test_msgb_api() msg->l3h = msg->head - 1; printf("Buffer: %s\n", msgb_hexdump(msg));
+ #if 0 extern void msgb_reset(struct msgb *m); #define msgb_l1(m) ((void *)(MSGB_CHECK2(m)->l1h)) @@ -97,15 +98,61 @@ static inline unsigned int msgb_l1len(const struct msgb *msgb) static inline unsigned char *msgb_get(struct msgb *msgb, unsigned int len) static inline unsigned char *msgb_push(struct msgb *msgb, unsigned int len) static inline unsigned char *msgb_pull(struct msgb *msgb, unsigned int len) - + static inline unsigned char *msgb_pull_to_l3(struct msgb *msg) static inline int msgb_trim(struct msgb *msg, int len) static inline int msgb_l3trim(struct msgb *msg, int l3len) uint8_t *msgb_data(const struct msgb *msg); - return; +return; #endif }
^ seems like those whitespace changes came in by accident...
Yes, thanks for noticing. I will remove the complete commentary block. Jacob -- - Jacob Erlbeck <jerlbeck@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
This function originates from openbsc/src/gprs but is just specific to BSSGP/Gb on the same level like bssgp_msgb_alloc. This commit puts the former gprs_msgb_copy function beside bssgp_msgb_alloc. Renamed function: gprs_msgb_copy -> bssgp_msgb_copy Sponsored-by: On-Waves ehf --- include/osmocom/gprs/gprs_bssgp.h | 1 + src/gb/gprs_bssgp_util.c | 30 ++++++++++++++++++++++++++++++ src/gb/libosmogb.map | 1 + tests/gb/gprs_bssgp_test.c | 34 ++++++++++++++++++++++++++++++++++ tests/gb/gprs_bssgp_test.ok | 4 ++++ 5 files changed, 70 insertions(+) diff --git a/include/osmocom/gprs/gprs_bssgp.h b/include/osmocom/gprs/gprs_bssgp.h index e24b563..c0b3f65 100644 --- a/include/osmocom/gprs/gprs_bssgp.h +++ b/include/osmocom/gprs/gprs_bssgp.h @@ -12,6 +12,7 @@ /* gprs_bssgp_util.c */ extern struct gprs_ns_inst *bssgp_nsi; struct msgb *bssgp_msgb_alloc(void); +struct msgb *bssgp_msgb_copy(const struct msgb *msg, const char *name); const char *bssgp_cause_str(enum gprs_bssgp_cause cause); /* Transmit a simple response such as BLOCK/UNBLOCK/RESET ACK/NACK */ int bssgp_tx_simple_bvci(uint8_t pdu_type, uint16_t nsei, diff --git a/src/gb/gprs_bssgp_util.c b/src/gb/gprs_bssgp_util.c index 3c42e4d..19ae23a 100644 --- a/src/gb/gprs_bssgp_util.c +++ b/src/gb/gprs_bssgp_util.c @@ -79,6 +79,36 @@ struct msgb *bssgp_msgb_alloc(void) return msg; } +struct msgb *bssgp_msgb_copy(const struct msgb *msg, const char *name) +{ + struct libgb_msgb_cb *old_cb, *new_cb; + struct msgb *new_msg; + + new_msg = msgb_copy(msg, name); + if (!new_msg) + return NULL; + + /* copy GB specific data */ + old_cb = LIBGB_MSGB_CB(msg); + new_cb = LIBGB_MSGB_CB(new_msg); + + if (old_cb->bssgph) + new_cb->bssgph = new_msg->_data + (old_cb->bssgph - msg->_data); + if (old_cb->llch) + new_cb->llch = new_msg->_data + (old_cb->llch - msg->_data); + + /* bssgp_cell_id is a pointer into the old msgb, so we need to make + * it a pointer into the new msgb */ + if (old_cb->bssgp_cell_id) + new_cb->bssgp_cell_id = new_msg->_data + + (old_cb->bssgp_cell_id - msg->_data); + new_cb->nsei = old_cb->nsei; + new_cb->bvci = old_cb->bvci; + new_cb->tlli = old_cb->tlli; + + return new_msg; +} + /* Transmit a simple response such as BLOCK/UNBLOCK/RESET ACK/NACK */ int bssgp_tx_simple_bvci(uint8_t pdu_type, uint16_t nsei, uint16_t bvci, uint16_t ns_bvci) diff --git a/src/gb/libosmogb.map b/src/gb/libosmogb.map index 43ebbf8..75406c0 100644 --- a/src/gb/libosmogb.map +++ b/src/gb/libosmogb.map @@ -6,6 +6,7 @@ bssgp_fc_in; bssgp_fc_init; bssgp_fc_ms_init; bssgp_msgb_alloc; +bssgp_msgb_copy; bssgp_msgb_tlli_put; bssgp_parse_cell_id; bssgp_tx_bvc_block; diff --git a/tests/gb/gprs_bssgp_test.c b/tests/gb/gprs_bssgp_test.c index 14ba4d1..bf35546 100644 --- a/tests/gb/gprs_bssgp_test.c +++ b/tests/gb/gprs_bssgp_test.c @@ -254,6 +254,39 @@ static void test_bssgp_flow_control_bvc(void) printf("----- %s END\n", __func__); } +static void test_bssgp_msgb_copy() +{ + struct msgb *msg, *msg2; + uint16_t bvci_be = htons(2); + uint8_t cause = BSSGP_CAUSE_OML_INTERV; + + printf("----- %s START\n", __func__); + msg = bssgp_msgb_alloc(); + + msg->l3h = msgb_data(msg); + msgb_v_put(msg, BSSGP_PDUT_BVC_RESET); + msgb_tvlv_put(msg, BSSGP_IE_BVCI, sizeof(bvci_be), (uint8_t *)&bvci_be); + msgb_tvlv_put(msg, BSSGP_IE_CAUSE, sizeof(cause), &cause); + + msgb_bvci(msg) = 0xbad; + msgb_nsei(msg) = 0xbee; + + printf("Old msgb: %s\n", msgb_hexdump(msg)); + msg2 = bssgp_msgb_copy(msg, "test"); + printf("New msgb: %s\n", msgb_hexdump(msg2)); + + OSMO_ASSERT(msgb_bvci(msg2) == 0xbad); + OSMO_ASSERT(msgb_nsei(msg2) == 0xbee); + OSMO_ASSERT(msgb_l3(msg2) == msgb_data(msg2)); + OSMO_ASSERT(msgb_bssgph(msg2) == msgb_data(msg2)); + OSMO_ASSERT(msgb_bssgp_len(msg2) == msgb_length(msg2)); + + msgb_free(msg); + msgb_free(msg2); + + printf("----- %s END\n", __func__); +} + static struct log_info info = {}; int main(int argc, char **argv) @@ -278,6 +311,7 @@ int main(int argc, char **argv) test_bssgp_status(); test_bssgp_bad_reset(); test_bssgp_flow_control_bvc(); + test_bssgp_msgb_copy(); printf("===== BSSGP test END\n\n"); exit(EXIT_SUCCESS); diff --git a/tests/gb/gprs_bssgp_test.ok b/tests/gb/gprs_bssgp_test.ok index 83d633b..c5b3e7d 100644 --- a/tests/gb/gprs_bssgp_test.ok +++ b/tests/gb/gprs_bssgp_test.ok @@ -13,5 +13,9 @@ BSSGP primitive, SAP 16777221, prim = 11, op = 2, msg = 41 07 81 05 04 82 04 d2 Got message: 26 1e 81 2a 05 82 10 22 03 82 c0 40 01 82 08 11 1c 82 60 20 Got message: 26 1e 81 2a 05 82 10 22 03 82 c0 40 01 82 08 11 1c 82 60 20 3c 81 78 06 82 11 44 ----- test_bssgp_flow_control_bvc END +----- test_bssgp_msgb_copy START +Old msgb: [L3]> 22 04 82 00 02 07 81 08 +New msgb: [L3]> 22 04 82 00 02 07 81 08 +----- test_bssgp_msgb_copy END ===== BSSGP test END -- 1.9.1
Hi, this patch set expects that the following patches have been applied to the libosmocore project: - msgb: Add msgb_resize_area and msgb_copy - gb: Add bssgp_msgb_copy function - gsm: Add APN conversion functions Jacob [PATCH 1/3] gprs: Remove gprs_apn_to_str and gprs_str_to_apn [PATCH 2/3] gprs: Remove gprs_msgb_resize_area and gprs_msgb_copy [PATCH 3/3] sgsn: Consolidate gprs_apn2str with osmo_apn_to_str ====== openbsc/include/openbsc/gprs_utils.h | 10 +-- openbsc/src/gprs/gb_proxy.c | 8 +- openbsc/src/gprs/gb_proxy_patch.c | 11 +-- openbsc/src/gprs/gb_proxy_vty.c | 6 +- openbsc/src/gprs/gprs_gmm.c | 2 +- openbsc/src/gprs/gprs_sgsn.c | 4 +- openbsc/src/gprs/gprs_subscriber.c | 5 +- openbsc/src/gprs/gprs_utils.c | 146 ----------------------------------- openbsc/src/gprs/gtphub.c | 4 +- openbsc/src/gprs/sgsn_cdr.c | 5 +- openbsc/src/gprs/sgsn_vty.c | 22 +----- openbsc/tests/gbproxy/gbproxy_test.c | 15 ++-- openbsc/tests/gprs/gprs_test.c | 29 +++---- openbsc/tests/gtphub/Makefile.am | 1 + openbsc/tests/sgsn/sgsn_test.c | 13 ++-- 15 files changed, 63 insertions(+), 218 deletions(-)
These functions are moved to libosmocore. This commit removes the definitions and changes the names to the corresponding libosmogsm ones: gprs_str_to_apn -> osmo_apn_from_str gprs_apn_to_str -> osmo_apn_to_str Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/gprs_utils.h | 4 +-- openbsc/src/gprs/gb_proxy_patch.c | 7 ++-- openbsc/src/gprs/gb_proxy_vty.c | 6 ++-- openbsc/src/gprs/gprs_sgsn.c | 4 +-- openbsc/src/gprs/gprs_subscriber.c | 5 +-- openbsc/src/gprs/gprs_utils.c | 62 ------------------------------------ openbsc/src/gprs/gtphub.c | 4 +-- openbsc/src/gprs/sgsn_cdr.c | 5 +-- openbsc/src/gprs/sgsn_vty.c | 2 +- openbsc/tests/gbproxy/gbproxy_test.c | 13 ++++---- openbsc/tests/gprs/gprs_test.c | 29 +++++++++-------- openbsc/tests/gtphub/Makefile.am | 1 + openbsc/tests/sgsn/sgsn_test.c | 13 ++++---- 13 files changed, 49 insertions(+), 106 deletions(-) diff --git a/openbsc/include/openbsc/gprs_utils.h b/openbsc/include/openbsc/gprs_utils.h index 6880e05..7af83ba 100644 --- a/openbsc/include/openbsc/gprs_utils.h +++ b/openbsc/include/openbsc/gprs_utils.h @@ -23,15 +23,13 @@ #include <stdint.h> #include <sys/types.h> +#include <osmocom/core/defs.h> struct msgb; struct msgb *gprs_msgb_copy(const struct msgb *msg, const char *name); int gprs_msgb_resize_area(struct msgb *msg, uint8_t *area, size_t old_size, size_t new_size); -char *gprs_apn_to_str(char *out_str, const uint8_t *apn_enc, size_t rest_chars); -int gprs_str_to_apn(uint8_t *apn_enc, size_t max_len, const char *str); - /* GSM 04.08, 10.5.7.3 GPRS Timer */ int gprs_tmr_to_secs(uint8_t tmr); uint8_t gprs_secs_to_tmr_floor(int secs); diff --git a/openbsc/src/gprs/gb_proxy_patch.c b/openbsc/src/gprs/gb_proxy_patch.c index c1d2497..725a863 100644 --- a/openbsc/src/gprs/gb_proxy_patch.c +++ b/openbsc/src/gprs/gb_proxy_patch.c @@ -28,6 +28,7 @@ #include <openbsc/debug.h> #include <osmocom/gprs/protocol/gsm_08_18.h> +#include <osmocom/gsm/apn.h> #include <osmocom/core/rate_ctr.h> /* patch RA identifier in place */ @@ -102,7 +103,7 @@ static void gbproxy_patch_apn_ie(struct msgb *msg, LOGP(DGPRS, LOGL_DEBUG, "Patching %s to SGSN: Removing APN '%s'\n", log_text, - gprs_apn_to_str(str1, apn, apn_len)); + osmo_apn_to_str(str1, apn, apn_len)); *new_apn_ie_len = 0; gprs_msgb_resize_area(msg, apn_ie, apn_ie_len, 0); @@ -117,8 +118,8 @@ static void gbproxy_patch_apn_ie(struct msgb *msg, "Patching %s to SGSN: " "Replacing APN '%s' -> '%s'\n", log_text, - gprs_apn_to_str(str1, apn, apn_len), - gprs_apn_to_str(str2, peer->cfg->core_apn, + osmo_apn_to_str(str1, apn, apn_len), + osmo_apn_to_str(str2, peer->cfg->core_apn, peer->cfg->core_apn_size)); *new_apn_ie_len = peer->cfg->core_apn_size + 2; diff --git a/openbsc/src/gprs/gb_proxy_vty.c b/openbsc/src/gprs/gb_proxy_vty.c index 933b6b0..bf11a6d 100644 --- a/openbsc/src/gprs/gb_proxy_vty.c +++ b/openbsc/src/gprs/gb_proxy_vty.c @@ -29,10 +29,10 @@ #include <openbsc/gsm_04_08.h> #include <osmocom/gprs/gprs_ns.h> +#include <osmocom/gsm/apn.h> #include <openbsc/debug.h> #include <openbsc/gb_proxy.h> -#include <openbsc/gprs_utils.h> #include <openbsc/vty.h> #include <osmocom/vty/command.h> @@ -107,7 +107,7 @@ static int config_write_gbproxy(struct vty *vty) if (g_cfg->core_apn_size > 0) { char str[500] = {0}; vty_out(vty, " core-access-point-name %s%s", - gprs_apn_to_str(str, g_cfg->core_apn, + osmo_apn_to_str(str, g_cfg->core_apn, g_cfg->core_apn_size), VTY_NEWLINE); } else { @@ -279,7 +279,7 @@ static int set_core_apn(struct vty *vty, const char *apn) g_cfg->core_apn = talloc_realloc_size(NULL, g_cfg->core_apn, apn_len + 1); g_cfg->core_apn_size = - gprs_str_to_apn(g_cfg->core_apn, apn_len + 1, apn); + osmo_apn_from_str(g_cfg->core_apn, apn_len + 1, apn); } return CMD_SUCCESS; diff --git a/openbsc/src/gprs/gprs_sgsn.c b/openbsc/src/gprs/gprs_sgsn.c index c4dc9d7..b770a3a 100644 --- a/openbsc/src/gprs/gprs_sgsn.c +++ b/openbsc/src/gprs/gprs_sgsn.c @@ -29,6 +29,7 @@ #include <osmocom/core/backtrace.h> #include <osmocom/gprs/gprs_ns.h> #include <osmocom/gprs/gprs_bssgp.h> +#include <osmocom/gsm/apn.h> #include <openbsc/gsm_subscriber.h> #include <openbsc/debug.h> @@ -36,7 +37,6 @@ #include <openbsc/sgsn.h> #include <openbsc/gsm_04_08_gprs.h> #include <openbsc/gprs_gmm.h> -#include <openbsc/gprs_utils.h> #include <openbsc/signal.h> #include "openbsc/gprs_llc.h" @@ -650,7 +650,7 @@ struct sgsn_ggsn_ctx *sgsn_mm_ctx_find_ggsn_ctx(struct sgsn_mm_ctx *mmctx, return NULL; } - gprs_apn_to_str(req_apn_str, + osmo_apn_to_str(req_apn_str, TLVP_VAL(tp, GSM48_IE_GSM_APN), TLVP_LEN(tp, GSM48_IE_GSM_APN)); diff --git a/openbsc/src/gprs/gprs_subscriber.c b/openbsc/src/gprs/gprs_subscriber.c index 3467293..8f5dc9d 100644 --- a/openbsc/src/gprs/gprs_subscriber.c +++ b/openbsc/src/gprs/gprs_subscriber.c @@ -27,10 +27,11 @@ #include <openbsc/gprs_sgsn.h> #include <openbsc/gprs_gmm.h> #include <openbsc/gprs_gsup_messages.h> -#include <openbsc/gprs_utils.h> #include <openbsc/debug.h> +#include <osmocom/gsm/apn.h> + #include <netinet/in.h> #include <arpa/inet.h> @@ -328,7 +329,7 @@ static void gprs_subscr_gsup_insert_data(struct gsm_subscriber *subscr, OSMO_ASSERT(pdp_data != NULL); pdp_data->pdp_type = pdp_info->pdp_type; - gprs_apn_to_str(pdp_data->apn_str, + osmo_apn_to_str(pdp_data->apn_str, pdp_info->apn_enc, pdp_info->apn_enc_len); memcpy(pdp_data->qos_subscribed, pdp_info->qos_enc, pdp_info->qos_enc_len); pdp_data->qos_subscribed_len = pdp_info->qos_enc_len; diff --git a/openbsc/src/gprs/gprs_utils.c b/openbsc/src/gprs/gprs_utils.c index ad479db..8a98ae6 100644 --- a/openbsc/src/gprs/gprs_utils.c +++ b/openbsc/src/gprs/gprs_utils.c @@ -113,68 +113,6 @@ int gprs_msgb_resize_area(struct msgb *msg, uint8_t *area, return 0; } -/* TODO: Move these conversion functions to a utils file. */ -/* TODO: consolidate with gprs_apn2str(). */ -/** memmove apn_enc to out_str, replacing the length octets in apn_enc with '.' - * (omitting the first one) and terminating with a '\0'. - * out_str needs to have rest_chars amount of bytes or 1 whatever is bigger. - */ -char * gprs_apn_to_str(char *out_str, const uint8_t *apn_enc, size_t rest_chars) -{ - char *str = out_str; - - while (rest_chars > 0 && apn_enc[0]) { - size_t label_size = apn_enc[0]; - if (label_size + 1 > rest_chars) - return NULL; - - memmove(str, apn_enc + 1, label_size); - str += label_size; - rest_chars -= label_size + 1; - apn_enc += label_size + 1; - - if (rest_chars) - *(str++) = '.'; - } - str[0] = '\0'; - - return out_str; -} - -int gprs_str_to_apn(uint8_t *apn_enc, size_t max_len, const char *str) -{ - uint8_t *last_len_field; - int len; - - /* Can we even write the length field to the output? */ - if (max_len == 0) - return -1; - - /* Remember where we need to put the length once we know it */ - last_len_field = apn_enc; - len = 1; - apn_enc += 1; - - while (str[0]) { - if (len >= max_len) - return -1; - - if (str[0] == '.') { - *last_len_field = (apn_enc - last_len_field) - 1; - last_len_field = apn_enc; - } else { - *apn_enc = str[0]; - } - apn_enc += 1; - str += 1; - len += 1; - } - - *last_len_field = (apn_enc - last_len_field) - 1; - - return len; -} - /* GSM 04.08, 10.5.7.3 GPRS Timer */ int gprs_tmr_to_secs(uint8_t tmr) { diff --git a/openbsc/src/gprs/gtphub.c b/openbsc/src/gprs/gtphub.c index f26a56a..09175eb 100644 --- a/openbsc/src/gprs/gtphub.c +++ b/openbsc/src/gprs/gtphub.c @@ -34,13 +34,13 @@ #include <openbsc/gtphub.h> #include <openbsc/debug.h> -#include <openbsc/gprs_utils.h> #include <osmocom/core/utils.h> #include <osmocom/core/logging.h> #include <osmocom/core/socket.h> #include <osmocom/core/rate_ctr.h> #include <osmocom/core/stats.h> +#include <osmocom/gsm/apn.h> static const int GTPH_GC_TICK_SECONDS = 1; @@ -498,7 +498,7 @@ static int get_ie_apn_str(union gtpie_member *ie[], const char **apn_str) len = sizeof(apn_buf) - 1; apn_buf[len] = '\0'; - *apn_str = gprs_apn_to_str(apn_buf, (uint8_t*)apn_buf, len); + *apn_str = osmo_apn_to_str(apn_buf, (uint8_t*)apn_buf, len); if (!(*apn_str)) { LOG(LOGL_ERROR, "APN IE: present but cannot be decoded: %s\n", osmo_hexdump((uint8_t*)apn_buf, len)); diff --git a/openbsc/src/gprs/sgsn_cdr.c b/openbsc/src/gprs/sgsn_cdr.c index d0cb712..5d16249 100644 --- a/openbsc/src/gprs/sgsn_cdr.c +++ b/openbsc/src/gprs/sgsn_cdr.c @@ -20,11 +20,12 @@ #include <openbsc/sgsn.h> #include <openbsc/signal.h> -#include <openbsc/gprs_utils.h> #include <openbsc/debug.h> #include <openbsc/vty.h> +#include <osmocom/gsm/apn.h> + #include <gtp.h> #include <pdp.h> @@ -145,7 +146,7 @@ static void cdr_log_pdp(struct sgsn_instance *inst, const char *ev, if (pdp->lib) { - gprs_apn_to_str(apni, pdp->lib->apn_use.v, pdp->lib->apn_use.l); + osmo_apn_to_str(apni, pdp->lib->apn_use.v, pdp->lib->apn_use.l); inet_ntop(AF_INET, &pdp->lib->hisaddr0.s_addr, ggsn_addr, sizeof(ggsn_addr)); extract_eua(&pdp->lib->eua, eua_addr); } diff --git a/openbsc/src/gprs/sgsn_vty.c b/openbsc/src/gprs/sgsn_vty.c index 3f61163..706c9ea 100644 --- a/openbsc/src/gprs/sgsn_vty.c +++ b/openbsc/src/gprs/sgsn_vty.c @@ -110,7 +110,7 @@ DECLARE_TIMER(3397, "Wait for DEACT AA PDP CTX ACK timer (s)") #define GSM48_MAX_APN_LEN 102 /* 10.5.6.1 */ -/* TODO: consolidate with gprs_apn_to_str(). */ +/* TODO: consolidate with osmo_apn_to_str(). */ /** Copy apn to a static buffer, replacing the length octets in apn_enc with '.' * and terminating with a '\0'. Return the static buffer. * len: the length of the encoded APN (which has no terminating zero). diff --git a/openbsc/tests/gbproxy/gbproxy_test.c b/openbsc/tests/gbproxy/gbproxy_test.c index 0ba827f..97aa32f 100644 --- a/openbsc/tests/gbproxy/gbproxy_test.c +++ b/openbsc/tests/gbproxy/gbproxy_test.c @@ -26,6 +26,7 @@ #include <osmocom/core/rate_ctr.h> #include <osmocom/gsm/tlv.h> #include <osmocom/gsm/gsm_utils.h> +#include <osmocom/gsm/apn.h> #include <osmocom/gprs/gprs_msgb.h> #include <osmocom/gprs/gprs_ns.h> #include <osmocom/gprs/gprs_bssgp.h> @@ -1660,7 +1661,7 @@ static void test_gbproxy_ra_patching() gbcfg.core_mcc = 123; gbcfg.core_mnc = 456; gbcfg.core_apn = talloc_zero_size(NULL, 100); - gbcfg.core_apn_size = gprs_str_to_apn(gbcfg.core_apn, 100, "foo.bar"); + gbcfg.core_apn_size = osmo_apn_from_str(gbcfg.core_apn, 100, "foo.bar"); gbcfg.patch_ptmsi = 0; configure_sgsn_peer(&sgsn_peer); @@ -2001,7 +2002,7 @@ static void test_gbproxy_ptmsi_assignment() gbcfg.core_mcc = 0; gbcfg.core_mnc = 0; gbcfg.core_apn = talloc_zero_size(NULL, 100); - gbcfg.core_apn_size = gprs_str_to_apn(gbcfg.core_apn, 100, "foo.bar"); + gbcfg.core_apn_size = osmo_apn_from_str(gbcfg.core_apn, 100, "foo.bar"); gbcfg.patch_ptmsi = 0; configure_sgsn_peer(&sgsn_peer); @@ -2235,7 +2236,7 @@ static void test_gbproxy_ptmsi_patching() gbcfg.core_mcc = 123; gbcfg.core_mnc = 456; gbcfg.core_apn = talloc_zero_size(NULL, 100); - gbcfg.core_apn_size = gprs_str_to_apn(gbcfg.core_apn, 100, "foo.bar"); + gbcfg.core_apn_size = osmo_apn_from_str(gbcfg.core_apn, 100, "foo.bar"); gbcfg.patch_ptmsi = 1; configure_sgsn_peer(&sgsn_peer); @@ -2554,7 +2555,7 @@ static void test_gbproxy_ptmsi_patching_bad_cases() gbcfg.core_mcc = 123; gbcfg.core_mnc = 456; gbcfg.core_apn = talloc_zero_size(NULL, 100); - gbcfg.core_apn_size = gprs_str_to_apn(gbcfg.core_apn, 100, "foo.bar"); + gbcfg.core_apn_size = osmo_apn_from_str(gbcfg.core_apn, 100, "foo.bar"); gbcfg.patch_ptmsi = 1; configure_sgsn_peer(&sgsn_peer); @@ -2738,7 +2739,7 @@ static void test_gbproxy_imsi_acquisition() gbcfg.core_mcc = 123; gbcfg.core_mnc = 456; gbcfg.core_apn = talloc_zero_size(NULL, 100); - gbcfg.core_apn_size = gprs_str_to_apn(gbcfg.core_apn, 100, "foo.bar"); + gbcfg.core_apn_size = osmo_apn_from_str(gbcfg.core_apn, 100, "foo.bar"); gbcfg.patch_ptmsi = 1; gbcfg.acquire_imsi = 1; @@ -3064,7 +3065,7 @@ static void test_gbproxy_secondary_sgsn() gbcfg.core_mcc = 123; gbcfg.core_mnc = 456; gbcfg.core_apn = talloc_zero_size(NULL, 100); - gbcfg.core_apn_size = gprs_str_to_apn(gbcfg.core_apn, 100, "foo.bar"); + gbcfg.core_apn_size = osmo_apn_from_str(gbcfg.core_apn, 100, "foo.bar"); gbcfg.patch_ptmsi = 1; gbcfg.acquire_imsi = 1; diff --git a/openbsc/tests/gprs/gprs_test.c b/openbsc/tests/gprs/gprs_test.c index c78b98a..20b0bb4 100644 --- a/openbsc/tests/gprs/gprs_test.c +++ b/openbsc/tests/gprs/gprs_test.c @@ -10,6 +10,7 @@ #include <openbsc/debug.h> #include <osmocom/core/application.h> +#include <osmocom/gsm/apn.h> #define ASSERT_FALSE(x) if (x) { printf("Should have returned false.\n"); abort(); } #define ASSERT_TRUE(x) if (!x) { printf("Should have returned true.\n"); abort(); } @@ -59,7 +60,7 @@ static void apn_round_trip(const uint8_t *input, size_t len, const char *wanted_ int enc_len; /* decode and verify we have what we want */ - out_str = gprs_apn_to_str(output, input, len); + out_str = osmo_apn_to_str(output, input, len); OSMO_ASSERT(out_str); OSMO_ASSERT(out_str == &output[0]); OSMO_ASSERT(strlen(out_str) == strlen(wanted_output)); @@ -67,11 +68,11 @@ static void apn_round_trip(const uint8_t *input, size_t len, const char *wanted_ /* encode and verify it */ if (len != 0) { - enc_len = gprs_str_to_apn(encoded, ARRAY_SIZE(encoded), wanted_output); + enc_len = osmo_apn_from_str(encoded, ARRAY_SIZE(encoded), wanted_output); OSMO_ASSERT(enc_len == len); OSMO_ASSERT(memcmp(encoded, input, enc_len) == 0); } else { - enc_len = gprs_str_to_apn(encoded, 0, wanted_output); + enc_len = osmo_apn_from_str(encoded, 0, wanted_output); OSMO_ASSERT(enc_len == -1); } } @@ -86,27 +87,27 @@ static void test_gsm_03_03_apn(void) int enc_len; memcpy(output, ref, ARRAY_SIZE(output)); - enc_len = gprs_str_to_apn(output, 0, ""); + enc_len = osmo_apn_from_str(output, 0, ""); OSMO_ASSERT(enc_len == -1); OSMO_ASSERT(memcmp(ref, output, ARRAY_SIZE(ref)) == 0); memcpy(output, ref, ARRAY_SIZE(output)); - enc_len = gprs_str_to_apn(output, 0, "foo"); + enc_len = osmo_apn_from_str(output, 0, "foo"); OSMO_ASSERT(enc_len == -1); OSMO_ASSERT(memcmp(ref, output, ARRAY_SIZE(ref)) == 0); memcpy(output, ref, ARRAY_SIZE(output)); - enc_len = gprs_str_to_apn(output, 1, "foo"); + enc_len = osmo_apn_from_str(output, 1, "foo"); OSMO_ASSERT(enc_len == -1); OSMO_ASSERT(memcmp(ref + 1, output + 1, ARRAY_SIZE(ref) - 1) == 0); memcpy(output, ref, ARRAY_SIZE(output)); - enc_len = gprs_str_to_apn(output, 2, "foo"); + enc_len = osmo_apn_from_str(output, 2, "foo"); OSMO_ASSERT(enc_len == -1); OSMO_ASSERT(memcmp(ref + 2, output + 2, ARRAY_SIZE(ref) - 2) == 0); memcpy(output, ref, ARRAY_SIZE(output)); - enc_len = gprs_str_to_apn(output, 3, "foo"); + enc_len = osmo_apn_from_str(output, 3, "foo"); OSMO_ASSERT(enc_len == -1); OSMO_ASSERT(memcmp(ref + 3, output + 3, ARRAY_SIZE(ref) - 3) == 0); } @@ -130,7 +131,7 @@ static void test_gsm_03_03_apn(void) uint8_t input[] = { 0x1, 65 }; const char *output = "A"; apn_round_trip(input, ARRAY_SIZE(input), output); - OSMO_ASSERT(gprs_apn_to_str(NULL, input, ARRAY_SIZE(input) - 1) == NULL); + OSMO_ASSERT(osmo_apn_to_str(NULL, input, ARRAY_SIZE(input) - 1) == NULL); } { @@ -138,11 +139,11 @@ static void test_gsm_03_03_apn(void) const char *output = "ABC.Zz"; char tmp[strlen(output) + 1]; apn_round_trip(input, ARRAY_SIZE(input), output); - OSMO_ASSERT(gprs_apn_to_str(tmp, input, ARRAY_SIZE(input) - 1) == NULL); - OSMO_ASSERT(gprs_apn_to_str(tmp, input, ARRAY_SIZE(input) - 2) == NULL); - OSMO_ASSERT(gprs_apn_to_str(tmp, input, ARRAY_SIZE(input) - 4) == NULL); - OSMO_ASSERT(gprs_apn_to_str(tmp, input, ARRAY_SIZE(input) - 5) == NULL); - OSMO_ASSERT(gprs_apn_to_str(tmp, input, ARRAY_SIZE(input) - 6) == NULL); + OSMO_ASSERT(osmo_apn_to_str(tmp, input, ARRAY_SIZE(input) - 1) == NULL); + OSMO_ASSERT(osmo_apn_to_str(tmp, input, ARRAY_SIZE(input) - 2) == NULL); + OSMO_ASSERT(osmo_apn_to_str(tmp, input, ARRAY_SIZE(input) - 4) == NULL); + OSMO_ASSERT(osmo_apn_to_str(tmp, input, ARRAY_SIZE(input) - 5) == NULL); + OSMO_ASSERT(osmo_apn_to_str(tmp, input, ARRAY_SIZE(input) - 6) == NULL); } } diff --git a/openbsc/tests/gtphub/Makefile.am b/openbsc/tests/gtphub/Makefile.am index dcb7211..7599119 100644 --- a/openbsc/tests/gtphub/Makefile.am +++ b/openbsc/tests/gtphub/Makefile.am @@ -20,5 +20,6 @@ gtphub_test_LDADD = \ $(top_builddir)/src/gprs/gtphub.o \ $(top_builddir)/src/gprs/gprs_utils.o \ $(LIBOSMOCORE_LIBS) \ + $(LIBOSMOGSM_LIBS) \ -lgtp -lrt diff --git a/openbsc/tests/sgsn/sgsn_test.c b/openbsc/tests/sgsn/sgsn_test.c index 859223f..5df7688 100644 --- a/openbsc/tests/sgsn/sgsn_test.c +++ b/openbsc/tests/sgsn/sgsn_test.c @@ -31,7 +31,8 @@ #include <osmocom/gprs/gprs_bssgp.h> -#include <osmocom/gsm/gsm_utils.h> +#include <osmocom/gsm/apn.h> + #include <openbsc/gsm_04_08_gprs.h> #include <osmocom/core/application.h> @@ -1957,7 +1958,7 @@ static void test_ggsn_selection(void) /* Resolve GGSNs */ tp.lv[GSM48_IE_GSM_APN].len = - gprs_str_to_apn(apn_enc, sizeof(apn_enc), "Test.Apn"); + osmo_apn_from_str(apn_enc, sizeof(apn_enc), "Test.Apn"); ggc = sgsn_mm_ctx_find_ggsn_ctx(ctx, &tp, &gsm_cause, apn_str); OSMO_ASSERT(ggc != NULL); @@ -1965,7 +1966,7 @@ static void test_ggsn_selection(void) OSMO_ASSERT(strcmp(apn_str, "Test.Apn") == 0); tp.lv[GSM48_IE_GSM_APN].len = - gprs_str_to_apn(apn_enc, sizeof(apn_enc), "Other.Apn"); + osmo_apn_from_str(apn_enc, sizeof(apn_enc), "Other.Apn"); ggc = sgsn_mm_ctx_find_ggsn_ctx(ctx, &tp, &gsm_cause, apn_str); OSMO_ASSERT(ggc != NULL); @@ -1991,7 +1992,7 @@ static void test_ggsn_selection(void) tp.lv[GSM48_IE_GSM_APN].val = apn_enc; tp.lv[GSM48_IE_GSM_APN].len = - gprs_str_to_apn(apn_enc, sizeof(apn_enc), "Foo.Bar"); + osmo_apn_from_str(apn_enc, sizeof(apn_enc), "Foo.Bar"); ggc = sgsn_mm_ctx_find_ggsn_ctx(ctx, &tp, &gsm_cause, apn_str); OSMO_ASSERT(ggc == NULL); @@ -2008,7 +2009,7 @@ static void test_ggsn_selection(void) strncpy(pdp_data->apn_str, "Test.Apn", sizeof(pdp_data->apn_str)-1); tp.lv[GSM48_IE_GSM_APN].len = - gprs_str_to_apn(apn_enc, sizeof(apn_enc), "Test.Apn"); + osmo_apn_from_str(apn_enc, sizeof(apn_enc), "Test.Apn"); ggc = sgsn_mm_ctx_find_ggsn_ctx(ctx, &tp, &gsm_cause, apn_str); OSMO_ASSERT(ggc != NULL); @@ -2016,7 +2017,7 @@ static void test_ggsn_selection(void) OSMO_ASSERT(strcmp(apn_str, "Test.Apn") == 0); tp.lv[GSM48_IE_GSM_APN].len = - gprs_str_to_apn(apn_enc, sizeof(apn_enc), "Other.Apn"); + osmo_apn_from_str(apn_enc, sizeof(apn_enc), "Other.Apn"); ggc = sgsn_mm_ctx_find_ggsn_ctx(ctx, &tp, &gsm_cause, apn_str); OSMO_ASSERT(ggc == NULL); -- 1.9.1
These functions are moved to libosmocore. This commit removes the definitions and changes the names to to the corresponding libosmogb/libosmocore ones: gprs_msgb_copy -> bssgp_msgb_copy (libosmogb) gprs_msgb_resize_area -> msgb_resize_area (libosmocore) Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/gprs_utils.h | 8 ++-- openbsc/src/gprs/gb_proxy.c | 8 ++-- openbsc/src/gprs/gb_proxy_patch.c | 4 +- openbsc/src/gprs/gprs_gmm.c | 2 +- openbsc/src/gprs/gprs_utils.c | 84 ------------------------------------ openbsc/tests/gbproxy/gbproxy_test.c | 2 +- 6 files changed, 13 insertions(+), 95 deletions(-) diff --git a/openbsc/include/openbsc/gprs_utils.h b/openbsc/include/openbsc/gprs_utils.h index 7af83ba..8942e40 100644 --- a/openbsc/include/openbsc/gprs_utils.h +++ b/openbsc/include/openbsc/gprs_utils.h @@ -27,9 +27,11 @@ struct msgb; -struct msgb *gprs_msgb_copy(const struct msgb *msg, const char *name); -int gprs_msgb_resize_area(struct msgb *msg, uint8_t *area, - size_t old_size, size_t new_size); +char *gprs_apn_to_str(char *out_str, const uint8_t *apn_enc, size_t rest_chars) + OSMO_DEPRECATED("Use osmo_apn_to_str instead"); +int gprs_str_to_apn(uint8_t *apn_enc, size_t max_len, const char *str) + OSMO_DEPRECATED("Use osmo_apn_from_str instead"); + /* GSM 04.08, 10.5.7.3 GPRS Timer */ int gprs_tmr_to_secs(uint8_t tmr); uint8_t gprs_secs_to_tmr_floor(int secs); diff --git a/openbsc/src/gprs/gb_proxy.c b/openbsc/src/gprs/gb_proxy.c index 6cad651..7a290d3 100644 --- a/openbsc/src/gprs/gb_proxy.c +++ b/openbsc/src/gprs/gb_proxy.c @@ -486,7 +486,7 @@ static int gbproxy_imsi_acquisition(struct gbproxy_peer *peer, msgb_nsei(msg), parse_ctx->llc_msg_name ? parse_ctx->llc_msg_name : "BSSGP"); - stored_msg = gprs_msgb_copy(msg, "process_bssgp_ul"); + stored_msg = bssgp_msgb_copy(msg, "process_bssgp_ul"); msgb_enqueue(&link_info->stored_msgs, stored_msg); if (!link_info->imsi_acq_pending) { @@ -750,7 +750,7 @@ static int gbprox_relay2sgsn(struct gbproxy_config *cfg, struct msgb *old_msg, { /* create a copy of the message so the old one can * be free()d safely when we return from gbprox_rcvmsg() */ - struct msgb *msg = gprs_msgb_copy(old_msg, "msgb_relay2sgsn"); + struct msgb *msg = bssgp_msgb_copy(old_msg, "msgb_relay2sgsn"); int rc; DEBUGP(DGPRS, "NSEI=%u proxying BTS->SGSN (NS_BVCI=%u, NSEI=%u)\n", @@ -774,7 +774,7 @@ static int gbprox_relay2peer(struct msgb *old_msg, struct gbproxy_peer *peer, { /* create a copy of the message so the old one can * be free()d safely when we return from gbprox_rcvmsg() */ - struct msgb *msg = gprs_msgb_copy(old_msg, "msgb_relay2peer"); + struct msgb *msg = bssgp_msgb_copy(old_msg, "msgb_relay2peer"); int rc; DEBUGP(DGPRS, "NSEI=%u proxying SGSN->BSS (NS_BVCI=%u, NSEI=%u)\n", @@ -1169,7 +1169,7 @@ static int gbprox_rx_sig_from_sgsn(struct gbproxy_config *cfg, return bssgp_tx_status(BSSGP_CAUSE_PROTO_ERR_UNSPEC, NULL, orig_msg); } - msg = gprs_msgb_copy(orig_msg, "rx_sig_from_sgsn"); + msg = bssgp_msgb_copy(orig_msg, "rx_sig_from_sgsn"); gbprox_process_bssgp_dl(cfg, msg, NULL); /* Update message info */ bgph = (struct bssgp_normal_hdr *) msgb_bssgph(msg); diff --git a/openbsc/src/gprs/gb_proxy_patch.c b/openbsc/src/gprs/gb_proxy_patch.c index 725a863..9c3510a 100644 --- a/openbsc/src/gprs/gb_proxy_patch.c +++ b/openbsc/src/gprs/gb_proxy_patch.c @@ -106,7 +106,7 @@ static void gbproxy_patch_apn_ie(struct msgb *msg, osmo_apn_to_str(str1, apn, apn_len)); *new_apn_ie_len = 0; - gprs_msgb_resize_area(msg, apn_ie, apn_ie_len, 0); + msgb_resize_area(msg, apn_ie, apn_ie_len, 0); } else { /* Resize the IE */ char str1[110]; @@ -123,7 +123,7 @@ static void gbproxy_patch_apn_ie(struct msgb *msg, peer->cfg->core_apn_size)); *new_apn_ie_len = peer->cfg->core_apn_size + 2; - gprs_msgb_resize_area(msg, apn, apn_len, peer->cfg->core_apn_size); + msgb_resize_area(msg, apn, apn_len, peer->cfg->core_apn_size); memcpy(apn, peer->cfg->core_apn, peer->cfg->core_apn_size); hdr->apn_len = peer->cfg->core_apn_size; } diff --git a/openbsc/src/gprs/gprs_gmm.c b/openbsc/src/gprs/gprs_gmm.c index 115e898..5ad8fd2 100644 --- a/openbsc/src/gprs/gprs_gmm.c +++ b/openbsc/src/gprs/gprs_gmm.c @@ -1888,7 +1888,7 @@ static int gsm48_rx_gsm_act_pdp_req(struct sgsn_mm_ctx *mmctx, * and the dynamic resolution will be the right thing * in the long run. */ - msg = gprs_msgb_copy(_msg, __func__); + msg = bssgp_msgb_copy(_msg, __func__); if (!msg) { struct gsm48_hdr *gh = (struct gsm48_hdr *) msgb_gmmh(_msg); uint8_t transaction_id = (gh->proto_discr >> 4); diff --git a/openbsc/src/gprs/gprs_utils.c b/openbsc/src/gprs/gprs_utils.c index 8a98ae6..ee6adfc 100644 --- a/openbsc/src/gprs/gprs_utils.c +++ b/openbsc/src/gprs/gprs_utils.c @@ -29,90 +29,6 @@ #include <string.h> -/* FIXME: this needs to go to libosmocore/msgb.c */ -struct msgb *gprs_msgb_copy(const struct msgb *msg, const char *name) -{ - struct libgb_msgb_cb *old_cb, *new_cb; - struct msgb *new_msg; - - new_msg = msgb_alloc(msg->data_len, name); - if (!new_msg) - return NULL; - - /* copy data */ - memcpy(new_msg->_data, msg->_data, new_msg->data_len); - - /* copy header */ - new_msg->len = msg->len; - new_msg->data += msg->data - msg->_data; - new_msg->head += msg->head - msg->_data; - new_msg->tail += msg->tail - msg->_data; - - if (msg->l1h) - new_msg->l1h = new_msg->_data + (msg->l1h - msg->_data); - if (msg->l2h) - new_msg->l2h = new_msg->_data + (msg->l2h - msg->_data); - if (msg->l3h) - new_msg->l3h = new_msg->_data + (msg->l3h - msg->_data); - if (msg->l4h) - new_msg->l4h = new_msg->_data + (msg->l4h - msg->_data); - - /* copy GB specific data */ - old_cb = LIBGB_MSGB_CB(msg); - new_cb = LIBGB_MSGB_CB(new_msg); - - if (old_cb->bssgph) - new_cb->bssgph = new_msg->_data + (old_cb->bssgph - msg->_data); - if (old_cb->llch) - new_cb->llch = new_msg->_data + (old_cb->llch - msg->_data); - - /* bssgp_cell_id is a pointer into the old msgb, so we need to make - * it a pointer into the new msgb */ - if (old_cb->bssgp_cell_id) - new_cb->bssgp_cell_id = new_msg->_data + - (old_cb->bssgp_cell_id - msg->_data); - new_cb->nsei = old_cb->nsei; - new_cb->bvci = old_cb->bvci; - new_cb->tlli = old_cb->tlli; - - return new_msg; -} - -/* TODO: Move this to libosmocore/msgb.c */ -int gprs_msgb_resize_area(struct msgb *msg, uint8_t *area, - size_t old_size, size_t new_size) -{ - int rc; - uint8_t *rest = area + old_size; - int rest_len = msg->len - old_size - (area - msg->data); - int delta_size = (int)new_size - (int)old_size; - - if (delta_size == 0) - return 0; - - if (delta_size > 0) { - rc = msgb_trim(msg, msg->len + delta_size); - if (rc < 0) - return rc; - } - - memmove(area + new_size, area + old_size, rest_len); - - if (msg->l1h >= rest) - msg->l1h += delta_size; - if (msg->l2h >= rest) - msg->l2h += delta_size; - if (msg->l3h >= rest) - msg->l3h += delta_size; - if (msg->l4h >= rest) - msg->l4h += delta_size; - - if (delta_size < 0) - msgb_trim(msg, msg->len + delta_size); - - return 0; -} - /* GSM 04.08, 10.5.7.3 GPRS Timer */ int gprs_tmr_to_secs(uint8_t tmr) { diff --git a/openbsc/tests/gbproxy/gbproxy_test.c b/openbsc/tests/gbproxy/gbproxy_test.c index 97aa32f..d5284a9 100644 --- a/openbsc/tests/gbproxy/gbproxy_test.c +++ b/openbsc/tests/gbproxy/gbproxy_test.c @@ -1064,7 +1064,7 @@ int gprs_ns_sendmsg(struct gprs_ns_inst *nsi, struct msgb *msg) if (received_messages) { struct msgb *msg_copy; - msg_copy = gprs_msgb_copy(msg, "received_messages"); + msg_copy = bssgp_msgb_copy(msg, "received_messages"); llist_add_tail(&msg_copy->list, received_messages); } -- 1.9.1
This commit replaces the implementation of gprs_apn2str by a call to osmo_apn_to_str. Sponsored-by: On-Waves ehf --- openbsc/src/gprs/sgsn_vty.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/openbsc/src/gprs/sgsn_vty.c b/openbsc/src/gprs/sgsn_vty.c index 706c9ea..384f1f8 100644 --- a/openbsc/src/gprs/sgsn_vty.c +++ b/openbsc/src/gprs/sgsn_vty.c @@ -40,6 +40,8 @@ #include <osmocom/vty/vty.h> #include <osmocom/vty/misc.h> +#include <osmocom/gsm/apn.h> + #include <osmocom/abis/ipa.h> #include <pdp.h> @@ -110,7 +112,6 @@ DECLARE_TIMER(3397, "Wait for DEACT AA PDP CTX ACK timer (s)") #define GSM48_MAX_APN_LEN 102 /* 10.5.6.1 */ -/* TODO: consolidate with osmo_apn_to_str(). */ /** Copy apn to a static buffer, replacing the length octets in apn_enc with '.' * and terminating with a '\0'. Return the static buffer. * len: the length of the encoded APN (which has no terminating zero). @@ -118,25 +119,8 @@ DECLARE_TIMER(3397, "Wait for DEACT AA PDP CTX ACK timer (s)") static char *gprs_apn2str(uint8_t *apn, unsigned int len) { static char apnbuf[GSM48_MAX_APN_LEN+1]; - unsigned int i = 0; - - if (!apn) - return ""; - - if (len > sizeof(apnbuf)-1) - len = sizeof(apnbuf)-1; - - memcpy(apnbuf, apn, len); - apnbuf[len] = '\0'; - - /* replace the domain name step sizes with dots */ - while (i < len) { - unsigned int step = apnbuf[i]; - apnbuf[i] = '.'; - i += step+1; - } - return apnbuf+1; + return osmo_apn_to_str(apnbuf, apn, len); } char *gprs_pdpaddr2str(uint8_t *pdpa, uint8_t len) -- 1.9.1
+/*! \brief Copy an msgb. I'd write just "a" here, not "an". I seem to be the English nitpicker among us ;) + * + * This function allocates a new msgb, copies the data buffer of msg, + * and adjusts the pointers (incl l1h-l4h) accordingly. The cb part + * is not copied. + * \param[in] msg The old msgb object + * \param[in] name Human-readable name to be associated with msgb + */ +struct msgb *msgb_copy(const struct msgb *msg, const char *name) +{ [...] +} + +/*! \brief Resize an area within an msgb + * + * This resizes a sub area of the msgb data and adjusts the pointers (incl + * l1h-l4h) accordingly. The cb part is not updated. If the area is extended, + * the contents of the extension is undefined. The complete sub area must be a + * part of [data,tail]. + * + * \param[inout] msg The msgb object + * \param[in] area A pointer to the sub-area + * \param[in] old_size The old size of the sub-area + * \param[in] new_size The new size of the sub-area + * \returns 0 on success, -1 if there is not enough space to extend the area + */ +int msgb_resize_area(struct msgb *msg, uint8_t *area, + size_t old_size, size_t new_size) +{ + int rc; + uint8_t *rest = area + old_size; + int rest_len = msg->len - old_size - (area - msg->data); + int delta_size = (int)new_size - (int)old_size; + + if (area < msg->data || rest > msg->tail) + MSGB_ABORT(msg, "Sub area is not fully contained in the msg data\n"); Just to be super paranoid: old_size is unsigned, sure, but uint8_t *rest could wrap when old_size is (accidentally/crafted) passed as very very large. I could pass area > msg->tail with rest < msg->tail. Also, if new_size were past INT_MAX, (int)new_size would end up negative. Same for old_size. My head is spinning a bit from trying to figure out the result of the subtraction in those cases... ;) What do you think? Not relevant for any normal use, sure, but should we rule out those cases entirely? ~Neels
On 19.11.2015 15:34, Neels Hofmeyr wrote:
+/*! \brief Copy an msgb.
I'd write just "a" here, not "an". I seem to be the English nitpicker among us ;)
I do not agree in this case. "msgb" is read em-es-... thus starting with a vowel sound. See http://www.macmillandictionary.com/dictionary/british/an_1 ("an X-ray").
+int msgb_resize_area(struct msgb *msg, uint8_t *area, + size_t old_size, size_t new_size) +{ + int rc; + uint8_t *rest = area + old_size; + int rest_len = msg->len - old_size - (area - msg->data); + int delta_size = (int)new_size - (int)old_size; + + if (area < msg->data || rest > msg->tail) + MSGB_ABORT(msg, "Sub area is not fully contained in the msg data\n");
Just to be super paranoid: old_size is unsigned, sure, but uint8_t *rest could wrap when old_size is (accidentally/crafted) passed as very very large. I could pass area > msg->tail with rest < msg->tail.
Also, if new_size were past INT_MAX, (int)new_size would end up negative. Same for old_size. My head is spinning a bit from trying to figure out the result of the subtraction in those cases... ;)
What do you think? Not relevant for any normal use, sure, but should we rule out those cases entirely?
You are right. So a quick fix is to check for rest < area in addition. Jacob -- - Jacob Erlbeck <jerlbeck@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
On Tue, Nov 24, 2015 at 09:40:48AM +0100, Jacob Erlbeck wrote:
On 19.11.2015 15:34, Neels Hofmeyr wrote:
+/*! \brief Copy an msgb.
I'd write just "a" here, not "an". I seem to be the English nitpicker among us ;)
I do not agree in this case. "msgb" is read em-es-... thus starting with
Ah ok, I see your point. I tend to read "message-bee", starting with a consonant, so I'd have written "a". Keep the "an", then :)
+int msgb_resize_area(struct msgb *msg, uint8_t *area, + size_t old_size, size_t new_size) +{ + int rc; + uint8_t *rest = area + old_size; + int rest_len = msg->len - old_size - (area - msg->data); + int delta_size = (int)new_size - (int)old_size; + + if (area < msg->data || rest > msg->tail) + MSGB_ABORT(msg, "Sub area is not fully contained in the msg data\n");
check for rest < area in addition.
Sounds good. Actually, msgb's len fields are typed as uint16_t, so I think old_size and new_size should be uint16_t arguments...? And then, I think these are also necessary: - assert rest_len >= 0, which ensures a valid old_size, i.e. short for old_size <= (msg->len - (area - msg->data)). - assert new_size <= (0xffff - (area - msg->data)) (so that the resulting len is within uint16_t). It's quite time consuming to figure this out in theory, so maybe we can just merge this and have a "break msgb contest", rewarding an Osmocom mug for every msgb API test case that allows arbitrary memory access ;) ~Neels
On 25.11.2015 15:58, Neels Hofmeyr wrote:
Actually, msgb's len fields are typed as uint16_t, so I think old_size and new_size should be uint16_t arguments...?
I am not fond of using uint16_t in an API for such purposes, the compiler does not catch overflow, it might even be slower than an int when passed as an argument. In my opinion this is just a space optimization for the struct, and should not leak out to the API. Unfortunately the API is already inconsistent by using a mixture of int, unsigned int, and uint16_t types for length parameters. I agree, that size_t should not be added to that list. So the question remains, whether to use "int" or "unsigned int". K&R evangelists would probably go for "int", which has the advantage of to be able to compute differences without changing signedness on the way and to move the wrapping point far away. "unsigned int" had the advantage of not having to check for negative values and that it is more explicit in the API (BTW, msgb_trim uses int and does not check, thanks for the mug ;-) Prefering the "int" variant I have changed the function to int msgb_resize_area(struct msgb *msg, uint8_t *area, int old_size, int new_size) { int rc; uint8_t *rest = area + old_size; int rest_len = msg->len - old_size - (area - msg->data); int delta_size = new_size - old_size; if (old_size < 0 || new_size < 0) MSGB_ABORT(msg, "Negative sizes are not supported\n"); if (area < msg->data || rest > msg->tail) MSGB_ABORT(msg, "Sub area is not fully contained in the msg data\n"); ... } which I believe to be much clearer.
And then, I think these are also necessary:
[...]
- assert new_size <= (0xffff - (area - msg->data)) (so that the resulting len is within uint16_t).
Note that msgb_trim is responsible for checking the length (which should be fixed for negative values) and thus the upper limit of new_len. I was wondering about whether it makes sense, to handle the "buffer too small" case differently (by returning -1 instead of aborting, like msgb_trim), but could make sense in cases, where one can react to this by allocating a new buffer instead of patching the old one instead. Jacob -- - Jacob Erlbeck <jerlbeck@sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Alt-Moabit 93 * 10559 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
Hi, I have reworked the patches (thanks go to Neels for comments), fixed some additional flaws, and improved the test cases. Jacob
These functions originate from openbsc/src/gprs but are generic msgb helper functions. msgb_copy: This function allocates a new msgb, copies the data buffer of msg, and adjusts the pointers (incl. l1h-l4h) accordingly. msgb_resize_area: This resizes a sub area of the msgb data and adjusts the pointers (incl. l1h-l4h) accordingly. Sponsored-by: On-Waves ehf --- include/osmocom/core/msgb.h | 3 ++ src/msgb.c | 91 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/include/osmocom/core/msgb.h b/include/osmocom/core/msgb.h index 644a639..9ffc64e 100644 --- a/include/osmocom/core/msgb.h +++ b/include/osmocom/core/msgb.h @@ -74,6 +74,9 @@ extern struct msgb *msgb_dequeue(struct llist_head *queue); extern void msgb_reset(struct msgb *m); uint16_t msgb_length(const struct msgb *msg); extern const char *msgb_hexdump(const struct msgb *msg); +extern int msgb_resize_area(struct msgb *msg, uint8_t *area, + int old_size, int new_size); +extern struct msgb *msgb_copy(const struct msgb *msg, const char *name); #ifdef MSGB_DEBUG #include <osmocom/core/panic.h> diff --git a/src/msgb.c b/src/msgb.c index b2fe1d2..3132644 100644 --- a/src/msgb.c +++ b/src/msgb.c @@ -153,6 +153,97 @@ void msgb_set_talloc_ctx(void *ctx) tall_msgb_ctx = ctx; } +/*! \brief Copy an msgb. + * + * This function allocates a new msgb, copies the data buffer of msg, + * and adjusts the pointers (incl l1h-l4h) accordingly. The cb part + * is not copied. + * \param[in] msg The old msgb object + * \param[in] name Human-readable name to be associated with msgb + */ +struct msgb *msgb_copy(const struct msgb *msg, const char *name) +{ + struct msgb *new_msg; + + new_msg = msgb_alloc(msg->data_len, name); + if (!new_msg) + return NULL; + + /* copy data */ + memcpy(new_msg->_data, msg->_data, new_msg->data_len); + + /* copy header */ + new_msg->len = msg->len; + new_msg->data += msg->data - msg->_data; + new_msg->head += msg->head - msg->_data; + new_msg->tail += msg->tail - msg->_data; + + if (msg->l1h) + new_msg->l1h = new_msg->_data + (msg->l1h - msg->_data); + if (msg->l2h) + new_msg->l2h = new_msg->_data + (msg->l2h - msg->_data); + if (msg->l3h) + new_msg->l3h = new_msg->_data + (msg->l3h - msg->_data); + if (msg->l4h) + new_msg->l4h = new_msg->_data + (msg->l4h - msg->_data); + + return new_msg; +} + +/*! \brief Resize an area within an msgb + * + * This resizes a sub area of the msgb data and adjusts the pointers (incl + * l1h-l4h) accordingly. The cb part is not updated. If the area is extended, + * the contents of the extension is undefined. The complete sub area must be a + * part of [data,tail]. + * + * \param[inout] msg The msgb object + * \param[in] area A pointer to the sub-area + * \param[in] old_size The old size of the sub-area + * \param[in] new_size The new size of the sub-area + * \returns 0 on success, -1 if there is not enough space to extend the area + */ +int msgb_resize_area(struct msgb *msg, uint8_t *area, + int old_size, int new_size) +{ + int rc; + uint8_t *post_start = area + old_size; + int pre_len = area - msg->data; + int post_len = msg->len - old_size - pre_len; + int delta_size = new_size - old_size; + + if (old_size < 0 || new_size < 0) + MSGB_ABORT(msg, "Negative sizes are not allowed\n"); + if (area < msg->data || post_start > msg->tail) + MSGB_ABORT(msg, "Sub area is not fully contained in the msg data\n"); + + if (delta_size == 0) + return 0; + + if (delta_size > 0) { + rc = msgb_trim(msg, msg->len + delta_size); + if (rc < 0) + return rc; + } + + memmove(area + new_size, area + old_size, post_len); + + if (msg->l1h >= post_start) + msg->l1h += delta_size; + if (msg->l2h >= post_start) + msg->l2h += delta_size; + if (msg->l3h >= post_start) + msg->l3h += delta_size; + if (msg->l4h >= post_start) + msg->l4h += delta_size; + + if (delta_size < 0) + msgb_trim(msg, msg->len + delta_size); + + return 0; +} + + /*! \brief Return a (static) buffer containing a hexdump of the msg * \param[in] msg message buffer * \returns a pointer to a static char array -- 1.9.1
This patch makes msgb_hexdump accept out of range lXh pointers and shows info about them instead of aborting the dump entirely. Sponsored-by: On-Waves ehf --- src/msgb.c | 46 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/src/msgb.c b/src/msgb.c index 3132644..4b108a4 100644 --- a/src/msgb.c +++ b/src/msgb.c @@ -266,10 +266,25 @@ const char *msgb_hexdump(const struct msgb *msg) if (!lxhs[i]) continue; - if (lxhs[i] < msg->data) - goto out_of_range; + if (lxhs[i] < msg->head) + continue; + if (lxhs[i] > msg->head + msg->data_len) + continue; if (lxhs[i] > msg->tail) - goto out_of_range; + continue; + if (lxhs[i] < msg->data || lxhs[i] > msg->tail) { + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "(L%d=data%+d) ", + i+1, lxhs[i] - msg->data); + buf_offs += nchars; + continue; + } + if (lxhs[i] < start) { + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "(L%d%+d) ", i+1, start - lxhs[i]); + buf_offs += nchars; + continue; + } nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, "%s[L%d]> ", osmo_hexdump(start, lxhs[i] - start), @@ -285,11 +300,28 @@ const char *msgb_hexdump(const struct msgb *msg) if (nchars < 0 || nchars + buf_offs >= sizeof(buf)) return "ERROR"; - return buf; + buf_offs += nchars; + + for (i = 0; i < ARRAY_SIZE(lxhs); i++) { + if (!lxhs[i]) + continue; + + if (lxhs[i] < msg->head || lxhs[i] > msg->head + msg->data_len) { + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "(L%d out of range) ", i+1); + } else if (lxhs[i] <= msg->data + msg->data_len && + lxhs[i] > msg->tail) { + nchars = snprintf(buf + buf_offs, sizeof(buf) - buf_offs, + "(L%d=tail%+d) ", + i+1, lxhs[i] - msg->tail); + } else + continue; + + if (nchars < 0 || nchars + buf_offs >= sizeof(buf)) + return "ERROR"; + buf_offs += nchars; + } -out_of_range: - nchars = snprintf(buf, sizeof(buf) - buf_offs, - "!!! L%d out of range", i+1); return buf; } -- 1.9.1
This adds a function that verifies whether a mgsb is consistent. Sponsored-by: On-Waves ehf --- include/osmocom/core/msgb.h | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/include/osmocom/core/msgb.h b/include/osmocom/core/msgb.h index 9ffc64e..9c99cad 100644 --- a/include/osmocom/core/msgb.h +++ b/include/osmocom/core/msgb.h @@ -77,6 +77,7 @@ extern const char *msgb_hexdump(const struct msgb *msg); extern int msgb_resize_area(struct msgb *msg, uint8_t *area, int old_size, int new_size); extern struct msgb *msgb_copy(const struct msgb *msg, const char *name); +static int msgb_test_invariant(const struct msgb *msg) __attribute__((pure)); #ifdef MSGB_DEBUG #include <osmocom/core/panic.h> @@ -412,6 +413,45 @@ static inline struct msgb *msgb_alloc_headroom(int size, int headroom, return msg; } +/*! \brief Check a message buffer for consistency + * \param[in] msg message buffer + * \returns 0 (false) if inconsistent, != 0 (true) otherwise + */ +static inline int msgb_test_invariant(const struct msgb *msg) +{ + const unsigned char *lbound; + if (!msg || !msg->data || !msg->tail || + (msg->data + msg->len != msg->tail) || + (msg->data < msg->head) || + (msg->tail > msg->head + msg->data_len)) + return 0; + + lbound = msg->head; + + if (msg->l1h) { + if (msg->l1h < lbound) + return 0; + lbound = msg->l1h; + } + if (msg->l2h) { + if (msg->l2h < lbound) + return 0; + lbound = msg->l2h; + } + if (msg->l3h) { + if (msg->l3h < lbound) + return 0; + lbound = msg->l3h; + } + if (msg->l4h) { + if (msg->l4h < lbound) + return 0; + lbound = msg->l4h; + } + + return lbound <= msg->head + msg->data_len; +} + /* non inline functions to ease binding */ uint8_t *msgb_data(const struct msgb *msg); -- 1.9.1
This tests several API functions of the msgb by checking the invariant and by dumping resulting message buffers as hex. Sponsored-by: On-Waves ehf Conflicts: tests/Makefile.am --- tests/Makefile.am | 9 ++++- tests/msgb/msgb_test.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/msgb/msgb_test.ok | 21 ++++++++++ tests/testsuite.at | 6 +++ 4 files changed, 138 insertions(+), 2 deletions(-) create mode 100644 tests/msgb/msgb_test.c create mode 100644 tests/msgb/msgb_test.ok diff --git a/tests/Makefile.am b/tests/Makefile.am index 4442355..edf25a0 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -10,7 +10,8 @@ check_PROGRAMS = timer/timer_test sms/sms_test ussd/ussd_test \ logging/logging_test fr/fr_test \ loggingrb/loggingrb_test strrb/strrb_test \ vty/vty_test comp128/comp128_test utils/utils_test \ - smscb/gsm0341_test stats/stats_test + smscb/gsm0341_test stats/stats_test \ + msgb/msgb_test if ENABLE_MSGFILE check_PROGRAMS += msgfile/msgfile_test @@ -52,6 +53,9 @@ gprs_gprs_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gs lapd_lapd_test_SOURCES = lapd/lapd_test.c lapd_lapd_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gsm/libosmogsm.la +msgb_msgb_test_SOURCES = msgb/msgb_test.c +msgb_msgb_test_LDADD = $(top_builddir)/src/libosmocore.la + msgfile_msgfile_test_SOURCES = msgfile/msgfile_test.c msgfile_msgfile_test_LDADD = $(top_builddir)/src/libosmocore.la @@ -127,7 +131,8 @@ EXTRA_DIST = testsuite.at $(srcdir)/package.m4 $(TESTSUITE) \ fr/fr_test.ok loggingrb/logging_test.ok \ loggingrb/logging_test.err strrb/strrb_test.ok \ vty/vty_test.ok comp128/comp128_test.ok \ - utils/utils_test.ok stats/stats_test.ok + utils/utils_test.ok stats/stats_test.ok \ + msgb/msgb_test.ok DISTCLEANFILES = atconfig diff --git a/tests/msgb/msgb_test.c b/tests/msgb/msgb_test.c new file mode 100644 index 0000000..7592509 --- /dev/null +++ b/tests/msgb/msgb_test.c @@ -0,0 +1,104 @@ +/* + * (C) 2014 by On-Waves + * All Rights Reserved + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 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 General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + */ + +#include <stdlib.h> +#include <osmocom/core/application.h> +#include <osmocom/core/logging.h> +#include <osmocom/core/utils.h> +#include <osmocom/core/msgb.h> + +#include <errno.h> + +#include <string.h> + +#define CHECK_RC(rc) \ + if (rc != 0) { \ + printf("Operation failed rc=%d on %s:%d\n", rc, __FILE__, __LINE__); \ + abort(); \ + } + +static void test_msgb_api() +{ + struct msgb *msg = msgb_alloc_headroom(4096, 128, "data"); + unsigned char *cptr = NULL; + int rc; + + printf("Testing the msgb API\n"); + + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + cptr = msg->l1h = msgb_put(msg, 4); + printf("put(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + cptr = msg->l2h = msgb_put(msg, 4); + printf("put(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + cptr = msg->l3h = msgb_put(msg, 4); + printf("put(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + cptr = msg->l4h = msgb_put(msg, 4); + printf("put(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + OSMO_ASSERT(msgb_length(msg) == 16); + cptr = msgb_push(msg, 4); + printf("push(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + OSMO_ASSERT(msgb_length(msg) == 20); + rc = msgb_trim(msg, 16); + printf("trim(16) -> %d\n", rc); + CHECK_RC(rc); + OSMO_ASSERT(msgb_test_invariant(msg)); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_length(msg) == 16); + + cptr = msgb_get(msg, 4); + printf("get(4) -> data%+d\n", cptr - msg->data); + printf("Buffer: %s\n", msgb_hexdump(msg)); + OSMO_ASSERT(msgb_test_invariant(msg)); + OSMO_ASSERT(msgb_length(msg) == 12); + + printf("Test msgb_hexdump\n"); + msg->l1h = msg->head; + printf("Buffer: %s\n", msgb_hexdump(msg)); + msg->l3h = msg->data; + printf("Buffer: %s\n", msgb_hexdump(msg)); + msg->l3h = msg->head - 1; + printf("Buffer: %s\n", msgb_hexdump(msg)); + + msgb_free(msg); +} + +static struct log_info info = {}; + +int main(int argc, char **argv) +{ + osmo_init_logging(&info); + + test_msgb_api(); + + printf("Success.\n"); + + return 0; +} diff --git a/tests/msgb/msgb_test.ok b/tests/msgb/msgb_test.ok new file mode 100644 index 0000000..f8de0cd --- /dev/null +++ b/tests/msgb/msgb_test.ok @@ -0,0 +1,21 @@ +Testing the msgb API +Buffer: +put(4) -> data+0 +Buffer: [L1]> 00 00 00 00 +put(4) -> data+4 +Buffer: [L1]> 00 00 00 00 [L2]> 00 00 00 00 +put(4) -> data+8 +Buffer: [L1]> 00 00 00 00 [L2]> 00 00 00 00 [L3]> 00 00 00 00 +put(4) -> data+12 +Buffer: [L1]> 00 00 00 00 [L2]> 00 00 00 00 [L3]> 00 00 00 00 [L4]> 00 00 00 00 +push(4) -> data+0 +Buffer: 00 00 00 00 [L1]> 00 00 00 00 [L2]> 00 00 00 00 [L3]> 00 00 00 00 [L4]> 00 00 00 00 +trim(16) -> 0 +Buffer: 00 00 00 00 [L1]> 00 00 00 00 [L2]> 00 00 00 00 [L3]> 00 00 00 00 [L4]> +get(4) -> data+12 +Buffer: 00 00 00 00 [L1]> 00 00 00 00 [L2]> 00 00 00 00 [L3]> (L4=tail+4) +Test msgb_hexdump +Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> 00 00 00 00 [L3]> (L4=tail+4) +Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> (L3+8) 00 00 00 00 (L4=tail+4) +Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> 00 00 00 00 (L3 out of range) (L4=tail+4) +Success. diff --git a/tests/testsuite.at b/tests/testsuite.at index 85c3e8b..12199e3 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -27,6 +27,12 @@ cat $abs_srcdir/conv/conv_test.ok > expout AT_CHECK([$abs_top_builddir/tests/conv/conv_test], [0], [expout]) AT_CLEANUP +AT_SETUP([msgb]) +AT_KEYWORDS([msgb]) +cat $abs_srcdir/msgb/msgb_test.ok > expout +AT_CHECK([$abs_top_builddir/tests/msgb/msgb_test], [0], [expout]) +AT_CLEANUP + if ENABLE_MSGFILE AT_SETUP([msgfile]) AT_KEYWORDS([msgfile]) -- 1.9.1
Currently the msgb error handling cannot be fully tested, since in many cases osmo_panic will be called. This will in turn call abort(). Using an osmo_panic_handler that just returns will not help, since many msgb functions rely on MSGB_ABORT to not return at all. This commit uses an alternative osmo_panic_raise handler that just calls longjmp to return to the test function. Since some of this activity is logged to stderr where the strings may contain variable parts like pointer addresses, stderr checking is disabled in testsuite.at. Sponsored-by: On-Waves ehf --- tests/msgb/msgb_test.c | 31 +++++++++++++++++++++++++++++++ tests/testsuite.at | 2 +- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/tests/msgb/msgb_test.c b/tests/msgb/msgb_test.c index 7592509..260aca5 100644 --- a/tests/msgb/msgb_test.c +++ b/tests/msgb/msgb_test.c @@ -23,6 +23,7 @@ #include <osmocom/core/logging.h> #include <osmocom/core/utils.h> #include <osmocom/core/msgb.h> +#include <setjmp.h> #include <errno.h> @@ -34,6 +35,36 @@ abort(); \ } +static jmp_buf jmp_env; +static int jmp_env_valid = 0; +static void osmo_panic_raise(const char *fmt, va_list args) +{ + /* + * The args can include pointer values which are not suitable for + * regression testing. So just write the (hopefully constant) format + * string to stdout and write the full message to stderr. + */ + printf("%s", fmt); + vfprintf(stderr, fmt, args); + if (!jmp_env_valid) + abort(); + longjmp(jmp_env, 1); +} + +/* Note that this does not nest */ +#define OSMO_PANIC_TRY(pE) (osmo_panic_try(pE, setjmp(jmp_env))) + +static int osmo_panic_try(volatile int *exception, int setjmp_result) +{ + jmp_env_valid = setjmp_result == 0; + *exception = setjmp_result; + + if (setjmp_result) + fprintf(stderr, "Exception caught: %d\n", setjmp_result); + + return *exception == 0; +} + static void test_msgb_api() { struct msgb *msg = msgb_alloc_headroom(4096, 128, "data"); diff --git a/tests/testsuite.at b/tests/testsuite.at index 12199e3..bd3fa1b 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -30,7 +30,7 @@ AT_CLEANUP AT_SETUP([msgb]) AT_KEYWORDS([msgb]) cat $abs_srcdir/msgb/msgb_test.ok > expout -AT_CHECK([$abs_top_builddir/tests/msgb/msgb_test], [0], [expout]) +AT_CHECK([$abs_top_builddir/tests/msgb/msgb_test], [0], [expout], [ignore]) AT_CLEANUP if ENABLE_MSGFILE -- 1.9.1
Sponsored-by: On-Waves ehf --- tests/msgb/msgb_test.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/msgb/msgb_test.ok | 12 +++++ 2 files changed, 149 insertions(+) diff --git a/tests/msgb/msgb_test.c b/tests/msgb/msgb_test.c index 260aca5..e6cb33e 100644 --- a/tests/msgb/msgb_test.c +++ b/tests/msgb/msgb_test.c @@ -121,6 +121,141 @@ static void test_msgb_api() msgb_free(msg); } +static void test_msgb_copy() +{ + struct msgb *msg = msgb_alloc_headroom(4096, 128, "data"); + struct msgb *msg2; + int i; + + printf("Testing msgb_copy\n"); + + msg->l1h = msgb_put(msg, 20); + msg->l2h = msgb_put(msg, 20); + msg->l3h = msgb_put(msg, 20); + msg->l4h = msgb_put(msg, 20); + + OSMO_ASSERT(msgb_length(msg) == 80); + for (i = 0; i < msgb_length(msg); i++) + msg->data[i] = (uint8_t)i; + + msg2 = msgb_copy(msg, "copy"); + + OSMO_ASSERT(msgb_length(msg) == msgb_length(msg2)); + OSMO_ASSERT(msgb_l1len(msg) == msgb_l1len(msg2)); + OSMO_ASSERT(msgb_l2len(msg) == msgb_l2len(msg2)); + OSMO_ASSERT(msgb_l3len(msg) == msgb_l3len(msg2)); + OSMO_ASSERT(msg->tail - msg->l4h == msg2->tail - msg2->l4h); + + for (i = 0; i < msgb_length(msg2); i++) + OSMO_ASSERT(msg2->data[i] == (uint8_t)i); + + printf("Src: %s\n", msgb_hexdump(msg)); + printf("Dst: %s\n", msgb_hexdump(msg)); + + msgb_free(msg); + msgb_free(msg2); +} + +static void test_msgb_resize_area() +{ + struct msgb *msg = msgb_alloc_headroom(4096, 128, "data"); + int rc; + volatile int e = 0; + int i, saved_i; + uint8_t *cptr, *old_l3h; + + osmo_set_panic_handler(osmo_panic_raise); + + rc = msgb_resize_area(msg, msg->data, 0, 0); + OSMO_ASSERT(rc >= 0); + + if (OSMO_PANIC_TRY(&e)) + msgb_resize_area(msg, NULL, 0, 0); + OSMO_ASSERT(e != 0); + + if (OSMO_PANIC_TRY(&e)) + msgb_resize_area(msg, NULL, (int)msg->data, 0); + OSMO_ASSERT(e != 0); + + if (OSMO_PANIC_TRY(&e)) + msgb_resize_area(msg, msg->data, 20, 0); + OSMO_ASSERT(e != 0); + + if (OSMO_PANIC_TRY(&e)) + msgb_resize_area(msg, msg->data, -1, 0); + OSMO_ASSERT(e != 0); + + if (OSMO_PANIC_TRY(&e)) + msgb_resize_area(msg, msg->data, 0, -1); + OSMO_ASSERT(e != 0); + + printf("Testing msgb_resize_area\n"); + + msg->l1h = msgb_put(msg, 20); + msg->l2h = msgb_put(msg, 20); + msg->l3h = msgb_put(msg, 20); + msg->l4h = msgb_put(msg, 20); + + for (i = 0; i < msgb_length(msg); i++) + msg->data[i] = (uint8_t)i; + + printf("Original: %s\n", msgb_hexdump(msg)); + + /* Extend area */ + saved_i = msg->l3h[0]; + old_l3h = msg->l3h; + + rc = msgb_resize_area(msg, msg->l2h, 20, 20 + 30); + + /* Reset the undefined part to allow printing the buffer to stdout */ + memset(old_l3h, 0, msg->l3h - old_l3h); + + printf("Extended: %s\n", msgb_hexdump(msg)); + + OSMO_ASSERT(rc >= 0); + OSMO_ASSERT(msgb_length(msg) == 80 + 30); + OSMO_ASSERT(msgb_l1len(msg) == 80 + 30); + OSMO_ASSERT(msgb_l2len(msg) == 60 + 30); + OSMO_ASSERT(msgb_l3len(msg) == 40); + OSMO_ASSERT(msg->tail - msg->l4h == 20); + + for (cptr = msgb_data(msg), i = 0; cptr < old_l3h; cptr++, i++) + OSMO_ASSERT(*cptr == (uint8_t)i); + + for (cptr = msg->l3h, i = saved_i; cptr < msg->tail; cptr++, i++) + OSMO_ASSERT(*cptr == (uint8_t)i); + + rc = msgb_resize_area(msg, msg->l2h, 50, 8000); + OSMO_ASSERT(rc == -1); + + /* Shrink area */ + saved_i = msg->l4h[0]; + OSMO_ASSERT(saved_i == (uint8_t)(msg->l4h[-1] + 1)); + + rc = msgb_resize_area(msg, msg->l3h, 20, 10); + + printf("Shrinked: %s\n", msgb_hexdump(msg)); + + OSMO_ASSERT(rc >= 0); + OSMO_ASSERT(msgb_length(msg) == 80 + 30 - 10); + OSMO_ASSERT(msgb_l1len(msg) == 80 + 30 - 10); + OSMO_ASSERT(msgb_l2len(msg) == 60 + 30 - 10); + OSMO_ASSERT(msgb_l3len(msg) == 40 - 10); + OSMO_ASSERT(msg->tail - msg->l4h == 20); + + OSMO_ASSERT(msg->l4h[0] != msg->l4h[-1] - 1); + + for (cptr = msg->l4h, i = saved_i; cptr < msg->tail; cptr++, i++) + OSMO_ASSERT(*cptr == (uint8_t)i); + + rc = msgb_resize_area(msg, msg->l2h, 50, 8000); + OSMO_ASSERT(rc == -1); + + msgb_free(msg); + + osmo_set_panic_handler(NULL); +} + static struct log_info info = {}; int main(int argc, char **argv) @@ -128,6 +263,8 @@ int main(int argc, char **argv) osmo_init_logging(&info); test_msgb_api(); + test_msgb_copy(); + test_msgb_resize_area(); printf("Success.\n"); diff --git a/tests/msgb/msgb_test.ok b/tests/msgb/msgb_test.ok index f8de0cd..4cb76a9 100644 --- a/tests/msgb/msgb_test.ok +++ b/tests/msgb/msgb_test.ok @@ -18,4 +18,16 @@ Test msgb_hexdump Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> 00 00 00 00 [L3]> (L4=tail+4) Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> (L3+8) 00 00 00 00 (L4=tail+4) Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> 00 00 00 00 (L3 out of range) (L4=tail+4) +Testing msgb_copy +Src: [L1]> 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 [L2]> 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 [L3]> 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37 38 39 3a 3b [L4]> 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f +Dst: [L1]> 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 [L2]> 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 [L3]> 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37 38 39 3a 3b [L4]> 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f +msgb(%p): Sub area is not fully contained in the msg data +msgb(%p): Sub area is not fully contained in the msg data +msgb(%p): Sub area is not fully contained in the msg data +msgb(%p): Negative sizes are not allowed +msgb(%p): Negative sizes are not allowed +Testing msgb_resize_area +Original: [L1]> 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 [L2]> 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 [L3]> 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37 38 39 3a 3b [L4]> 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f +Extended: [L1]> 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 [L2]> 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [L3]> 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37 38 39 3a 3b [L4]> 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f +Shrinked: [L1]> 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 [L2]> 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [L3]> 28 29 2a 2b 2c 2d 2e 2f 30 31 [L4]> 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f Success. -- 1.9.1
Currently msgb_trim only checks for len > data_len and returns -1 in that case, allowing the caller to fix it somehow. Using a negative length will always lead to a corrupt msgb, but this is not being checked. This commit adds a check for len < 0 and a conditional call to MSGB_ABORT. Sponsored-by: On-Waves ehf --- include/osmocom/core/msgb.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/osmocom/core/msgb.h b/include/osmocom/core/msgb.h index 9c99cad..b057caa 100644 --- a/include/osmocom/core/msgb.h +++ b/include/osmocom/core/msgb.h @@ -373,6 +373,8 @@ static inline void msgb_reserve(struct msgb *msg, int len) */ static inline int msgb_trim(struct msgb *msg, int len) { + if (len < 0) + MSGB_ABORT(msg, "Negative length is not allowed\n"); if (len > msg->data_len) return -1; -- 1.9.1
Include a test for msgb_trim. Sponsored-by: On-Waves ehf --- tests/msgb/msgb_test.c | 22 ++++++++++++++++++++++ tests/msgb/msgb_test.ok | 2 ++ 2 files changed, 24 insertions(+) diff --git a/tests/msgb/msgb_test.c b/tests/msgb/msgb_test.c index e6cb33e..8839a2e 100644 --- a/tests/msgb/msgb_test.c +++ b/tests/msgb/msgb_test.c @@ -121,6 +121,27 @@ static void test_msgb_api() msgb_free(msg); } +static void test_msgb_api_errors() +{ + struct msgb *msg = msgb_alloc_headroom(4096, 128, "data"); + volatile int e = 0; + int rc; + + printf("Testing the msgb API error handling\n"); + + osmo_set_panic_handler(osmo_panic_raise); + + if (OSMO_PANIC_TRY(&e)) + msgb_trim(msg, -1); + OSMO_ASSERT(e != 0); + + rc = msgb_trim(msg, 4096 + 500); + OSMO_ASSERT(rc == -1); + + msgb_free(msg); + osmo_set_panic_handler(NULL); +} + static void test_msgb_copy() { struct msgb *msg = msgb_alloc_headroom(4096, 128, "data"); @@ -263,6 +284,7 @@ int main(int argc, char **argv) osmo_init_logging(&info); test_msgb_api(); + test_msgb_api_errors(); test_msgb_copy(); test_msgb_resize_area(); diff --git a/tests/msgb/msgb_test.ok b/tests/msgb/msgb_test.ok index 4cb76a9..6603fe7 100644 --- a/tests/msgb/msgb_test.ok +++ b/tests/msgb/msgb_test.ok @@ -18,6 +18,8 @@ Test msgb_hexdump Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> 00 00 00 00 [L3]> (L4=tail+4) Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> (L3+8) 00 00 00 00 (L4=tail+4) Buffer: (L1=data-124) 00 00 00 00 00 00 00 00 [L2]> 00 00 00 00 (L3 out of range) (L4=tail+4) +Testing the msgb API error handling +msgb(%p): Negative length is not allowed Testing msgb_copy Src: [L1]> 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 [L2]> 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 [L3]> 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37 38 39 3a 3b [L4]> 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f Dst: [L1]> 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 [L2]> 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 [L3]> 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 36 37 38 39 3a 3b [L4]> 3c 3d 3e 3f 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f -- 1.9.1
participants (2)
-
Jacob Erlbeck -
Neels Hofmeyr