[PATCH] selftests/bpf:fix a resource leak in main()

[PATCH] selftests/bpf:fix a resource leak in main()

[Ma Ke]

The requested resources should be closed before return in main(), otherwise
 resource leak will occur. Add a check of cg_fd before close().

Fixes: 435f90a338ae ("selftests/bpf: add a test case for sock_ops perf-event notification")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 tools/testing/selftests/bpf/test_tcpnotify_user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_tcpnotify_user.c b/tools/testing/selftests/bpf/test_tcpnotify_user.c
index 595194453ff8..f81c60db586e 100644
--- a/tools/testing/selftests/bpf/test_tcpnotify_user.c
+++ b/tools/testing/selftests/bpf/test_tcpnotify_user.c
@@ -161,7 +161,8 @@ int main(int argc, char **argv)
 	error = 0;
 err:
 	bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
-	close(cg_fd);
+	if (cg_fd >= 0)
+		close(cg_fd);
 	cleanup_cgroup_environment();
 	perf_buffer__free(pb);
 	return error;
-- 
2.25.1

[PATCH] selftests/bpf:fix a resource leak in main()

[Ma Ke]

The requested resources should be closed before return in main(), otherwise
resource leak will occur. Add a check of cg_fd before close().

Fixes: 1f5fa9ab6e2e ("selftests/bpf: Test BPF_CGROUP_SYSCTL")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 tools/testing/selftests/bpf/test_sysctl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
index bcdbd27f22f0..242246ba8d1e 100644
--- a/tools/testing/selftests/bpf/test_sysctl.c
+++ b/tools/testing/selftests/bpf/test_sysctl.c
@@ -1627,7 +1627,8 @@ int main(int argc, char **argv)
 err:
 	err = -1;
 out:
-	close(cgfd);
+	if (cgfd >= 0)
+		close(cgfd);
 	cleanup_cgroup_environment();
 	return err;
 }
-- 
2.25.1

[PATCH] selftests/bpf:fix a resource leak in main()

[Ma Ke]

The requested resources should be closed before return in main(), otherwise
resource leak will occur. Add a check of cgfd before close().

Fixes: 5ecd8c22739b ("selftests/bpf: Selftest for bpf_skb_ancestor_cgroup_id")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 tools/testing/selftests/bpf/test_skb_cgroup_id_user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c b/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c
index ed518d075d1d..f84faddd72e6 100644
--- a/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c
+++ b/tools/testing/selftests/bpf/test_skb_cgroup_id_user.c
@@ -176,7 +176,8 @@ int main(int argc, char **argv)
 err:
 	err = -1;
 out:
-	close(cgfd);
+	if (cgfd >= 0)
+		close(cgfd);
 	cleanup_cgroup_environment();
 	printf("[%s]\n", err ? "FAIL" : "PASS");
 	return err;
-- 
2.25.1

Re: [PATCH] selftests/bpf:fix a resource leak in main()

[Stanislav Fomichev]

On 07/11, Ma Ke wrote:
> The requested resources should be closed before return in main(), otherwise
>  resource leak will occur. Add a check of cg_fd before close().
> 
> Fixes: 435f90a338ae ("selftests/bpf: add a test case for sock_ops perf-event notification")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
>  tools/testing/selftests/bpf/test_tcpnotify_user.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_tcpnotify_user.c b/tools/testing/selftests/bpf/test_tcpnotify_user.c
> index 595194453ff8..f81c60db586e 100644
> --- a/tools/testing/selftests/bpf/test_tcpnotify_user.c
> +++ b/tools/testing/selftests/bpf/test_tcpnotify_user.c
> @@ -161,7 +161,8 @@ int main(int argc, char **argv)
>  	error = 0;
>  err:
>  	bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
> -	close(cg_fd);

Worst case that happens here is that we call close(-1) which returns
EBADFS or something similar. Don't think there is any problem. This
applies to the rest of that patches that add an extra 'if' check.

Re: [PATCH] selftests/bpf:fix a resource leak in main()

[Markus Elfring]

> The requested resources should be closed before return in main(), otherwise
> resource leak will occur. Add a check of cg_fd before close().
>
> Fixes: 435f90a338ae ("selftests/bpf: add a test case for sock_ops perf-event notification")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>

Please reconsider such information once more.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc7#n398
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/researcher-guidelines.rst?h=v6.10-rc7#n5


How many source code analysis tools should be able to point out that the return value
from the call of a function like pthread_create() should get more development attention
(also for discussed test functions)?
https://elixir.bootlin.com/linux/v6.10-rc7/source/tools/testing/selftests/bpf/test_tcpnotify_user.c#L122

See also:
* https://cwe.mitre.org/data/definitions/252.html

* https://wiki.sei.cmu.edu/confluence/display/c/POS54-C.+Detect+and+handle+POSIX+library+errors


Regards,
Markus

Re: [PATCH] selftests/bpf:fix a resource leak in main()

[Stanislav Fomichev]

On 07/12, Markus Elfring wrote:
> > The requested resources should be closed before return in main(), otherwise
> > resource leak will occur. Add a check of cg_fd before close().
> >
> > Fixes: 435f90a338ae ("selftests/bpf: add a test case for sock_ops perf-event notification")
> > Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> 
> Please reconsider such information once more.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc7#n398
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/researcher-guidelines.rst?h=v6.10-rc7#n5
> 
> 
> How many source code analysis tools should be able to point out that the return value
> from the call of a function like pthread_create() should get more development attention
> (also for discussed test functions)?
> https://elixir.bootlin.com/linux/v6.10-rc7/source/tools/testing/selftests/bpf/test_tcpnotify_user.c#L122
> 
> See also:
> * https://cwe.mitre.org/data/definitions/252.html
> 
> * https://wiki.sei.cmu.edu/confluence/display/c/POS54-C.+Detect+and+handle+POSIX+library+errors

We are talking about testing binaries here. We don't have infinite
amount of time to polish them. If you really want to help, look at
the flakes on the bpf dashboard and help us weed them out.