| Message ID | 20220720205336.3628755-1-deso@posteo.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] samples/bpf: Don't use uninitialized cg2 variable | expand |
On Wed, Jul 20, 2022 at 08:53:36PM +0000, Daniel Müller wrote: > When the setup_cgroup_environment function returns false, we enter a > cleanup path that uses the cg2 variable, which, at this point, is not > initialized. > This change fixes the issue by introducing a new error label that does > not perform the close operation which uses said variable on this path. > > Signed-off-by: Daniel Müller <deso@posteo.net> > --- > samples/bpf/test_current_task_under_cgroup_user.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/samples/bpf/test_current_task_under_cgroup_user.c b/samples/bpf/test_current_task_under_cgroup_user.c > index ac251a..04a37f 100644 > --- a/samples/bpf/test_current_task_under_cgroup_user.c > +++ b/samples/bpf/test_current_task_under_cgroup_user.c > @@ -55,7 +55,7 @@ int main(int argc, char **argv) > } > > if (setup_cgroup_environment()) > - goto err; > + goto err_cgroup; > > cg2 = create_and_get_cgroup(CGROUP_PATH); > > @@ -104,6 +104,7 @@ int main(int argc, char **argv) > > err: > close(cg2); > +err_cgroup: > cleanup_cgroup_environment(); > > cleanup: > -- > 2.30.2 > I see that I may have jumped the gun here. We probably shouldn't be cleaning up the cgroup environment when the setup failed. Will update the patch. Thanks, Daniel
On Wed, Jul 20, 2022 at 09:03:16PM +0000, Daniel Müller wrote: > On Wed, Jul 20, 2022 at 08:53:36PM +0000, Daniel Müller wrote: > > When the setup_cgroup_environment function returns false, we enter a > > cleanup path that uses the cg2 variable, which, at this point, is not > > initialized. > > This change fixes the issue by introducing a new error label that does > > not perform the close operation which uses said variable on this path. > > > > Signed-off-by: Daniel Müller <deso@posteo.net> > > --- > > samples/bpf/test_current_task_under_cgroup_user.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/samples/bpf/test_current_task_under_cgroup_user.c b/samples/bpf/test_current_task_under_cgroup_user.c > > index ac251a..04a37f 100644 > > --- a/samples/bpf/test_current_task_under_cgroup_user.c > > +++ b/samples/bpf/test_current_task_under_cgroup_user.c > > @@ -55,7 +55,7 @@ int main(int argc, char **argv) > > } > > > > if (setup_cgroup_environment()) > > - goto err; > > + goto err_cgroup; > > > > cg2 = create_and_get_cgroup(CGROUP_PATH); > > > > @@ -104,6 +104,7 @@ int main(int argc, char **argv) > > > > err: > > close(cg2); > > +err_cgroup: > > cleanup_cgroup_environment(); > > > > cleanup: > > -- > > 2.30.2 > > > > I see that I may have jumped the gun here. We probably shouldn't be cleaning up > the cgroup environment when the setup failed. Will update the patch. ...but the semantics of setup_cgroup_environment are actually that it may leave half initialized state around on failure and cleanup_cgroup_environment is designed to deal with that. So we are good after all. Sorry for the noise. Thanks, Daniel
diff --git a/samples/bpf/test_current_task_under_cgroup_user.c b/samples/bpf/test_current_task_under_cgroup_user.c index ac251a..04a37f 100644 --- a/samples/bpf/test_current_task_under_cgroup_user.c +++ b/samples/bpf/test_current_task_under_cgroup_user.c @@ -55,7 +55,7 @@ int main(int argc, char **argv) } if (setup_cgroup_environment()) - goto err; + goto err_cgroup; cg2 = create_and_get_cgroup(CGROUP_PATH); @@ -104,6 +104,7 @@ int main(int argc, char **argv) err: close(cg2); +err_cgroup: cleanup_cgroup_environment(); cleanup:
When the setup_cgroup_environment function returns false, we enter a cleanup path that uses the cg2 variable, which, at this point, is not initialized. This change fixes the issue by introducing a new error label that does not perform the close operation which uses said variable on this path. Signed-off-by: Daniel Müller <deso@posteo.net> --- samples/bpf/test_current_task_under_cgroup_user.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)