The memory allocated within a function should be released before the function return,otherwise memleak will occur.
Signed-off-by: zhujun2 zhujun2@cmss.chinamobile.com --- tools/testing/selftests/memfd/fuse_test.c | 3 +++ tools/testing/selftests/memfd/memfd_test.c | 10 ++++++++++ 2 files changed, 13 insertions(+)
diff --git a/tools/testing/selftests/memfd/fuse_test.c b/tools/testing/selftests/memfd/fuse_test.c index 93798c8c5d54..f302294a9001 100644 --- a/tools/testing/selftests/memfd/fuse_test.c +++ b/tools/testing/selftests/memfd/fuse_test.c @@ -205,6 +205,7 @@ static pid_t spawn_sealing_thread(void) stack = malloc(STACK_SIZE); if (!stack) { printf("malloc(STACK_SIZE) failed: %m\n"); + free(stack); abort(); }
@@ -214,9 +215,11 @@ static pid_t spawn_sealing_thread(void) NULL); if (pid < 0) { printf("clone() failed: %m\n"); + free(stack); abort(); }
+ free(stack); return pid; }
diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c index 3df008677239..917ffc210723 100644 --- a/tools/testing/selftests/memfd/memfd_test.c +++ b/tools/testing/selftests/memfd/memfd_test.c @@ -658,15 +658,18 @@ static void mfd_assert_grow_write(int fd) buf = malloc(mfd_def_size * 8); if (!buf) { printf("malloc(%zu) failed: %m\n", mfd_def_size * 8); + free(buf); abort(); }
l = pwrite(fd, buf, mfd_def_size * 8, 0); if (l != (mfd_def_size * 8)) { printf("pwrite() failed: %m\n"); + free(buf); abort(); }
+ free(buf); mfd_assert_size(fd, mfd_def_size * 8); }
@@ -682,14 +685,18 @@ static void mfd_fail_grow_write(int fd) buf = malloc(mfd_def_size * 8); if (!buf) { printf("malloc(%zu) failed: %m\n", mfd_def_size * 8); + free(buf); abort(); }
l = pwrite(fd, buf, mfd_def_size * 8, 0); if (l == (mfd_def_size * 8)) { printf("pwrite() didn't fail as expected\n"); + free(buf); abort(); } + + free(buf); }
static void mfd_assert_mode(int fd, int mode) @@ -771,15 +778,18 @@ static pid_t spawn_thread(unsigned int flags, int (*fn)(void *), void *arg) stack = malloc(STACK_SIZE); if (!stack) { printf("malloc(STACK_SIZE) failed: %m\n"); + free(stack); abort(); }
pid = clone(fn, stack + STACK_SIZE, SIGCHLD | flags, arg); if (pid < 0) { printf("clone() failed: %m\n"); + free(stack); abort(); }
+ free(stack); return pid; }
On 2023-11-15 00:45, zhujun2 wrote:
The memory allocated within a function should be released before the function return,otherwise memleak will occur.
[...]
--- a/tools/testing/selftests/memfd/fuse_test.c +++ b/tools/testing/selftests/memfd/fuse_test.c @@ -205,6 +205,7 @@ static pid_t spawn_sealing_thread(void) stack = malloc(STACK_SIZE); if (!stack) { printf("malloc(STACK_SIZE) failed: %m\n");
abort();free(stack);
Freeing process memory immediately before an abort(3) seems rather pointless, because the whole process is going away anyway. It's like tidying up your house right before it is scheduled to be bulldozed away to make room for a new highway. :-)
Thanks,
Mathieu
The memory allocated within a function should be released before the function return,otherwise memleak will occur.
Signed-off-by: zhujun2 zhujun2@cmss.chinamobile.com --- tools/testing/selftests/memfd/fuse_test.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/memfd/fuse_test.c b/tools/testing/selftests/memfd/fuse_test.c index 93798c8c5d54..fd934487c528 100644 --- a/tools/testing/selftests/memfd/fuse_test.c +++ b/tools/testing/selftests/memfd/fuse_test.c @@ -217,6 +217,7 @@ static pid_t spawn_sealing_thread(void) abort(); }
+ free(stack); return pid; }
On Mon, 20 Nov 2023 18:55:06 -0800 zhujun2 zhujun2@cmss.chinamobile.com wrote:
The memory allocated within a function should be released before the function return,otherwise memleak will occur.
...
--- a/tools/testing/selftests/memfd/fuse_test.c +++ b/tools/testing/selftests/memfd/fuse_test.c @@ -217,6 +217,7 @@ static pid_t spawn_sealing_thread(void) abort(); }
- free(stack); return pid;
}
We just freed a thread's stack while it is still running?
On 2023-11-21 18:43, Andrew Morton wrote:
On Mon, 20 Nov 2023 18:55:06 -0800 zhujun2 zhujun2@cmss.chinamobile.com wrote:
The memory allocated within a function should be released before the function return,otherwise memleak will occur.
...
--- a/tools/testing/selftests/memfd/fuse_test.c +++ b/tools/testing/selftests/memfd/fuse_test.c @@ -217,6 +217,7 @@ static pid_t spawn_sealing_thread(void) abort(); }
- free(stack); return pid; }
We just freed a thread's stack while it is still running?
Indeed, this patch is completely wrong. Freeing the stack memory should not happen before joining the thread which uses it.
This also means that this patch was probably not even runtime-tested, even less tested with tools like valgrind which should have caught this.
I would advise the patch author to test the changes before submitting a patch, especially considering the amount of tools out there which can be used to validate userspace programs.
Moreover, the changes done should take into consideration the behavior of the system calls they surround. Trying to "fix" a resource leak without understanding first the semantic of the system call using the resource is causing more problems than it solves.
Thanks,
Mathieu
linux-kselftest-mirror@lists.linaro.org