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

let runc work with glibc lt 2.32 in go 1.22.x #4310

Closed
wants to merge 1 commit into from
Closed

let runc work with glibc lt 2.32 in go 1.22.x #4310

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions libcontainer/nsenter/nsenter_go122.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

The file was removed in:

So I guess we can just close this PR?

Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
//go:build go1.22
//go:build go1.22 && !go1.23

package nsenter

/*
// We know for sure that glibc has issues with pthread_self() when called from
// Go after nsenter has run. This is likely a more general problem with how we
// ignore the rules in signal-safety(7), and so it's possible musl will also
// have issues, but as this is just a hotfix let's only block glibc builds.
// In glibc versions older than 2.32 (before commit 4721f95058),
// pthread_getattr_np does not always initialize the `attr` argument,
// and when it fails, it results in a NULL pointer dereference in
// pthread_attr_destroy down the road. This has been fixed in go 1.22.4.
// We hack this to let runc can work with glibc < 2.32 in go 1.22.x,
// once runc doesn't support 1.22, we can remove this hack.
#cgo LDFLAGS: -Wl,--wrap=pthread_getattr_np
#include <features.h>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of overwriting go runtime internal functions. It is definitely something fishy, specially if a dependency suddenly starts doing that (as you were saying nsenter is for nomad in another thread). IMHO, a better course of action is to ask libc maintainers to backport the commit where they fix the missing init. Or ask users to use the latest go patch release, as that is already out.

However, as this should be short-lived (only until go 1.22.4 is more widely adopted, or glibc backports the fix), I'm not too stressed about it. But IMHO, if we do this, we should remove it quite soon after.

Copy link
Member Author

@lifubang lifubang Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I'm also not a fan of this hack for the entire go 1.22.x versions. But there is no other way for libct users(please see #4292 (comment)), if we are doing some error out in the code for golang lt 1.22.4, other libct users will get an error even in golang 1.22.4, except they also do some work in Makefile like what we are doing in runc , or runc doesn't do anything in the code(except Makefile) for golang lt 1.22.4 .

Copy link
Member Author

Choose a reason for hiding this comment

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

But IMHO, if we do this, we should remove it quite soon after.

Yes, I added a comment in the code: once runc doesn't support 1.22, we can remove this hack.

Copy link
Member

@rata rata Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeap, I understand that. IMHO we should either (a) do nothing and document in the function/package (and users using the unlucky combination will suffer), that is not so bad as the fixes are already backported and out (in go at least, I don't know in glibc, but the go fix is enough to not hit the bug); (b) do something like this.

What I don't like about doing something like this from the get-go, is that you can't revert it without breaking users. In contrast, we can always add it later if this generates bug reports.

IMHO, we should do (a).

But as I said, I'm not too stressed by this PR either.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO we should either (a) do nothing and document in the function/package

Yes, maybe this is another choice.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about it the whole day yesterday :) ended up doing it in #4292 today.

#ifdef __GLIBC__
# error "runc does not currently work properly with Go >=1.22. See <https://github.com/opencontainers/runc/issues/4233>."
#include <pthread.h>
int __real_pthread_getattr_np (pthread_t __th, pthread_attr_t *__attr);
int __wrap_pthread_getattr_np (pthread_t __th, pthread_attr_t *__attr) {
#if __GLIBC__ && defined(__GLIBC_PREREQ)
# if !__GLIBC_PREREQ(2, 32)
pthread_attr_init(__attr);
# endif
#endif
return __real_pthread_getattr_np(__th, __attr);
}
*/
import "C"
Loading