On Sun, 25 Sep 2011 20:11:06 +0200 Colomban Wendling lists.ban@herbesfolles.org wrote:
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?)
Just want to bring it up again. Release is near so at least we should add a comment to api documentation about this.
Cheers, Frank