[Geany-devel] Bug on utils_string_replace()?

[Colomban Wendling]

Le 25/09/2011 19:36, Thomas Martitz a écrit :
> Am 25.09.2011 19:20, schrieb Frank Lanitz:
>> Hi folks,
>>
>> I found a possible issue with changes done on utils_string_replace-*
>> functions i've added a workaround to geanysendmail with svn r2214.
>>
>> I was digging a bit and I guess I found the smoking gun. Unfortunately
>> I'm not 100 sure how to fix it without breaking anything.
>>
>> This is what I found:
>>
>> /* Replaces @len characters from offset @a pos.
>>   * len can be -1 for str->len.
>>   * returns: pos + strlen(replace). */
>> gint utils_string_replace(GString *str, gint pos, gint len, const
>> gchar *replace)
>> {
>>     g_string_erase(str, pos, len);
>>     g_string_insert(str, pos, replace);
>>
>>     return pos + strlen(replace);
>> }
>>
>> is not checking whether replace is != NULL so its failing with a
>> segfault. I was able to reproduce it on geanysendmail with this
>> backtrace:
>>
>>      #0  0x00000000004b9bd4 in utils_string_replace (str=0x31518c0,
>> pos=117, len=2,
>>          replace=0x0) at ../src/utils.c:1561
>>      #1  0x00000000004b9c6b in utils_string_replace_all
>> (haystack=0x31518c0,
>>          needle=0x7fffe7b3e68c "%r", replace=0x0) at ../src/utils.c:1588
>>      #2  0x00007fffe7b3d619 in send_as_attachment (menuitem=0xb692c0,
>> gdata=0x0)
>>          at ../geanysendmail/src/geanysendmail.c:152
>>      #3  0x00007ffff5d1de7e in g_closure_invoke () from
>> /usr/lib/libgobject-2.0.so.0
>>      #4  0x00007ffff5d2f8d7 in ?? () from /usr/lib/libgobject-2.0.so.0
>>      #5  0x00007ffff5d38d05 in g_signal_emit_valist ()
>>         from /usr/lib/libgobject-2.0.so.0
>>      #6  0x00007ffff5d38ed3 in g_signal_emit () from
>> /usr/lib/libgobject-2.0.so.0
>>
>> However. I'd add a g_return_val_if_fail (replace != NULL, pos); but not
>> sure whether this will break anything. Opinions?
>>
>> Cheers,
>> Frank
> 
> The caller should be fixed, passing NULL makes no sense at all.

Agreed (though one might argue about treating NULL as an alias of "",
but huh...).

However, I'm not against adding a g_return_val_if_fail().  This is meant
for defensive programming, while still showing it's not correct --
g_return*_if_fail() will emit a warning (or do nothing, depending on a
build-time setting).

Cheers,
Colomban