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

[DarkTrick]

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] → Members → error : Note, that the GError must not be freed inside the callback.

[3] Simple XML Subset Parser: GLib Reference Manual

[StefanSalewski]

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.

[sdroege (Sebastian Dröge)]

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?

[DarkTrick]

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

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);

[sdroege (Sebastian Dröge)]

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

[ebassi (Emmanuele Bassi)]

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.