Segmentation fault, if `GMarkupParser`'s `error` callback frees/clear `GError`

Current situation

The GMarkupParser [1] has an error-callback function, which receives a GErroras parameter.

Within the error-callback, the user must not call g_clear_error or g_error_free on the GError. Otherwise a (much later) call to g_markup_parse_context_unref / g_markup_parse_context_free will cause a segmentation fault.

[1] Simple XML Subset Parser: GLib Reference Manual

Problem

I find this hard to understand because

  • The GError example in [2] is suggesting to free GErrors, if received.
  • Freeing the error causes a problem much, much later on (very difficult to catch)

[2] Error Reporting: GLib Reference Manual

Solution

I suggest either of these solutions:

  1. Iff it’s a bug, fix it.
  2. Otherwise, update the documentation: [3] → Memberserror : Note, that the GError must not be freed inside the callback.

[3] Simple XML Subset Parser: GLib Reference Manual

That seems to be just a classical double free issue. The classical programming languages like C with manual memory management generally can not avoid double free issues and memory leaks. Languages with a Garbage Collector avoids these problems, and Rust with its borrow checker or Nim with ARC/ORC should also avoid it without any GC disadvantages like slow startup or delayed deallocation. Unfortunately for Nim we have still some issues when calling GTK.

The documentation already mentions that the error should not be freed:

  /* Called on error, including one set by other
   * methods in the vtable. The GError should not be freed.
   */

Where would you have looked for this instead, where is this note missing?

Classic, look everywhere in the docs, but not immediately on the spot! Perhaps this is solved as “RTFM properly”.

But “digging” a little…maybe it would help, if that part would be syntax highlighted, so the comments would stick out more(?):

struct GMarkupParser {
  /* Called for open tags <foo bar="baz"> */
  void (*start_element)  (GMarkupParseContext *context,
                          const gchar         *element_name,
                          const gchar        **attribute_names,
                          const gchar        **attribute_values,
                          gpointer             user_data,
                          GError             **error);

  /* Called for close tags </foo> */
  void (*end_element)    (GMarkupParseContext *context,
                          const gchar         *element_name,
                          gpointer             user_data,
                          GError             **error);

  /* Called for character data */
  /* text is not nul-terminated */
  void (*text)           (GMarkupParseContext *context,
                          const gchar         *text,
                          gsize                text_len,
                          gpointer             user_data,
                          GError             **error);

  /* Called for strings that should be re-saved verbatim in this same
   * position, but are not otherwise interpretable.  At the moment
   * this includes comments and processing instructions.
   */
  /* text is not nul-terminated. */
  void (*passthrough)    (GMarkupParseContext *context,
                          const gchar         *passthrough_text,
                          gsize                text_len,
                          gpointer             user_data,
                          GError             **error);

  /* Called on error, including one set by other
   * methods in the vtable. The GError should not be freed.
   */
  void (*error)          (GMarkupParseContext *context,
                          GError              *error,
                          gpointer             user_data);
};

For example, in devhelp's GError “Description” has syntax highlighting on:

Alternative idea:
Put the freeing part in the next paragraph, as it has nothing to do with the information that it’s “called on error”

  /* Called on error, including one set by other
   * methods in the vtable. 
   * The GError should not be freed.
   */
  void (*error)          (GMarkupParseContext *context,
                          GError              *error,
                          gpointer             user_data);

It should probably get ownership transfer annotations for all the callback parameters, so in this case it would be (transfer none) for the error.

The transfer none is the default; plus GLib uses gtk-doc, so you won’t get annotations for callbacks in structure fields. At most, we could replace the inlined function pointers with a typedef function pointer, which would be introspected and appear in the documentation.

Of course, there’s not much we can do about the styling, since gtk-doc is unmaintained.

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.