| Message ID | 20160229000810.GA28083@dub6 |
|---|---|
| State | Changes Requested |
| Headers | show |
> On 29 Feb 2016, at 01:08, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote: > > > > So we have the API talking about length "in octets" while the implementation > clearly employs septet length. > > Fixing the implementation to match the API doc would break binary > compatibility. I should probably fix the API doc to match that weird > implementation, but it nags and hurts a little. > > Any opinions? please fix the API comment. IIRC there are several fields in encoded messages where the length is passed as septets. E.g. for SMS the last 7 bits of an octet might not belong to the text anymore. Passing the number of septets avoids this issue. holger
diff --git a/src/gsm/gsm_utils.c b/src/gsm/gsm_utils.c index e8e452f..a9afc1a 100644 --- a/src/gsm/gsm_utils.c +++ b/src/gsm/gsm_utils.c @@ -184,8 +184,9 @@ return text - text_buf_begin; } -int gsm_7bit_decode_n(char *text, size_t n, const uint8_t *user_data, uint8_t septet_l) +int gsm_7bit_decode_n(char *text, size_t n, const uint8_t *user_data, uint8_t octet_l) { + uint8_t septet_l = ((uint16_t)octet_l * 8) / 7; return gsm_7bit_decode_n_hdr(text, n, user_data, septet_l, 0); }
Hi list, I just thought I had fixed gsm_7bit_decode_n(), since its API doc says * \param length The length of the input sequence (in octets). int gsm_7bit_decode_n([...], uint8_t length); but the implementation is int gsm_7bit_decode_n([...], uint8_t septet_l) and indeed the length parameter is handled as septet length. So, logically, I came up with this patch: (the cast to uint16_t makes 100% sure the calculation isn't truncated to uint8_t after the *8 multiplication -- I picked this up while hacking on 8bit microcontrollers. That truncation doesn't really happen on our 32bit/64bit machines, but if it did, the length would effectively be limited to 255/8 == 31 characters.) Then I noticed that callers actually do *8/7 before calling the function: parse_process_uss_req(): num_chars = (uss_req_data[6] * 8) / 7; /* Prevent a mobile-originated buffer-overrun! */ if (num_chars > MAX_LEN_USSD_STRING) num_chars = MAX_LEN_USSD_STRING; gsm_7bit_decode_n_ussd((char *)req->ussd_text, sizeof(req->ussd_text), &(uss_req_data[7]), num_chars); (This is gsm_7bit_decode_n_ussd(), its API doc says "see gsm_7bit_decode_n()") So we have the API talking about length "in octets" while the implementation clearly employs septet length. Fixing the implementation to match the API doc would break binary compatibility. I should probably fix the API doc to match that weird implementation, but it nags and hurts a little. Any opinions? ~Neels