src/svgtiny.c: deallocate stylesheets during finalisation

We allocate stylesheets in svgtiny_parse_style_element() and append
them to our global select_ctx, but the select_ctx retains only a const
reference to them and is not responsible for freeing their resources.
Instead, we have to do it during finalisation.

This is a little ugly because we have to de-const the sheet references
before we can destroy them. This relies on the non-local knowledge
that every sheet appended to the context does in fact originate in the
one function svgtiny_parse_style_element(). On the other hand, using
the context to keep track of these sheets saves us the trouble of
maintaining a duplicate array that would differ only in type
signature.

In summary:

  * The sheets are now removed from the context before destroying
    them. This avoids a potential issue where destroying the context
    itself (which we do afterwards) might try to access the sheets.

  * If we fail to obtain a pointer to a sheet, we stop processing that
    sheet. This should not happen, and if something extremely weird
    happens it can hide the problem, but that's still better than the
    segfault we'd get from trying to operate on an invalid pointer.

  * The sheets are removed/destroyed in reverse order, last-in
    first-out.

  * Extremely unlikely error paths call assert(). Production builds
    will still catch and propagate errors.

diff --git a/src/svgtiny.c b/src/svgtiny.c
index d87c568ed05ec0a2122a0289fdaac4a9fb74c271..824bbc6b4211f87ec0f3057975c13fa4332d5895 100644 (file)
--- a/src/svgtiny.c
+++ b/src/svgtiny.c
@@ -1279,6 +1279,11 @@ static svgtiny_code svgtiny_parse_style_element(dom_element *style,
        }
 
        dom_string_unref(data);
+
+       /* The only reference we retain to the newly-allocated "sheet"
+        * is via state.select_ctx->sheets. Eventually, we will
+        * destroy the sheet immediately prior to destroying the
+        * context. */
        return svgtiny_OK;
 }
 
@@ -1428,6 +1433,7 @@ initialise_parse_state(struct svgtiny_parse_state *state,
 
 static svgtiny_code finalise_parse_state(struct svgtiny_parse_state *state)
 {
+       svgtiny_code code = svgtiny_OK;
        css_error css_code;
        svgtiny_cleanup_state_local(state);
 
@@ -1441,12 +1447,69 @@ static svgtiny_code finalise_parse_state(struct svgtiny_parse_state *state)
                lwc_string_unref(state->interned_svg_xmlns);
        }
 
+       /* Clean up the select_ctx and any sheets that were appended
+        * to it along the way.  These sheets are allocated in
+        * svgtiny_parse_style_element(), and the only references we
+        * have to them are the ones in state->select_ctx->sheets.
+        * Those are const, but we are indeed responsible for the
+        * resources, so the un-const cast below is kosher.
+        *
+        * Some of these destructors can fail, but we want to loop
+        * through the entire list regardless. */
+       unsigned int i;
+       uint32_t n_sheets;
+       const css_stylesheet* sheet;
+       css_select_ctx_count_sheets(state->select_ctx, &n_sheets);
+       for (i = 0; i < n_sheets; i++) {
+               /* Note the use of (n_sheets - 1 - i) below to
+                * remove/destroy the sheets in reverse order, last-in
+                * first-out. */
+               css_code = css_select_ctx_get_sheet(state->select_ctx,
+                                                   n_sheets - 1 - i,
+                                                   &sheet);
+               if (css_code != CSS_OK) {
+                       /* The API just told us that there were n_sheets
+                        * valid sheets. So while we attempt to handle
+                        * the error gracefully in production builds,
+                        * this should never happen. */
+                       assert(0);
+                       code = svgtiny_LIBCSS_ERROR;
+                       /* If we didn't get a new sheet, don't try to
+                        * remove/destroy whatever happens to live at
+                        * the pointer. */
+                       continue;
+               }
+               /* Remove the sheets from the context as well, since
+                * there is no promise that css_select_ctx_destroy()
+                * will not try to access them. */
+               css_code = css_select_ctx_remove_sheet(state->select_ctx, sheet);
+               if (css_code != CSS_OK) {
+                       /* Same as the assert() above. This sheet arose
+                        * from a call to css_select_ctx_get_sheet() a
+                        * moment ago, it should be valid! */
+                       assert(0);
+                       code = svgtiny_LIBCSS_ERROR;
+                       /* Don't try to destroy the sheet if removal
+                        * failed, because that would suggest that our
+                        * implied contract (that nobody else modifies
+                        * our context) has been violated. */
+                       continue;
+               }
+               css_code = css_stylesheet_destroy((css_stylesheet*)sheet);
+               if (css_code != CSS_OK) {
+                       /* Once more, "sheet" should have been valid
+                        * and un-destroyed. */
+                       assert(0);
+                       code = svgtiny_LIBCSS_ERROR;
+               }
+       }
+
        css_code = css_select_ctx_destroy(state->select_ctx);
        if (css_code != CSS_OK) {
-               return svgtiny_LIBCSS_ERROR;
+               code = svgtiny_LIBCSS_ERROR;
        }
 
-       return svgtiny_OK;
+       return code;
 }