This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.
Holger Hans Peter Freyther holger at freyther.deOn Mon, Aug 12, 2013 at 02:08:17PM +0200, Jacob Erlbeck wrote: > The 7bit<->8bit encoding/decoding functions didn't check whether > there is still enough space in the destination buffer. Therefore a > buffer size parameter ('n' like in snprintf) has been added to each > of the functions which leads to truncation if the the buffer is too > small. I think there has to be a comma and 'the the' > In addition, the return value of the decoding functions has been > changed to number of characters written (excluding \0), so this > value is always equal to strlen(decoded) if n>0. I commented that in the code, but maybe here as well. Maybe it is easier to forbid n==0 and add an OSMO_ASSERT? The argument for that is one can unconditionally printf the result? > +/* > + * The following functions write at most n octets to the destination buffer > + * ('decoded' or 'result') and return the number of octets written (always > + * <= n). > + */ Doxygen has the concept of groups[1], do you want to use that for these methods or do you just want to remove this comment? > msgb_put_u8(msg, ASN1_OCTET_STRING_TAG); > ussd_len_ptr = msgb_put(msg, 1); > data = msgb_put(msg, 0); > - gsm_7bit_encode_ussd(data, text, &len); > + gsm_7bit_encode_n_ussd(data, msgb_tailroom(msg), text, &len); i think there is no test coverage for all of these calls. > + gsm_7bit_decode_n_ussd(req->text, sizeof (req->text), no space. :) or just forbid this case? OSMO_ASSERT(n > 0)? This way no one can accidently do a printf on the text and crash? > - if (c == 0x1b && i + 1 < septet_l) { > + c8 = gsm_7bit_alphabet[0x7f + c7]; > + } else if (c7 == 0x1b && i + 1 < septet_l) { > next_is_ext = 1; > + continue; we try to avoid continue/break inside if statements. I have no counter proposal for the code flow though. :} > + nchars = (int)text - (int)text_buf_begin; I thought you wanted to use ptrdiff_t here? I think we should not truncate a 64bit point to potentially 32bit. > -int gsm_7bit_encode_oct(uint8_t *result, const char *data, int *octets) > +int gsm_7bit_encode_n(uint8_t *result, size_t n, const char *data, int *octets) > { > int y = 0; > + int o; > + int max_septets = n * 8 / 7; > > /* prepare for the worst case, every character expanding to two bytes */ > uint8_t *rdata = calloc(strlen(data) * 2, sizeof(uint8_t)); while we are here we could add the null check. and null terminate the result? > y = gsm_septet_encode(rdata, data); > - *octets = gsm_septets2octets(result, rdata, y, 0); > + > + if (y > max_septets) > + y = max_septets; when would this happen? does this break an assumption of our code? > +int gsm_7bit_encode_n_ussd(uint8_t *result, size_t n, const char *data, int *octets) > +{ > + int y; > + > + y = gsm_7bit_encode_n(result, n, data, octets); > + /* if last octet contains only one bit, add <CR> */ > + if (((y * 7) & 7) == 1) > + result[(*octets) - 1] |= ('\r' << 1); okay, y >= 1 => *octets >= 1. But shouldn't we increase y in this case too? > + .input = (const uint8_t *)concatenated_text, i think we tend to put a ' ' after the ')'. But thanks for fixing all the warnings GCC started to emit. holger [1] http://www.stack.nl/~dimitri/doxygen/manual/grouping.html