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

On Mon, 21 Oct 2024 at 00:08, Michael Orlitzky <michael@orlitzky.com> wrote:
>
> On Sat, 2024-10-19 at 16:28 +0100, Michael Drake wrote:

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

> I fixed the dom_string leak in one commit, and then dropped the
> comparison entirely (option 2) in another. Unless you are dying to
> revamp this API, the first option seems like a lot of work for little
> benefit.

Agreed!

> > 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.
>
> I should know better, but in my defence, the documentation says it
> always returns DOM_NO_ERR :)

Whoops! :D

> This is now fixed.

https://gitweb.michael.orlitzky.com/?p=libsvgtiny.git;a=blobdiff;f=src/svgtiny_css.c;h=4856ac3f7bdefe4856f4bbf49415b6bf2c796415;hp=7d52ffa14df7cc11e808df741daa0f3f3db73826;hb=051ee66c3445a9071db366ed55cfd606cb579ec7;hpb=fe0f044f15984ebb02e017ab1b691178175c4b78

This fixes it for DOM_NO_MEM_ERR, but in general I think the
best approach is if a function returns an error code, we should
handle any non-successful value.

So it could be something like:

if (err == DOM_NO_MEM_ERR) {
return CSS_NOMEM;
} else if (err != DOM_NO_ERR) {
return CSS_INVALID;
}

Because if the implementation of a function ever changes, adding
a new type of error, we don't want to be checking every call site
in every project using the library!

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

> Yeah I don't know how I overlooked this. I've modified all of the error
> paths to clean up the select results. I don't think the missing call to
> svgtiny_cleanup_state_local() was my fault but I see no reason why it
> shouldn't be there, so I've added that too.

Thanks, that looks great now!

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

[snip]

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

> 2. Just destroy whatever sheets are in the context object when it's
> time to clean up the context.
>
> I implemented the second option, but it
> feels a little dirty because you have to un-const the sheet references.

I agree with doing it this way, and you've written a good comment to
explain the dirtiness.

However I'm a bit uncomfortable about leaving the selection
context with dangling sheet pointers, even if we are about to
destroy it. E.g. someone might change the context destroy
code to log stuff from its sheets for debug.

Also the error handling is not correct. If `css_select_ctx_get_sheet`
fails, it will try to destroy whatever is pointed to by sheet anyway,
potentially causing a double-free or free of some random uninitialised
pointer.

I'd rather do something like this (untested):

for (i = 0; i < n_sheets; i++) {
/* Remove them in reverse order. */
css_code = css_select_ctx_get_sheet(state.select_ctx,
n_sheets - 1 - i, &sheet);
if (css_code != CSS_OK) {
if (code == svgtiny_OK) {
code = svgtiny_LIBCSS_ERROR;
}
continue;
}
css_code = css_select_ctx_remove_sheet(state.select_ctx, sheet);
if (css_code != CSS_OK) {
if (code == svgtiny_OK) {
code = svgtiny_LIBCSS_ERROR;
}
continue;
}
css_code = css_stylesheet_destroy((css_stylesheet*)sheet);
if (css_code != CSS_OK) {
if (code == svgtiny_OK) {
code = svgtiny_LIBCSS_ERROR;
}
}
}

Note the added `continue`s.

> Unrelated to these changes, I also noticed that GCC's -fanalyzer finds
> some problems with libsvgtiny,

Oh, thanks for letting us know! I'll look into it soon.

> and building against libwapcaplet fails
> with clang-19 due to a bunch of these:

Yeah, I think we'll need to drop `-pedantic` from the library CFLAGS.
We don't use it with our other libraries.

For this commit:

https://gitweb.michael.orlitzky.com/?p=libsvgtiny.git;a=commit;h=29e40ed74de9f729755aa45459ff4f770b5be9ca

I don't think that's the right fix. The real issue is that
`_node_has_attribute_substring` has an early `return CSS_OK`
(substrlen == 0) which doesn't assign a result to `*match`.

Best regards,
Michael