Le 25/09/2011 20:01, Colomban Wendling a écrit :
> 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).
Hum, I should have "blamed" the file before answering. Actually the
behaviour of utils_string_replace_all() seems to have changed, because
it seems it used to accept NULL (and I guess it was treated as "").
I think then we should restore the previous behaviour not to break the
plugin API -- e.g., and as Frank noticed, now all plugins passing NULL
will start to segfault while they worked seamlessly using 0.20.
Any opinion? (Nick?)
Cheers,
Colomban
_______________________________________________
Geany-devel mailing list
Geany-devel@uvena.de
https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
