Re: [PATCH] selftests/mm: check strdup() and fix buf leak in parse_test_type()

[Anshuman Tewari]

Thanks David, agreed — strdup() is overkill here for a single-use
parse, and dropping it is the right call.

One small consideration on the approach: working on argv[0] in place
means strsep() will overwrite the : separator with '\0', so the
original string (e.g. "khugepaged:anon") ends up truncated after
parsing. Nothing today reads argv[0] again afterward, so it's safe as
things stand, but it does mean correctness quietly depends on that
staying true — a future change that logs argv[0], re-parses it, or
echoes it back in an error/usage message would get the mutated version
instead of what the user actually typed.

If we'd rather not rely on that invariant, an alternative that still
avoids strdup()/free() entirely: copy the type argument into a small
fixed-size stack buffer (with a bounds check against its length first)
and run strsep() on that copy instead of on argv[0] directly. Same
benefit as your version — no allocation, nothing to free, no
NULL-check needed — but argv[0] itself stays untouched.

Happy to write this up as a v2 if it seems worthwhile, or if you think
relying on "nothing downstream needs argv[0]" is fine as-is, I'm okay
going with your version too. Your call.


On Fri, 21 Aug 2026 at 19:49, David Hildenbrand (Arm) <david@xxxxxxxxxx> wrote:
>
> On 8/21/26 13:44, Anshuman wrote:
> > The return value of strdup() is never checked before being passed to
> > strsep() and strcmp(). If strdup() fails and returns NULL, strsep()
> > returns NULL as well, and the subsequent strcmp(NULL, "all") is
> > undefined behavior, likely causing a crash.
>
> In practice this is extraordinarily unlikely to ever fail. :)
>
> So I don't think we would ever experience this.
>
> >
> > Additionally, buf is never freed. strsep() advances the buf pointer
> > past the first token, so by the time buf would normally be freed,
> > the original pointer returned by strdup() has already been
> > overwritten and is no longer available.
>
> Given that parse_test_type() is called only once, nobody cares.
>
> >
> > Check strdup()'s return value and fail cleanly on allocation failure.
> > Keep a separate pointer to the original allocation so it can be
> > freed once buf is done being used, after all parsing has completed
> > successfully.
> >
> > Signed-off-by: Anshuman <anshumantewari123@xxxxxxxxx>
> > ---
>
> [...]
> That's too much churn for something that is irrelevant in practice and
> makes the code more complicated.
>
> So the following is better I think:
>
> From 36d525409eb16f56e667b2f979f2c6ef112ff235 Mon Sep 17 00:00:00 2001
> From: "David Hildenbrand (Arm)" <david@xxxxxxxxxx>
> Date: Fri, 21 Aug 2026 15:57:57 +0200
> Subject: [PATCH] selftests/mm: khugepaged: remove str_dup() usage
>
> We don't check str_dup() return value and never free it. While both
> things are irrelevant in practice, let's just work on argv[0] directly
> and avoid the str_dup().
>
> Nobody after us needs these parts of the argv[0] string.
>
> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> ---
>  tools/testing/selftests/mm/khugepaged.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/mm/khugepaged.c
> b/tools/testing/selftests/mm/khugepaged.c
> index d3a53673e1f9..7520fc0483ac 100644
> --- a/tools/testing/selftests/mm/khugepaged.c
> +++ b/tools/testing/selftests/mm/khugepaged.c
> @@ -1226,7 +1226,7 @@ static void parse_test_type(int argc, char **argv)
>                 return;
>         }
>  -      buf = strdup(argv[0]);
> +       buf = argv[0];
>         token = strsep(&buf, ":");
>         if (!strcmp(token, "all")) {
> --
> 2.43.0
>
>
> --
> Cheers,
>
> David
>