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; }
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.
* 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
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
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; }
* 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);
We should unconditionally close 'fd' after the mmap() call, not just in the perror() branch.
Thanks,
Ingo
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; }
* 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
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");
* 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
This patch resolves resource leaks reported by cppcheck in lam.c. Specifically, the 'file_fd' and 'fd' were not closed in do_uring() and allocate_dsa_pasid() functions, respectively.
cppcheck output before this patch: tools/testing/selftests/x86/lam.c:685:3: error: Resource leak: file_fd [resourceLeak] tools/testing/selftests/x86/lam.c:693:3: error: Resource leak: file_fd [resourceLeak] tools/testing/selftests/x86/lam.c:1195:2: error: Resource leak: fd [resourceLeak]
cppcheck output after this patch: No resource leaks found
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");
linux-kselftest-mirror@lists.linaro.org
-
Ingo Molnar -
Malaya Kumar Rout -
malaya kumar rout
-
Peter Zijlstra