Created attachment 293949 [details] [review]
Don't crash with empty .devhelp* files

Review of attachment 293949 [details] [review]:

That looks good and works fine for me.

Review of attachment 293949 [details] [review]:

::: src/dh-parser.c
@@ +608,3 @@
+                        result = FALSE;
+                        goto exit;
+                }

This logic depends on whether the caller passed an error variable or not, which is not good (i.e. the call would behave differently if the caller passed a valid error or just NULL). Probably not a big deal, because this is not a library and we can make sure an error is always passed, but a safer way is to just avoid the assumption. So, if you really do want to check if end_parser() returned an error; use another variable, e.g. inner_error, and check for that one being set. And if set, use g_propagate_error (error, inner_error).

Review of attachment 293949 [details] [review]:

::: src/dh-parser.c
@@ +608,3 @@
+                        result = FALSE;
+                        goto exit;
+                }

This logic depends on whether the caller passed an error variable or not, which is not good (i.e. the call would behave differently if the caller passed a valid error or just NULL). Probably not a big deal, because this is not a library and we can make sure an error is always passed, but a safer way is to just avoid the assumption. So, if you really do want to check if end_parser() returned an error; use another variable, e.g. inner_error, and check for that one being set. And if set, use g_propagate_error (error, inner_error).

(In reply to comment #3)
> Review of attachment 293949 [details] [review]:

Thanks for the review, Aleksander.

> if you really do want to check if end_parser()
> returned an error; use another variable, e.g. inner_error, and check for that
> one being set. And if set, use g_propagate_error (error, inner_error).

Or, I could check the return value from g_markup_parse_context_end_parse. That would be simpler, but if you want then I could use an inner_error too.

By the way, the same problem exists for the g_markup_parse_context_parse call a few lines above. I will make a similar patch for it too.

Created attachment 295039 [details] [review]
Don't crash with empty .devhelp* files

Created attachment 295040 [details] [review]
Don't rely on the GError** passed by the caller

Debarshi, calling end_parse() LGTM, but is it also necessary to remove the parser_error_cb()? Is it one or the other, or do they both track different parsing issues?

(In reply to comment #8)
> Debarshi, calling end_parse() LGTM, but is it also necessary to remove the
> parser_error_cb()? Is it one or the other, or do they both track different
> parsing issues?

Yes, it is necessary to remove parser_error_cb.

It was broken and unnecesary because we should not be freeing the context inside it and dh_parser_free would have done it for us anyway. The error callback is called from set_error_literal which can happen at various points and invalidating the context can lead to bad things.

Somehow this wasn't an issue so far.

Now that we are calling g_markup_parse_context_end_parse, which calls parser_error_cb, the context becomes invalid for the second half of g_markup_parse_context_end_parse leading to a crash.

Created attachment 296012 [details] [review]
Don't crash with empty .devhelp* files

Thanks, Aleksander.