Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix double-free of process user in crun exec #1538

Merged
merged 1 commit into from
Sep 2, 2024
Merged

Fix double-free of process user in crun exec #1538

merged 1 commit into from
Sep 2, 2024

Conversation

bduffany
Copy link
Contributor

@bduffany bduffany commented Sep 2, 2024 •
edited
Loading

Because the user is assigned by reference, the process struct cleanup in crun_command_exec was doubly freeing the process->user pointer, which was previously freed by the container struct cleanup in libcrun_container_exec_with_options.

Fixes #1537

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@bduffany
Fix double-free in crun exec
f72483a 
The process struct cleanup in crun_command_exec was doubly freeing the
process->user pointer, which was previously freed by the container struct
cleanup in libcrun_container_exec_with_options.

Signed-off-by: Brandon Duffany <brandon@buildbuddy.io>
@packit-as-a-service Packit-as-a-Service
Copy link

podman system tests failed. @containers/packit-build please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@packit-as-a-service Packit-as-a-Service
Copy link

podman system tests failed. @containers/packit-build please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@packit-as-a-service Packit-as-a-Service
Copy link

podman system tests failed. @containers/packit-build please check.

giuseppe
giuseppe reviewed Sep 2, 2024
View reviewed changes
src/libcrun/container.c
@@ -3620,7 +3644,7 @@ libcrun_container_exec_with_options (libcrun_context_t *context, const char *id,
process->apparmor_profile = xstrdup (container->container_def->process->apparmor_profile);

if (process->user == NULL && container->container_def->process->user)
process->user = container->container_def->process->user;
process->user = process_user_dup (container->container_def->process->user);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function owns container.

Could we just steal the reference to process->user?

process->user = container->container_def->process->user;
container->container_def->process->user = NULL;

What do you think?

Copy link
Contributor Author

@bduffany bduffany Sep 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that approach, but container gets passed to libcrun_join_process and exec_process_entrypoint which both have some complex logic, and I wasn't sure whether those functions need to read the container->container_def->process->user field. (Even if they aren't reading it today, they might try to read it in the future.)

Let me know if you think this is not much of a concern though - happy to do whatever you think is practical here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, better to not mess with the fields as it is difficult to track what would happen if we reset it.

I can't think of any better way (except changing libocispec to generate "clone" operations, but that is too much work for such a simple fix), so I'll just merge the current version

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened a new PR to use libocispec: #1554

@giuseppe giuseppe merged commit 9bc06d0 into containers:main Sep 2, 2024
32 of 56 checks passed
@giuseppe
Copy link
Member

giuseppe commented Sep 2, 2024

LGTM

@saschagrunert
Copy link
Member

saschagrunert commented Sep 3, 2024 •
edited
Loading

This PR regresses the supplemental groups critest: https://github.com/cri-o/cri-o/actions/runs/10680190170/job/29601173374

Summarizing 3 Failures:
  [FAIL] [k8s.io] Security Context SupplementalGroupsPolicy when SupplementalGroupsPolicy=Strict [It] even if the container's primary UID belongs to some groups in the image, runtime should not add SupplementalGroups to them
  sigs.k8s.io/cri-tools/pkg/validate/security_context_linux.go:737
  [FAIL] [k8s.io] Security Context bucket [It] runtime should support SupplementalGroups
  sigs.k8s.io/cri-tools/pkg/validate/security_context_linux.go:309
  [FAIL] [k8s.io] Security Context SupplementalGroupsPolicy when SupplementalGroupsPolicy=Merge (Default) [It] if the container's primary UID belongs to some groups in the image, runtime should add SupplementalGroups to them
  sigs.k8s.io/cri-tools/pkg/validate/security_context_linux.go:669

@saschagrunert saschagrunert mentioned this pull request Sep 3, 2024
Merged
@bduffany bduffany mentioned this pull request Sep 3, 2024
Closed
@giuseppe giuseppe mentioned this pull request Sep 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flouthoc flouthoc flouthoc left review comments

@giuseppe giuseppe Awaiting requested review from giuseppe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression in crun 1.16.1 causing occasional error "corrupted size vs. prev_size in fastbins"
4 participants
@bduffany @giuseppe @saschagrunert @flouthoc