let runc work with glibc lt 2.32 in go 1.22.x #4310
Conversation
|
@lifubang have you checked it locally? It doesn't work for me: [kir@kir-tp1 runc]$ GO=go1.22.3 make all
rm -f libcontainer/dmz/binary/runc-dmz
go1.22.3 generate -tags "seccomp urfave_cli_no_docs " ./libcontainer/dmz
make[1]: Entering directory '/home/kir/git/runc/libcontainer/dmz'
gcc -fno-asynchronous-unwind-tables -fno-ident -s -Os -nostdlib -lgcc -static -o binary/runc-dmz _dmz.c
strip -gs binary/runc-dmz
make[1]: Leaving directory '/home/kir/git/runc/libcontainer/dmz'
go1.22.3 build -trimpath "-buildmode=pie" -tags "seccomp urfave_cli_no_docs " -ldflags "-X main.gitCommit=v1.2.0-rc.1-85-gbef89d46 -X main.version=1.2.0-rc.1+dev " -o runc .
# github.com/opencontainers/runc
/home/kir/sdk/go1.22.3/pkg/tool/linux_amd64/link: running gcc failed: exit status 1
/usr/bin/ld: /tmp/go-link-4194289539/000025.o: in function `x_cgo_getstackbound':
/_/GOROOT/src/runtime/cgo/gcc_stack_unix.c:25:(.text+0x1a): undefined reference to `__wrap_pthread_getattr_np'
collect2: error: ld returned 1 exit status
make: *** [Makefile:78: runc-bin] Error 1 |
|
I checked it with glibc 2.31 in go 1.22.3, but didn't check glibc gt 2.31. |
lifubang
force-pushed
the
fix-go-lt-1.22.4
branch
from
bef89d4
to
9b1298d
Compare
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. Signed-off-by: lifubang <[email protected]>
lifubang
force-pushed
the
fix-go-lt-1.22.4
branch
from
9b1298d
to
3c2ca11
Compare
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 .
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
IMHO we should either (a) do nothing and document in the function/package
Yes, maybe this is another choice.
There was a problem hiding this comment.
I was thinking about it the whole day yesterday :) ended up doing it in #4292 today.
There was a problem hiding this comment.
The file was removed in:
So I guess we can just close this PR?
For some libct users, it's hard to let them deal with the bug before go 1.22.4 with glibc version < 2.32, so let's do a hack to let runc can work for the entire go 1.22 versions.