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
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.0However. 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.
Best regards.
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
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
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.
I did a short grep on sources of geany-plugins and found no further occurrences of utils_string_replace_all() in combination with an NULL-replacment. But I agree, it should be changed IMHO too. At least the api documentation should be clear here
Cheers, Frank
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
Le 28/09/2011 19:36, Frank Lanitz a écrit :
On Sun, 25 Sep 2011 20:11:06 +0200 [...]
Just want to bring it up again. Release is near so at least we should add a comment to api documentation about this.
Right. Fixed now in r5965, thanks.
If we want to deprecate the use of NULL here, I think we better do it after the release so plugin developers have a chance to fix their use until next-next release.
Cheers, Colomban
On 28/09/2011 20:56, Colomban Wendling wrote:
Le 28/09/2011 19:36, Frank Lanitz a écrit :
On Sun, 25 Sep 2011 20:11:06 +0200 [...]
Just want to bring it up again. Release is near so at least we should add a comment to api documentation about this.
Right. Fixed now in r5965, thanks.
If we want to deprecate the use of NULL here, I think we better do it after the release so plugin developers have a chance to fix their use until next-next release.
Thanks for fixing my mistake. I think there's no need to stop accepting null on utils_string_replace_all. It's bad to change plugin API behaviour unless the old use will get caught by the compiler.