[PATCH 1/1] src: out of bounds problem in smsutil

[PATCH 1/1] src: out of bounds problem in smsutil

[Jessica Nilsson]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

---

This one was exposed when wgmodem2.5 CBS was run with valgrind.

Best Regards,
Jessica Nilsson

 src/smsutil.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index 5524932..b3a1ba1 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -4628,7 +4628,7 @@ char *cbs_topic_ranges_to_string(GSList *ranges)
 	}
 
 	/* Space for ranges, commas and terminator null */
-	ret = g_new(char, len + nelem);
+	ret = g_new0(char, len + nelem + 1);
 
 	len = 0;
 
-- 
1.7.3.5

Re: [PATCH 1/1] src: out of bounds problem in smsutil

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

Hi Jessica,

On 02/16/2011 06:04 AM, Jessica Nilsson wrote:
> ---
> 
> This one was exposed when wgmodem2.5 CBS was run with valgrind.
> 
> Best Regards,
> Jessica Nilsson
> 

Can you post the actual error and the data this happened on?

>  src/smsutil.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/src/smsutil.c b/src/smsutil.c
> index 5524932..b3a1ba1 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -4628,7 +4628,7 @@ char *cbs_topic_ranges_to_string(GSList *ranges)
>  	}
>  
>  	/* Space for ranges, commas and terminator null */
> -	ret = g_new(char, len + nelem);
> +	ret = g_new0(char, len + nelem + 1);

I'm having trouble seeing how the old code was wrong.  nelem contains
the number of elements.  Since the last element does not end with a
comma, the use of nelem + 1 in g_new is not necessary.  sprintf takes
care of adding the terminating null, so using g_new0 is also less efficient.

Are you adding channels that are 5 digits long by any chance?

>  
>  	len = 0;
>  

Regards,
-Denis

Re: [PATCH 1/1] src: out of bounds problem in smsutil

[Andreas WESTIN]

[-- Attachment #1: Type: text/plain, Size: 4540 bytes --]



On 2011-02-16 16:25, Denis Kenzior wrote:
> Hi Jessica,
>
> On 02/16/2011 06:04 AM, Jessica Nilsson wrote:
>> ---
>>
>> This one was exposed when wgmodem2.5 CBS was run with valgrind.
>>
>> Best Regards,
>> Jessica Nilsson
>>
>
> Can you post the actual error and the data this happened on?
>
>>   src/smsutil.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/smsutil.c b/src/smsutil.c
>> index 5524932..b3a1ba1 100644
>> --- a/src/smsutil.c
>> +++ b/src/smsutil.c
>> @@ -4628,7 +4628,7 @@ char *cbs_topic_ranges_to_string(GSList *ranges)
>>   	}
>>
>>   	/* Space for ranges, commas and terminator null */
>> -	ret = g_new(char, len + nelem);
>> +	ret = g_new0(char, len + nelem + 1);
>
> I'm having trouble seeing how the old code was wrong.  nelem contains
> the number of elements.  Since the last element does not end with a
> comma, the use of nelem + 1 in g_new is not necessary.  sprintf takes
> care of adding the terminating null, so using g_new0 is also less efficient.
>
> Are you adding channels that are 5 digits long by any chance?

Valgrind complains that we step outside the allocated memory by 1 byte 
since we loop the string with:

while (*topics != '\0')

the allocated memory is the size of the string and any \0 ends up 
outside. At least that's my interpretation.

I saw it in isimodem/cbs.c in the patches sent in.

This is the output from valgrind:


==2450== Invalid read of size 1
==2450==    at 0x3427C: get_topics_len (cbs.c:112)
==2450==    by 0x347CF: isi_set_topics (cbs.c:223)
==2450==    by 0xE6343: cbs_set_powered (cbs.c:466)
==2450==    by 0xE6E4B: cbs_got_file_contents (cbs.c:752)
==2450==    by 0xE74BF: sim_cbmid_read_cb (cbs.c:889)
==2450==    by 0x103017: sim_fs_op_read_block_cb (simfs.c:388)
==2450==    by 0x375C3: isi_read_file_transparent_resp (sim.c:1013)
==2450==    by 0x19B5B: pending_remove_and_dispatch (modem.c:171)
==2450==    by 0x19C87: service_dispatch (modem.c:218)
==2450==    by 0x1A0D7: isi_callback (modem.c:334)
==2450==    by 0x48D1A4B: g_io_unix_dispatch (giounix.c:162)
==2450==    by 0x4898117: g_main_context_dispatch (gmain.c:1960)
==2450==  Address 0x4c031b2 is 0 bytes after a block of size 10 alloc'd
==2450==    at 0x4833F58: malloc (vg_replace_malloc.c:236)
==2450==    by 0x48D614F: g_malloc (gmem.c:132)
==2450==    by 0xCE887: cbs_topic_ranges_to_string (smsutil.c:4631)
==2450==    by 0xE5DC7: cbs_topics_to_str (cbs.c:329)
==2450==    by 0xE631F: cbs_set_powered (cbs.c:465)
==2450==    by 0xE6E4B: cbs_got_file_contents (cbs.c:752)
==2450==    by 0xE74BF: sim_cbmid_read_cb (cbs.c:889)
==2450==    by 0x103017: sim_fs_op_read_block_cb (simfs.c:388)
==2450==    by 0x375C3: isi_read_file_transparent_resp (sim.c:1013)
==2450==    by 0x19B5B: pending_remove_and_dispatch (modem.c:171)
==2450==    by 0x19C87: service_dispatch (modem.c:218)
==2450==    by 0x1A0D7: isi_callback (modem.c:334)
==2450==
==2450== Invalid read of size 1
==2450==    at 0x34588: parse_topics (cbs.c:153)
==2450==    by 0x34957: isi_set_topics (cbs.c:257)
==2450==    by 0xE6343: cbs_set_powered (cbs.c:466)
==2450==    by 0xE6E4B: cbs_got_file_contents (cbs.c:752)
==2450==    by 0xE74BF: sim_cbmid_read_cb (cbs.c:889)
==2450==    by 0x103017: sim_fs_op_read_block_cb (simfs.c:388)
==2450==    by 0x375C3: isi_read_file_transparent_resp (sim.c:1013)
==2450==    by 0x19B5B: pending_remove_and_dispatch (modem.c:171)
==2450==    by 0x19C87: service_dispatch (modem.c:218)
==2450==    by 0x1A0D7: isi_callback (modem.c:334)
==2450==    by 0x48D1A4B: g_io_unix_dispatch (giounix.c:162)
==2450==    by 0x4898117: g_main_context_dispatch (gmain.c:1960)
==2450==  Address 0x4c031b2 is 0 bytes after a block of size 10 alloc'd
==2450==    at 0x4833F58: malloc (vg_replace_malloc.c:236)
==2450==    by 0x48D614F: g_malloc (gmem.c:132)
==2450==    by 0xCE887: cbs_topic_ranges_to_string (smsutil.c:4631)
==2450==    by 0xE5DC7: cbs_topics_to_str (cbs.c:329)
==2450==    by 0xE631F: cbs_set_powered (cbs.c:465)
==2450==    by 0xE6E4B: cbs_got_file_contents (cbs.c:752)
==2450==    by 0xE74BF: sim_cbmid_read_cb (cbs.c:889)
==2450==    by 0x103017: sim_fs_op_read_block_cb (simfs.c:388)
==2450==    by 0x375C3: isi_read_file_transparent_resp (sim.c:1013)
==2450==    by 0x19B5B: pending_remove_and_dispatch (modem.c:171)
==2450==    by 0x19C87: service_dispatch (modem.c:218)
==2450==    by 0x1A0D7: isi_callback (modem.c:334)

Regards
Andreas

Re: [PATCH 1/1] src: out of bounds problem in smsutil

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

Hi Andreas,

>>>       }
>>>
>>>       /* Space for ranges, commas and terminator null */
>>> -    ret = g_new(char, len + nelem);
>>> +    ret = g_new0(char, len + nelem + 1);
>>
>> I'm having trouble seeing how the old code was wrong.  nelem contains
>> the number of elements.  Since the last element does not end with a
>> comma, the use of nelem + 1 in g_new is not necessary.  sprintf takes
>> care of adding the terminating null, so using g_new0 is also less
>> efficient.
>>
>> Are you adding channels that are 5 digits long by any chance?
> 
> Valgrind complains that we step outside the allocated memory by 1 byte
> since we loop the string with:
> 
> while (*topics != '\0')
> 
> the allocated memory is the size of the string and any \0 ends up
> outside. At least that's my interpretation.
> 

It might be your loop is actually going past the end, not that the
terminating NULL is not within bounds returned from
cbs_topic_ranges_to_string.  If the original code was wrong then we
should be seeing valgrind report errors on the cbs code used in
unit/test-sms.c.  I'm not seeing this at all.

Regards,
-Denis

Re: [PATCH 1/1] src: out of bounds problem in smsutil

[Andreas WESTIN]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

Hi,

>> Valgrind complains that we step outside the allocated memory by 1 byte
>> since we loop the string with:
>>
>> while (*topics != '\0')
>>
>> the allocated memory is the size of the string and any \0 ends up
>> outside. At least that's my interpretation.
>>
>
> It might be your loop is actually going past the end, not that the
> terminating NULL is not within bounds returned from
> cbs_topic_ranges_to_string.  If the original code was wrong then we
> should be seeing valgrind report errors on the cbs code used in
> unit/test-sms.c.  I'm not seeing this at all.

Yes you absolutely correct, we step it an extra time in one case.

Please disregard the patch.

Regards
Andreas