On Fri, 2025-08-08 at 21:45 +1000, Oliver Chang wrote: > (Apologies for the noise, I'm new to this. One more attempt to resend > this as text-only for those who have seen this email multiple times). > > Thank you for the feedback. The problem here is that there is a heap > buffer overflow found by fuzzing with the following testcase: > > `<sequence><foo/><text/></sequence>` > > This causes the `compute_seq_size(ctx_data->stack_head->data);` to be > called on `ctx_data->stack_head->data` that isn't a sequence type. > This patch adds some type checks to guard against that. > > I don't believe a regression test using valgrind would catch this -- > we used AddressSanitizer to detect this. I meant that there should be: - a test for SDP XML - valgrind run of that shows this specific memory being leaked (don't need ASAN for that...) - patch that fixes the leak with the section of the valgrind log We don't need the memory leak itself to be regression tested, don't think it's something that's easy to put in place right now. > While fixing this, we also discovered a memory leak in the error > handling path touched by the patch (` if > (g_markup_parse_context_parse(ctx, data, size, NULL) == FALSE) `), > which we included a fix for. > Would it be better if we separated out the heap buffer overflow fix > and the memory leak fix into 2 separate commits? Separate commits would be useful.
