[netsurf-dev] Re: Pull request: CSS support for libsvgtiny

On Mon, 14 Oct 2024 at 17:37, Michael Orlitzky <michael@orlitzky.com> wrote:
>
> On Mon, 2023-11-20 at 12:27 -0500, Michael Orlitzky wrote:

> > https://gitweb.michael.orlitzky.com/?p=libsvgtiny.git;a=shortlog;h=refs/heads/libcss

> Any chance I can rekindle the interest in this? I'm still around to
> support the patchset and there are no known bugs.

Sorry it's taken so long. I've finally had another look.

https://gitweb.michael.orlitzky.com/?p=libsvgtiny.git;a=blob;f=src/svgtiny_css.c;h=96991ddfcfcfe4684b9411709c44367b4651fa92;hb=refs/heads/libcss#l1929

In `svgtiny_dom_user_data_handler` str is leaked. There is a comment
there is a comment there that says that there is no way to access the
`state->interned_userdata_key`.

There are two options here:

1. We could change the `dom_node_set_user_data` function to take
a `void *pw` alongside the `handler` callback, and change the hander
prototype to add an argument to pass back the user `pw`.
2. We can simply drop the string comparison here. The only user data
we add that uses that callback is the libcss_node_data. If we added
any other node data, we could add alongside a different callback
function.

There's also a comment saying:

> dom_node_set_user_data() always returns DOM_NO_ERR

However, it can return `DOM_NO_MEM_ERR` when malloc fails.
In general there are a lot of ignored return values in the patches.

https://gitweb.michael.orlitzky.com/?p=libsvgtiny.git;a=blob;f=src/svgtiny.c;h=9a6709a7e0069aad5290d43ebad3dcc9067d58a3;hb=refs/heads/libcss#l978

In `svgtiny_parse_svg`, it calls `svgtiny_parse_styles` which returns
the css_select_results. These are leaked on all the error paths in
`svgtiny_parse_svg` and I think at least one of the early returns is
missing a call to `svgtiny_cleanup_state_local`.

https://gitweb.michael.orlitzky.com/?p=libsvgtiny.git;a=blob;f=src/svgtiny.c;h=9a6709a7e0069aad5290d43ebad3dcc9067d58a3;hb=refs/heads/libcss#l812

In `svgtiny_parse_style_element`, the `svgtiny_create_stylesheet`
function is called, which returns a css_stylesheet pointer. Data is
added to the sheet and then the sheet is given to
`css_select_ctx_append_sheet`.

However, `css_select_ctx_append_sheet` does not take ownership
of the sheet (it takes it with the const qualifier).

So the sheet is leaked at the end of `svgtiny_parse_style_element`.

> Long term, I think using libcss is a better approach for parsing all of
> the CSS properties that now live inside the SVG spec.

Agreed!

Thanks for working on this, and sorry it's taken me so long to look at it!

Michael