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.

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

Use raw syscalls to avoid sporadic hangs #2425

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

jprendes
Copy link
Contributor

The setgroups/setresgid/setresuid functions in nix/libc are not safe to call after clone/clone3.
These function use a blocking mechanism that synchronises across all threads in the process.
To do this libc (glibc/musl) needs to do some bookkeeping, which breaks on the new process after clone.
See the discussion in containerd/runwasi#347 for more details.

This PR worksaround that issue by using raw syscalls for setgroups/setresgid/setresuid.

This is generally not safe, as the raw syscalls only affect the calling thread and not the whole process.
It is safe to do so in this case because at the point when they are called in libcontainer the init process only has one thread.

@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2023

Codecov Report

Merging #2425 (32ffc4d) into main (8faff1c) will decrease coverage by 0.08%.
Report is 4 commits behind head on main.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2425      +/-   ##
==========================================
- Coverage   66.01%   65.93%   -0.08%     
==========================================
  Files         133      133              
  Lines       16832    16851      +19     
==========================================
  Hits        11111    11111              
- Misses       5721     5740      +19     

Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
@utam0k
Copy link
Member

utam0k commented Oct 11, 2023

@jprendes I appreciate your investigation and PR. Have you checked if this patch worked fine for runwasi?

@jprendes
Copy link
Contributor Author

@utam0k it seems to work. It is difficult to say as the original issue is sporadic.
But so far after a few runs I haven't gotten any failure with this change.

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

LGTM

@utam0k utam0k merged commit fd25afb into youki-dev:main Oct 12, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants