[PATCH 1/2] sms: Added result buffer size parameter to 7bit conv funs

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.de
Mon Aug 12 12:42:29 UTC 2013


On 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




More information about the OpenBSC mailing list