| Message ID | 20240711071018.2197252-1-make24@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/bpf:fix a resource leak in main() | expand |
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.
> 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
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.
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;
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(-)