[PATCH] selftests/x86/lam: fix memory leak and resource leak in lam.c

List overview All Threads
Download

newer

older

[PATCH v3 0/2] memcg: Fix...

Re: [PATCH net-next]...

Malaya Kumar Rout

24 Mar 2025 24 Mar '25

12:47 p.m.

Static Analyis for bench_htab_mem.c with cppcheck:error tools/testing/selftests/x86/lam.c:585:3: error: Resource leak: file_fd [resourceLeak] tools/testing/selftests/x86/lam.c:593:3: error: Resource leak: file_fd [resourceLeak] tools/testing/selftests/x86/lam.c:600:3: error: Memory leak: fi [memleak] tools/testing/selftests/x86/lam.c:1066:2: error: Resource leak: fd [resourceLeak]

fix the issue by closing the file descriptors and releasing the allocated memory.

Signed-off-by: Malaya Kumar Rout malayarout91@gmail.com --- tools/testing/selftests/x86/lam.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c index 4d4a76532dc9..0b43b83ad142 100644 --- a/tools/testing/selftests/x86/lam.c +++ b/tools/testing/selftests/x86/lam.c @@ -581,24 +581,28 @@ int do_uring(unsigned long lam) if (file_fd < 0) return 1;

- if (fstat(file_fd, &st) < 0) + if (fstat(file_fd, &st) < 0) { + close(file_fd); return 1; - + } off_t file_sz = st.st_size;

int blocks = (int)(file_sz + URING_BLOCK_SZ - 1) / URING_BLOCK_SZ;

fi = malloc(sizeof(*fi) + sizeof(struct iovec) * blocks); - if (!fi) + if (!fi) { + close(file_fd); return 1; - + } fi->file_sz = file_sz; fi->file_fd = file_fd;

ring = malloc(sizeof(*ring)); - if (!ring) + if (!ring) { + close(file_fd); + free(fi); return 1; - + } memset(ring, 0, sizeof(struct io_ring));

if (setup_io_uring(ring)) @@ -1060,8 +1064,10 @@ void *allocate_dsa_pasid(void)

wq = mmap(NULL, 0x1000, PROT_WRITE, MAP_SHARED | MAP_POPULATE, fd, 0); - if (wq == MAP_FAILED) + if (wq == MAP_FAILED) { + close(fd); perror("mmap"); + }

return wq; }

-- 2.43.0

Show replies by date

Peter Zijlstra

24 Mar 24 Mar

12:51 p.m.

On Mon, Mar 24, 2025 at 06:17:50PM +0530, Malaya Kumar Rout wrote:

...

Static Analyis for bench_htab_mem.c with cppcheck:error tools/testing/selftests/x86/lam.c:585:3: error: Resource leak: file_fd [resourceLeak] tools/testing/selftests/x86/lam.c:593:3: error: Resource leak: file_fd [resourceLeak] tools/testing/selftests/x86/lam.c:600:3: error: Memory leak: fi [memleak] tools/testing/selftests/x86/lam.c:1066:2: error: Resource leak: fd [resourceLeak]

fix the issue by closing the file descriptors and releasing the allocated memory.

But but but, doesn't the program just exit on any of those 'errors' anyway?

That is, iirc this is a single shot program.

Ingo Molnar

25 Mar 25 Mar

9:29 a.m.

* Peter Zijlstra peterz@infradead.org wrote:

...

On Mon, Mar 24, 2025 at 06:17:50PM +0530, Malaya Kumar Rout wrote:

...
Static Analyis for bench_htab_mem.c with cppcheck:error tools/testing/selftests/x86/lam.c:585:3: error: Resource leak: file_fd [resourceLeak] tools/testing/selftests/x86/lam.c:593:3: error: Resource leak: file_fd [resourceLeak] tools/testing/selftests/x86/lam.c:600:3: error: Memory leak: fi [memleak] tools/testing/selftests/x86/lam.c:1066:2: error: Resource leak: fd [resourceLeak]

fix the issue by closing the file descriptors and releasing the allocated memory.

But but but, doesn't the program just exit on any of those 'errors' anyway?

That is, iirc this is a single shot program.

While that's true, still proper cleanup of resources is a good practice - and in more complicated tools it's useful to fix even these semi-false-positives, to make sure other warnings don't get missed.

Having said that, the error/cleanup control flow here doesn't look overly clean here to begin with, so I'd suggest fixing that (with goto labels or such) - which would fix the file_fd 'leak' as a happy side effect.

Thanks,

Ingo

malaya kumar rout

9:56 a.m.

I appreciate all the feedback and recommendations provided. We will incorporate the same.

Thanks & Regards, Malaya Kumar Rout

On Tue, Mar 25, 2025 at 2:59 PM Ingo Molnar mingo@kernel.org wrote:

...

Peter Zijlstra peterz@infradead.org wrote:

...
On Mon, Mar 24, 2025 at 06:17:50PM +0530, Malaya Kumar Rout wrote:

...
Static Analyis for bench_htab_mem.c with cppcheck:error tools/testing/selftests/x86/lam.c:585:3: error: Resource leak: file_fd [resourceLeak] tools/testing/selftests/x86/lam.c:593:3: error: Resource leak: file_fd [resourceLeak] tools/testing/selftests/x86/lam.c:600:3: error: Memory leak: fi [memleak] tools/testing/selftests/x86/lam.c:1066:2: error: Resource leak: fd [resourceLeak]

fix the issue by closing the file descriptors and releasing the allocated memory.

But but but, doesn't the program just exit on any of those 'errors' anyway?

That is, iirc this is a single shot program.

While that's true, still proper cleanup of resources is a good practice

and in more complicated tools it's useful to fix even these

semi-false-positives, to make sure other warnings don't get missed.

Having said that, the error/cleanup control flow here doesn't look overly clean here to begin with, so I'd suggest fixing that (with goto labels or such) - which would fix the file_fd 'leak' as a happy side effect.

Thanks,
    Ingo

Malaya Kumar Rout

7 Apr 7 Apr

10:44 a.m.

New subject: [PATCH v2] selftests/x86/lam: fix resource leak in do_uring() and allocate_dsa_pasid()

Exception branch returns without closing the file descriptors 'file_fd' and 'fd'

Signed-off-by: Malaya Kumar Rout malayarout91@gmail.com :wq --- tools/testing/selftests/x86/lam.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c index 18d736640ece..1d3631ce4b69 100644 --- a/tools/testing/selftests/x86/lam.c +++ b/tools/testing/selftests/x86/lam.c @@ -682,7 +682,7 @@ int do_uring(unsigned long lam) return 1;

if (fstat(file_fd, &st) < 0) - return 1; + goto cleanup;

off_t file_sz = st.st_size;

@@ -690,7 +690,7 @@ int do_uring(unsigned long lam)

fi = malloc(sizeof(*fi) + sizeof(struct iovec) * blocks); if (!fi) - return 1; + goto cleanup;

fi->file_sz = file_sz; fi->file_fd = file_fd; @@ -698,7 +698,7 @@ int do_uring(unsigned long lam) ring = malloc(sizeof(*ring)); if (!ring) { free(fi); - return 1; + goto cleanup; }

memset(ring, 0, sizeof(struct io_ring)); @@ -729,6 +729,8 @@ int do_uring(unsigned long lam) }

free(fi); +cleanup: + close(file_fd);

return ret; } @@ -1189,9 +1191,10 @@ void *allocate_dsa_pasid(void)

wq = mmap(NULL, 0x1000, PROT_WRITE, MAP_SHARED | MAP_POPULATE, fd, 0); - if (wq == MAP_FAILED) + if (wq == MAP_FAILED){ + close(fd); perror("mmap"); - + } return wq; }

-- 2.43.0

Ingo Molnar

6:20 p.m.

New subject: [PATCH v2] selftests/x86/lam: fix resource leak in do_uring() and allocate_dsa_pasid()

* Malaya Kumar Rout malayarout91@gmail.com wrote:

...

@@ -1189,9 +1191,10 @@ void *allocate_dsa_pasid(void) wq = mmap(NULL, 0x1000, PROT_WRITE, MAP_SHARED | MAP_POPULATE, fd, 0);

if (wq == MAP_FAILED)
if (wq == MAP_FAILED){
close(fd);
perror("mmap");

We should unconditionally close 'fd' after the mmap() call, not just in the perror() branch.

Thanks,

Ingo

Malaya Kumar Rout

7:34 p.m.

New subject: [PATCH v3] selftests/x86/lam: fix resource leak in do_uring() and allocate_dsa_pasid()

Exception branch returns without closing the file descriptors 'file_fd' and 'fd'

Signed-off-by: Malaya Kumar Rout malayarout91@gmail.com --- tools/testing/selftests/x86/lam.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c index 18d736640ece..88482d8112de 100644 --- a/tools/testing/selftests/x86/lam.c +++ b/tools/testing/selftests/x86/lam.c @@ -682,7 +682,7 @@ int do_uring(unsigned long lam) return 1;

if (fstat(file_fd, &st) < 0) - return 1; + goto cleanup;

off_t file_sz = st.st_size;

@@ -690,7 +690,7 @@ int do_uring(unsigned long lam)

fi = malloc(sizeof(*fi) + sizeof(struct iovec) * blocks); if (!fi) - return 1; + goto cleanup;

fi->file_sz = file_sz; fi->file_fd = file_fd; @@ -698,7 +698,7 @@ int do_uring(unsigned long lam) ring = malloc(sizeof(*ring)); if (!ring) { free(fi); - return 1; + goto cleanup; }

memset(ring, 0, sizeof(struct io_ring)); @@ -729,6 +729,8 @@ int do_uring(unsigned long lam) }

free(fi); +cleanup: + close(file_fd);

return ret; } @@ -1192,6 +1194,7 @@ void *allocate_dsa_pasid(void) if (wq == MAP_FAILED) perror("mmap");

+ close(fd); return wq; }

-- 2.43.0

Ingo Molnar

8 Apr 8 Apr

8:09 a.m.

New subject: [PATCH v3] selftests/x86/lam: fix resource leak in do_uring() and allocate_dsa_pasid()

* Malaya Kumar Rout malayarout91@gmail.com wrote:

...

Exception branch returns without closing the file descriptors 'file_fd' and 'fd'

Signed-off-by: Malaya Kumar Rout malayarout91@gmail.com

tools/testing/selftests/x86/lam.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c index 18d736640ece..88482d8112de 100644 --- a/tools/testing/selftests/x86/lam.c +++ b/tools/testing/selftests/x86/lam.c @@ -682,7 +682,7 @@ int do_uring(unsigned long lam) return 1; if (fstat(file_fd, &st) < 0)
return 1;
goto cleanup;
off_t file_sz = st.st_size; @@ -690,7 +690,7 @@ int do_uring(unsigned long lam) fi = malloc(sizeof(*fi) + sizeof(struct iovec) * blocks); if (!fi)
return 1;
goto cleanup;
fi->file_sz = file_sz; fi->file_fd = file_fd; @@ -698,7 +698,7 @@ int do_uring(unsigned long lam) ring = malloc(sizeof(*ring)); if (!ring) { free(fi);
return 1;
goto cleanup;
}
memset(ring, 0, sizeof(struct io_ring)); @@ -729,6 +729,8 @@ int do_uring(unsigned long lam) } free(fi); +cleanup:

close(file_fd);

return ret; } @@ -1192,6 +1194,7 @@ void *allocate_dsa_pasid(void) if (wq == MAP_FAILED) perror("mmap");

close(fd); return wq;

So in your previous patch you closed the file before the perror(), presumably so that file-leak detection in Valgrind or whatever tool you are using doesn't trigger.

But here it's done after the perror() call, why? It's perfectly fine to close the mapping fd straight after an mmap() call.

Finally, it would be nice to quote the before/after output of the leak detection tool you are using.

Thanks,

Ingo

Malaya Kumar Rout

6:52 p.m.

New subject: [PATCH RESEND x86-next v4] selftests/x86/lam: fix resource leak in do_uring() and allocate_dsa_pasid()

Exception branch returns without closing the file descriptors 'file_fd' and 'fd'

Signed-off-by: Malaya Kumar Rout malayarout91@gmail.com --- tools/testing/selftests/x86/lam.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c index 18d736640ece..0873b0e5f48b 100644 --- a/tools/testing/selftests/x86/lam.c +++ b/tools/testing/selftests/x86/lam.c @@ -682,7 +682,7 @@ int do_uring(unsigned long lam) return 1;

if (fstat(file_fd, &st) < 0) - return 1; + goto cleanup;

off_t file_sz = st.st_size;

@@ -690,7 +690,7 @@ int do_uring(unsigned long lam)

fi = malloc(sizeof(*fi) + sizeof(struct iovec) * blocks); if (!fi) - return 1; + goto cleanup;

fi->file_sz = file_sz; fi->file_fd = file_fd; @@ -698,7 +698,7 @@ int do_uring(unsigned long lam) ring = malloc(sizeof(*ring)); if (!ring) { free(fi); - return 1; + goto cleanup; }

memset(ring, 0, sizeof(struct io_ring)); @@ -729,6 +729,8 @@ int do_uring(unsigned long lam) }

free(fi); +cleanup: + close(file_fd);

return ret; } @@ -1189,6 +1191,7 @@ void *allocate_dsa_pasid(void)

wq = mmap(NULL, 0x1000, PROT_WRITE, MAP_SHARED | MAP_POPULATE, fd, 0); + close(fd); if (wq == MAP_FAILED) perror("mmap");

-- 2.43.0

Ingo Molnar

8:12 p.m.

New subject: [PATCH RESEND x86-next v4] selftests/x86/lam: fix resource leak in do_uring() and allocate_dsa_pasid()

* Malaya Kumar Rout malayarout91@gmail.com wrote:

...

Exception branch returns without closing the file descriptors 'file_fd' and 'fd'

Signed-off-by: Malaya Kumar Rout malayarout91@gmail.com

tools/testing/selftests/x86/lam.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)

Why have you ignored this request I made:

...

...
Finally, it would be nice to quote the before/after output of the leak detection tool you are using.

Thanks,

Ingo

days inactive

days old

linux-kselftest-mirror@lists.linaro.org

9 comments

participants

tags (0)

participants (4)

Ingo Molnar
Malaya Kumar Rout
malaya kumar rout
Peter Zijlstra