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

[1.1] [hotfix] nsenter: refuse to build with Go 1.22 on glibc #4240

Closed
wants to merge 1 commit into from

Conversation

kolyshkin
Copy link
Contributor

This is a backport of #4234 to release-1.1 branch.


We will almost certainly need to eventually rework nsenter to:

  1. Figure out a way to make pthread_self() not break after nsenter runs (probably not possible, because the core issue is likely that we are ignoring the rules of signal-safety(7)); or
  2. Do an other re-exec of /proc/self/exe to execute the Go half of "runc init" -- after we've done the nsenter setup. This would reset all of the process state and ensure we have a clean glibc state for Go, but it would make runc slower...

For now, just block Go 1.22 builds to avoid having broken runcs floating around until we resolve the issue. It seems possible for musl to also have an issue, but it appears to work and so for now just block glibc builds.

Note that this will only block builds for anything that uses nsenter -- so users of our (internal) libcontainer libraries should be fine. Only users that are starting containers using nsenter to actually start containers will see the error (which is precisely what we want).

(cherry picked from commit e377e16)

We will almost certainly need to eventually rework nsenter to:

 1. Figure out a way to make pthread_self() not break after nsenter runs
    (probably not possible, because the core issue is likely that we are
    ignoring the rules of signal-safety(7)); or
 2. Do an other re-exec of /proc/self/exe to execute the Go half of
    "runc init" -- after we've done the nsenter setup. This would reset
    all of the process state and ensure we have a clean glibc state for
    Go, but it would make runc slower...

For now, just block Go 1.22 builds to avoid having broken runcs floating
around until we resolve the issue. It seems possible for musl to also
have an issue, but it appears to work and so for now just block glibc
builds.

Note that this will only block builds for anything that uses nsenter --
so users of our (internal) libcontainer libraries should be fine. Only
users that are starting containers using nsenter to actually start
containers will see the error (which is precisely what we want).

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
(cherry picked from commit e377e16)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin added the backport/1.1-pr A backport to 1.1.x release. label Apr 2, 2024
@kolyshkin kolyshkin added this to the 1.1.13 milestone Apr 2, 2024
@kolyshkin
Copy link
Contributor Author

CI failures in cross-i386 and cirrus are expected and are being fixed in #4231 / #4239. Once either one is merged, I'll rebase.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

NACK for backporting, as it has been working with some glibc versions, and we can't break users' environments that have been actually functional

@AkihiroSuda AkihiroSuda removed this from the 1.1.13 milestone Apr 3, 2024
@liggitt
Copy link
Contributor

liggitt commented Apr 3, 2024

Will this PR break things which are using go1.22 to link / build runc as a library and don't actually exercise the nsenter code at all? If so, that seems unnecessary and really problematic.

Asking another way, do we know that everything importing this package is running nsenter?

@kolyshkin
Copy link
Contributor Author

Will this PR break things which are using go1.22 to link / build runc as a library and don't actually exercise the nsenter code at all? If so, that seems unnecessary and really problematic.

Unless nsenter is explicitly vendored, there's no breakage. You can check by doing something like go mod why github.com/opencontainers/runc/libcontainer/nsenter or git grep github.com/opencontainers/runc/libcontainer/nsenter.

Asking another way, do we know that everything importing this package is running nsenter?

I think there is some software in the wild that actually uses nsenter, but to my best knowledge kubernetes does not.

@rata
Copy link
Contributor

rata commented Apr 4, 2024

Yeap, Kubernetes is not using nsenter.

@kolyshkin kolyshkin closed this Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.1-pr A backport to 1.1.x release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants