GNOME Bugzilla – Bug 614717
gtk_text_buffer_create_tag fails to return error on duplicate tag
Last modified: 2014-05-12 17:12:21 UTC
Created attachment 157788 [details] Demonstration of the bug resulting in a SEGV. When calling the above function with an existing tag, a warning is printed, but instead of NULL an invalid object (filled with zero, reference count zero) is returned. See gtktextbuffer.c: gtk_text_buffer_create_tag. It calls gtk_text_tag_table_add which has a void return value but may fail. Proposed solution: if, at function's end, before the final g_object_unref call, the refcount of the tag is just one, NULL should be returned (after calling g_object_unref of course) instead of the tag. Unfortunately I don't know how to determine the current refcount. If this isn't possible, gtk_text_tag_table_add should return a flag indicating success or error.
I would propose to make gtk_text_tag_table_add() return a boolean and then make gtk_text_buffer_create_tag() return NULL if the add call failed. All of this is just making an error case a little nicer, though. It is still a programming error, the caller needs to check (or otherwise ensure) that the name is not taken yet.
Created attachment 240585 [details] [review] Add boolean return value for gtk_text_tag_table_add()
Created attachment 240586 [details] [review] gtk_text_buffer_create_tag(): return NULL on failure
Review of attachment 240586 [details] [review]: Code looks fine. Commit message needs a body, and: ::: gtk/gtktextbuffer.c @@ +2482,3 @@ * of properties to set on the tag, as with g_object_set(). * + * Return value: (transfer none): a new tag, or %NULL on failure. I don't think change is right - if it's programmer error to duplicate a tag name (and this is what the docs for gtk_text_tag_table_add() say), then %NULL here is not possible for legitimate uses of the API. Adding "or %NULL on failure" here implies that you have to check - and you don't need to check.
OK, if the application or the plugin creates tags with a namespace, this should be enough to avoid conflicts. So this is a WONTFIX?
Created attachment 276381 [details] [review] Add boolean return value for gtk_text_tag_table_add() The user doesn't need to check the return value, because if FALSE is returned it is a programmer error. But it permits a nicer behavior for gtk_text_buffer_create_tag() in case of failure.
Created attachment 276382 [details] [review] gtk_text_buffer_create_tag(): returns NULL on failure Returns NULL in case of a duplicated tag name in the tag table. It is still a programmer error to duplicate a name, but if it happens the behavior is a little nicer (and hopefully doesn't crash).
Review of attachment 276381 [details] [review]: ok
Review of attachment 276382 [details] [review]: ok
Pushed to the master branch, thanks for the review.