On 2025-03-31, Nam Cao namcao@linutronix.de wrote:

The buffer pointer "line" is not initialized. This pointer is passed to getline().

Ouch.

It can still work if the stack is zero-initialized, because getline() can work with a NULL pointer as buffer.

But this is obviously broken. This bug shows up while running the test on a riscv64 machine.

Fix it by properly initializing the pointer.

Fixes: 15858da53542 ("selftests: coredump: Add stackdump test") Signed-off-by: Nam Cao namcao@linutronix.de

---

tools/testing/selftests/coredump/stackdump_test.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/coredump/stackdump_test.c b/tools/testing/selftests/coredump/stackdump_test.c index 137b2364a082..1dc54e128586 100644 --- a/tools/testing/selftests/coredump/stackdump_test.c +++ b/tools/testing/selftests/coredump/stackdump_test.c @@ -100,6 +100,8 @@ TEST_F(coredump, stackdump) FILE *file; pid_t pid;

• line = NULL;

The syntax of getline(3) is quite interesting, since it allocates/reallocates/uses the lineptr as needed and possibly requires the application to free the data. I recommend moving the initialization down to the getline() call and also add the corresponding free().

Something like this:

diff --git a/tools/testing/selftests/coredump/stackdump_test.c b/tools/testing/selftests/coredump/stackdump_test.c index 137b2364a082..c23cf95c3f6d 100644 --- a/tools/testing/selftests/coredump/stackdump_test.c +++ b/tools/testing/selftests/coredump/stackdump_test.c @@ -138,10 +138,12 @@ TEST_F(coredump, stackdump) ASSERT_NE(file, NULL);

/* Step 4: Make sure all stack pointer values are non-zero */ + line = NULL; for (i = 0; -1 != getline(&line, &line_length, file); ++i) { stack = strtoull(line, NULL, 10); ASSERT_NE(stack, 0); } + free(line);

ASSERT_EQ(i, 1 + NUM_THREAD_SPAWN);

Because of how getline() works, technically your patch is good enough. But we should probably excercise more precision in the use of getline() so as to set a good example.

John Ogness

On 2025-03-31, Nam Cao namcao@linutronix.de wrote:

The test waits for coredump to finish by busy-waiting for the stackdump_values file to be created. The maximum wait time is 10 seconds.

This doesn't work for slow machine (qemu-system-riscv64), because coredump takes longer.

Switch to use waitpid().

Note that you are now assuming that returning from waitpid() means that:

1. the coredumping has completed

and

2. the STACKDUMP_FILE with all its contents are visible to the parent process

With this, the stack_values file doesn't need to be atomically written by coredump anymore, therefore simplify the stackdump script.

Fixes: 15858da53542 ("selftests: coredump: Add stackdump test") Signed-off-by: Nam Cao namcao@linutronix.de

---

tools/testing/selftests/coredump/stackdump | 6 +----- tools/testing/selftests/coredump/stackdump_test.c | 13 ++++++------- 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/coredump/stackdump b/tools/testing/selftests/coredump/stackdump index 96714ce42d12..ad487fd5ff15 100755 --- a/tools/testing/selftests/coredump/stackdump +++ b/tools/testing/selftests/coredump/stackdump @@ -4,11 +4,7 @@ CRASH_PROGRAM_ID=$1 STACKDUMP_FILE=$2 -TMP=$(mktemp)

for t in /proc/$CRASH_PROGRAM_ID/task/*; do tid=$(basename $t)

• cat /proc/$tid/stat | awk '{print $29}' >> $TMP

• cat /proc/$tid/stat | awk '{print $29}' >> $STACKDUMP_FILE

done

-mv $TMP $STACKDUMP_FILE

I would leave this as it was. Then the availability of STACKDUMP_FILE means the full contents are available.

diff --git a/tools/testing/selftests/coredump/stackdump_test.c b/tools/testing/selftests/coredump/stackdump_test.c index 1dc54e128586..733feaa0f895 100644 --- a/tools/testing/selftests/coredump/stackdump_test.c +++ b/tools/testing/selftests/coredump/stackdump_test.c @@ -96,7 +96,7 @@ TEST_F(coredump, stackdump) char *test_dir, *line; size_t line_length; char buf[PATH_MAX];

• int ret, i;

• int ret, i, status; FILE *file; pid_t pid;

@@ -131,12 +131,11 @@ TEST_F(coredump, stackdump) /* * Step 3: Wait for the stackdump script to write the stack pointers to the stackdump file */

• for (i = 0; i < 10; ++i) {

• file = fopen(STACKDUMP_FILE, "r");
  

• if (file)
  

• 	break;
  

• sleep(1);
  

• }

• waitpid(pid, &status, 0);
• ASSERT_TRUE(WIFSIGNALED(status));
• ASSERT_TRUE(WCOREDUMP(status));

Why not just put these 3 lines above the for-loop? So you would wait for the process to end, then go into the 20-second timeout loop waiting for STACKDUMP_FILE to show up.

John Ogness