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

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

[Anshuman]

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.

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.

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@gmail.com>
---
 tools/testing/selftests/mm/khugepaged.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c
index 10e8dedcb..a84fb87bd 100644
--- a/tools/testing/selftests/mm/khugepaged.c
+++ b/tools/testing/selftests/mm/khugepaged.c
@@ -1110,6 +1110,7 @@ static void parse_test_type(int argc, char **argv)
 {
 	int opt;
 	char *buf;
+	char *c;
 	const char *token;
 
 	while ((opt = getopt(argc, argv, "s:h")) != -1) {
@@ -1135,7 +1136,10 @@ static void parse_test_type(int argc, char **argv)
 	}
 
 	buf = strdup(argv[0]);
-	token = strsep(&buf, ":");
+	if (!buf)
+		ksft_exit_fail_msg("Insufficient memory\n");
+	c = buf;
+	token = strsep(&c, ":");
 
 	if (!strcmp(token, "all")) {
 		khugepaged_context =  &__khugepaged_context;
@@ -1148,26 +1152,27 @@ static void parse_test_type(int argc, char **argv)
 		usage();
 	}
 
-	if (!buf)
+	if (!c)
 		usage();
 
-	if (!strcmp(buf, "all")) {
+	if (!strcmp(c, "all")) {
 		read_only_file_ops =  &__read_only_file_ops;
 		read_write_file_read_ops =  &__read_write_file_read_ops;
 		read_write_file_write_ops =  &__read_write_file_write_ops;
 		anon_ops = &__anon_ops;
 		shmem_ops = &__shmem_ops;
-	} else if (!strcmp(buf, "anon")) {
+	} else if (!strcmp(c, "anon")) {
 		anon_ops = &__anon_ops;
-	} else if (!strcmp(buf, "file")) {
+	} else if (!strcmp(c, "file")) {
 		read_only_file_ops =  &__read_only_file_ops;
 		read_write_file_read_ops =  &__read_write_file_read_ops;
 		read_write_file_write_ops =  &__read_write_file_write_ops;
-	} else if (!strcmp(buf, "shmem")) {
+	} else if (!strcmp(c, "shmem")) {
 		shmem_ops = &__shmem_ops;
 	} else {
 		usage();
 	}
+	free(buf);
 
 	if (!read_only_file_ops && !read_write_file_read_ops &&
 	    !read_write_file_write_ops)
-- 
2.55.0

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

[David Hildenbrand (Arm)]

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@gmail.com>
> ---

[...]
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@kernel.org>
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@kernel.org>
---
 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

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@kernel.org> 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@gmail.com>
> > ---
>
> [...]
> 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@kernel.org>
> 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@kernel.org>
> ---
>  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
>

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

[David Hildenbrand (Arm)]

On 8/21/26 18:09, Anshuman Tewari wrote:
> 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.

I don't think we have to worry about other such argv[0] users. If they ever
appear, basic testing would reveal them.

-- 
Cheers,

David

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

[Anshuman Tewari]

Sounds good, no objection from me — happy to go with your version.

One small ask: since this fixes the issue from my original patch,
could you add a Reported-by: Anshuman Tewari
<anshumantewari123@gmail.com> when you post/apply it?

Thanks!

On Fri, 21 Aug 2026 at 21:48, David Hildenbrand (Arm) <david@kernel.org> wrote:
>
> On 8/21/26 18:09, Anshuman Tewari wrote:
> > 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.
>
> I don't think we have to worry about other such argv[0] users. If they ever
> appear, basic testing would reveal them.
>
> --
> Cheers,
>
> David

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

[David Hildenbrand (Arm)]

On 8/21/26 22:14, Anshuman Tewari wrote:
> Sounds good, no objection from me — happy to go with your version.
> 
> One small ask: since this fixes the issue from my original patch,
> could you add a Reported-by: Anshuman Tewari
> <anshumantewari123@gmail.com> when you post/apply it?

We typically use Reported-by: for actual bugs and must accompany it with a
Closes: link. That doesn't quite apply here because this isn't really
something that would ever trigger and really needs fixing. It's

Suggested-by or Debugged-by also don't apply here.

I will add

"
This patch is inspired by previous work from Anshuman Tewari [1].
    
Link: https://lore.kernel.org/r/20260821114416.12255-1-anshumantewari123@gmail.com [1]
"

If you want you can send your Reviewed-by or Tested-by tags once
I send it out officially.

-- 
Cheers,

David

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

[Anshuman Tewari]

That makes sense, thanks for explaining — I wasn't aware Reported-by
needed a Closes: link specifically. The inspired-by note with the
link is great, appreciate it.

I'll take a look once you send it out and add a Reviewed-by if it
looks good.

Anshuman

On Tue, 25 Aug 2026 at 16:33, David Hildenbrand (Arm) <david@kernel.org> wrote:
>
> On 8/21/26 22:14, Anshuman Tewari wrote:
> > Sounds good, no objection from me — happy to go with your version.
> >
> > One small ask: since this fixes the issue from my original patch,
> > could you add a Reported-by: Anshuman Tewari
> > <anshumantewari123@gmail.com> when you post/apply it?
>
> We typically use Reported-by: for actual bugs and must accompany it with a
> Closes: link. That doesn't quite apply here because this isn't really
> something that would ever trigger and really needs fixing. It's
>
> Suggested-by or Debugged-by also don't apply here.
>
> I will add
>
> "
> This patch is inspired by previous work from Anshuman Tewari [1].
>
> Link: https://lore.kernel.org/r/20260821114416.12255-1-anshumantewari123@gmail.com [1]
> "
>
> If you want you can send your Reviewed-by or Tested-by tags once
> I send it out officially.
>
> --
> Cheers,
>
> David

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

[David Hildenbrand (Arm)]

On 8/25/26 20:20, Anshuman Tewari wrote:
> That makes sense, thanks for explaining — I wasn't aware Reported-by
> needed a Closes: link specifically.
Yes, it's documented in Documentation/process/submitting-patches.rst and
checkpatch.pl warns if you don't follow the documented rules.

-- 
Cheers,

David