On Sun, 25 Sep 2011 20:11:06 +0200 Colomban Wendling <lists....@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 -- http://frank.uvena.de/en/
pgp2glIBC7qcJ.pgp
Description: PGP signature
_______________________________________________ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel