GNOME Bugzilla – Bug 694613
gtksourceundomanagerdefault.c relies on undefined compiler behavior to perform comparison to INVALID marker
Last modified: 2013-02-25 17:50:16 UTC
INVALID is defined as a pointer to a literal string: http://git.gnome.org/browse/gtksourceview/tree/gtksourceview/gtksourceundomanagerdefault.c#n112 Subsequently, a pointer comparison is performed to match INVALID: http://git.gnome.org/browse/gtksourceview/tree/gtksourceview/gtksourceundomanagerdefault.c#n1197 The compiler isn't guaranteed to use the same pointer each time the same literal is used, so it'd be better if this worked differently. In Pidgin, we use a modified version of the file (we don't need the rest of gtksourceview, this was just a convenient example of how to implement undo for a GtkTextBuffer - thanks) and this potential issue was noticed by some static analysis that is being run on our code. I committed the following fix in our copy of the file to use a GQuark instead of a direct string literal: https://hg.pidgin.im/pidgin/main/rev/b495bcb42c5c
Thanks for pointing the issue. However I think there can be a problem with the GQuark solution too. The GQuark is a guint32, which is casted to a gpointer with GSIZE_TO_POINTER(). I think a malloc can return (with a very very low probability) the same value as INVALID. The root of the problem, for me, is that the 'modified_action' struct field is used for too many purposes, at a first glance at the code. It's maybe possible to modify the code so the INVALID pointer is not needed. If the INVALID is really needed, another solution is to add a boolean struct field to tell if 'modified_action' is valid or not.
Created attachment 237371 [details] [review] UndoManagerDefault: don't use the INVALID action An INVALID action was used to distinguish from a NULL action in the 'modified_action' struct field. But using NULL instead of INVALID works fine. The only place where an INVALID value would make a different result is in modified_changed_handler(): if (manager->priv->modified_action != NULL) { g_message ("%s: oops", G_STRLOC); return; } If the rest of the code is correct, it doesn't change anything.
Review of attachment 237371 [details] [review]: I'd say go for it. But please double test it.
(In reply to comment #1) > Thanks for pointing the issue. > > However I think there can be a problem with the GQuark solution too. The GQuark > is a guint32, which is casted to a gpointer with GSIZE_TO_POINTER(). > > I think a malloc can return (with a very very low probability) the same value > as INVALID. Hmm... I grabbed that code from the definition of the G_DEFINE_QUARK macro, but that seems to have changed since the version that I took the code from. You're probably right that it's possible (although extremely unlikely) for malloc to use that memory address. > > The root of the problem, for me, is that the 'modified_action' struct field is > used for too many purposes, at a first glance at the code. It's maybe possible > to modify the code so the INVALID pointer is not needed. If the INVALID is > really needed, another solution is to add a boolean struct field to tell if > 'modified_action' is valid or not. I'm not familiar enough with the code to evaluate your patch, but if it works correctly, it seems much simpler :) Once you decide what to do, I'll port your fix over to our fork of the file.
Comment on attachment 237371 [details] [review] UndoManagerDefault: don't use the INVALID action Double tested. I also tested with a low max-undo-levels, so the modified action gets out of bound, and an INVALID action was placed (with the previous code).