11:04 p.m.
---
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
2:25 a.m.
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
2:50 a.m.
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
3:02 a.m.
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
3:13 a.m.
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
4232
days inactive
4232
days old
4 comments
3 participants
participants (3)
-
Andreas WESTIN -
Denis Kenzior -
Jessica Nilsson