| Message ID | 1447753069-17466-1-git-send-email-jerlbeck@sysmocom.de |
|---|---|
| State | Superseded |
| Headers | show |
+/*! \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
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
Hi, I have reworked the patches (thanks go to Neels for comments), fixed some additional flaws, and improved the test cases. Jacob
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
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(+)