[Geany-devel] Bug on utils_string_replace()?

[Frank Lanitz]

On Sun, 25 Sep 2011 20:11:06 +0200
Colomban Wendling <lists.ban at 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/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.geany.org/pipermail/devel/attachments/20110925/194fcf98/attachment.pgp>