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) <[email protected]> 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 <[email protected]> > > --- > > [...] > 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)" <[email protected]> > 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) <[email protected]> > --- > 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 >
