On 2023/9/1 13:18, David Gow wrote:
> On Thu, 31 Aug 2023 at 15:17, 'Jinjie Ruan' via KUnit Development
> <kunit-dev@xxxxxxxxxxxxxxxx> wrote:
>>
>> Take the last kfree(parsed_filters) and add it to be the first. Take
>> the first kfree(copy) and add it to be the last. The Best practice is to
>> return these errors reversely.
>>
>> Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")
>> Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()")
>> Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
>> ---
>
> Agreed, these should be freed in reverse order.
>
> Would it make sense to initialise 'copy' to NULL, and free it if (copy
> != NULL), rather than if (*err)? As mentioned in the previous patch, I
> think that'd be more correct.
There is a problem because the normal return also go through the all err
paths but 'copy' is not NULL and should not be freed.
>
> We could also have several labels which target only the things which
> actually have been allocated so far.
That's a good idea!
>
> Thoughts?
> -- David
>
>> lib/kunit/executor.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
>> index 7654c09c1ab1..da9444d22711 100644
>> --- a/lib/kunit/executor.c
>> +++ b/lib/kunit/executor.c
>> @@ -229,16 +229,16 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>> filtered.end = copy;
>>
>> err:
>> - if (*err)
>> - kfree(copy);
>> + if (filter_count)
>> + kfree(parsed_filters);
>>
>> if (filter_glob) {
>> kfree(parsed_glob.suite_glob);
>> kfree(parsed_glob.test_glob);
>> }
>
> I think this might also be potentially broken. If
> parsed_glob.{suite,test}_glob are not successfully allocated,
> filter_glob will still be set, and we'll kfree() something invalid.
> Should we also init parsed_glob.* to NULL, and free them if non-NULL,
> rather than relying on the presence of filter_glob?
if not successsfully allocated, it will be NULL and kfree NULL is ok.
>
>
>
>>
>> - if (filter_count)
>> - kfree(parsed_filters);
>> + if (*err)
>> + kfree(copy);
>>
>> return filtered;
>> }
>> --
>> 2.34.1
>>
>> --
>> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@xxxxxxxxxxxxxxxx.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230831071655.2907683-5-ruanjinjie%40huawei.com.
