On Sat, 2024-10-19 at 16:28 +0100, Michael Drake wrote:
>
> Sorry it's taken so long. I've finally had another look.
No problem, thanks for taking the time!
> 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.
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.
> 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 defense, the documentation says it
always returns DOM_NO_ERR :)
This is now fixed.
> 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`.
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.
We were also leaking some DOM nodes that I've moved under the
"cleanup:" label.
> 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`.
Indeed. We can't destroy the sheet yet at the end of that function, so
I saw two options:
1. Keep track of the allocated sheets in the global "state" that we
pass around, so that they can eventually be cleaned up.
2. Just destroy whatever sheets are in the context object when it's
time to clean up the context.
Option 1 seems pointlessly wasteful, but option 2 only works if you
know that nobody else is going to be adding or removing sheets from the
context object (true for now). I implemented the second option, but it
feels a little dirty because you have to un-const the sheet references.
I'm happy to change it if you prefer the first option.
Unrelated to these changes, I also noticed that GCC's -fanalyzer finds
some problems with libsvgtiny, and building against libwapcaplet fails
with clang-19 due to a bunch of these:
src/svgtiny_css.c:121:9: error: use of GNU statement expression
extension from macro expansion [-Werror,-Wgnu-statement-expression-
from-macro-expansion]
121 | *abs = lwc_string_ref(rel);
| ^
/usr/include/libwapcaplet/libwapcaplet.h:137:30: note: expanded from
macro 'lwc_string_ref'
137 | #define lwc_string_ref(str) ({lwc_string *__lwc_s = (str);
assert(__lwc_s != NULL); __lwc_s->refcnt++; __lwc_s;})
| ^
No comments:
Post a Comment