[go] runtime: fixes using go .so and .a libraries with musl/uclinux
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
runtime: fixes using go .so and .a libraries with musl/uclinux
This works around the segfault seen when compiling a shared archive and linking it on a musl/uclinux system.
The issue is that the go runtime is doing some magic to extract the program arguments even though the go side is compiled as a library.
By including my changes the glibc behavior remains unchanged, and for musl/uclinux systems we simply do not get any arguments if we are linked as a shared library.
I've attempted to get these changes included in the past and have been manually patching my go releases for alpine. I believe the issue raised previously was that in theory we could parse the arguments from /proc but honestly I don't have time to mess with that and don't see the need for a library to be aware of argc/argv unless someone here thinks otherwise.
Change diff
diff --git a/src/cmd/cgo/internal/testcarchive/testdata/libgo/libgo.go b/src/cmd/cgo/internal/testcarchive/testdata/libgo/libgo.go
index 37b30c1..3a0684e 100644
--- a/src/cmd/cgo/internal/testcarchive/testdata/libgo/libgo.go
+++ b/src/cmd/cgo/internal/testcarchive/testdata/libgo/libgo.go
@@ -46,7 +46,11 @@
//export CheckArgs
func CheckArgs() {
- if len(os.Args) != 3 || os.Args[1] != "arg1" || os.Args[2] != "arg2" {
+ // Dynamic linkers which supply the library initialization functions with the
+ // main program's argc / argc should have 3 args here, else they should have
+ // none.
+ valid := (len(os.Args) == 3 && os.Args[1] == "arg1" && os.Args[2] == "arg2") || (len(os.Args) == 0)
+ if !valid {
fmt.Printf("CheckArgs: want [_, arg1, arg2], got: %v\n", os.Args)
os.Exit(2)
}
diff --git a/src/runtime/cgo.go b/src/runtime/cgo.go
index 40c8c74..65c4e76 100644
--- a/src/runtime/cgo.go
+++ b/src/runtime/cgo.go
@@ -12,6 +12,7 @@
//go:linkname _cgo_init _cgo_init
//go:linkname _cgo_thread_start _cgo_thread_start
+//go:linkname _cgo_sys_lib_args_valid _cgo_sys_lib_args_valid
//go:linkname _cgo_sys_thread_create _cgo_sys_thread_create
//go:linkname _cgo_notify_runtime_init_done _cgo_notify_runtime_init_done
//go:linkname _cgo_callers _cgo_callers
@@ -24,6 +25,7 @@
var (
_cgo_init unsafe.Pointer
_cgo_thread_start unsafe.Pointer
+ _cgo_sys_lib_args_valid unsafe.Pointer
_cgo_sys_thread_create unsafe.Pointer
_cgo_notify_runtime_init_done unsafe.Pointer
_cgo_callers unsafe.Pointer
diff --git a/src/runtime/cgo/callbacks.go b/src/runtime/cgo/callbacks.go
index 3c246a8..6a53f2b 100644
--- a/src/runtime/cgo/callbacks.go
+++ b/src/runtime/cgo/callbacks.go
@@ -59,6 +59,14 @@
var x_cgo_thread_start byte
var _cgo_thread_start = &x_cgo_thread_start
+// Determines if the argc / argv passed to the library initialization functions
+// are valid.
+//go:cgo_import_static x_cgo_sys_lib_args_valid
+//go:linkname x_cgo_sys_lib_args_valid x_cgo_sys_lib_args_valid
+//go:linkname _cgo_sys_lib_args_valid _cgo_sys_lib_args_valid
+var x_cgo_sys_lib_args_valid byte
+var _cgo_sys_lib_args_valid = &x_cgo_sys_lib_args_valid
+
// Creates a new system thread without updating any Go state.
//
// This method is invoked during shared library loading to create a new OS
diff --git a/src/runtime/cgo/gcc_libinit.c b/src/runtime/cgo/gcc_libinit.c
index 33a9ff9..97dc7b4 100644
--- a/src/runtime/cgo/gcc_libinit.c
+++ b/src/runtime/cgo/gcc_libinit.c
@@ -13,6 +13,7 @@
#include <pthread.h>
#include <errno.h>
+#include <limits.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h> // strerror
@@ -35,6 +36,20 @@
// The context function, used when tracing back C calls into Go.
static void (*cgo_context_function)(struct context_arg*);
+// Detect if using glibc
+int
+x_cgo_sys_lib_args_valid()
+{
+ // The ELF gABI doesn't require an argc / argv to be passed to the functions
+ // in the DT_INIT_ARRAY. However, glibc always does.
+ // Ignore uClibc masquerading as glibc.
+#if defined(__GLIBC__) && !defined(__UCLIBC__)
+ return 1;
+#else
+ return 0;
+#endif
+}
+
void
x_cgo_sys_thread_create(void* (*func)(void*), void* arg) {
pthread_t p;
diff --git a/src/runtime/cgo/gcc_libinit_windows.c b/src/runtime/cgo/gcc_libinit_windows.c
index 9a8c65e..a4693fb 100644
--- a/src/runtime/cgo/gcc_libinit_windows.c
+++ b/src/runtime/cgo/gcc_libinit_windows.c
@@ -62,6 +62,11 @@
}
}
+int
+x_cgo_sys_lib_args_valid() {
+ return 1;
+}
+
void
x_cgo_sys_thread_create(void (*func)(void*), void* arg) {
_cgo_beginthread(func, arg);
diff --git a/src/runtime/os_linux.go b/src/runtime/os_linux.go
index 6ce656c..0a97b44 100644
--- a/src/runtime/os_linux.go
+++ b/src/runtime/os_linux.go
@@ -233,21 +233,44 @@
func sysargs(argc int32, argv **byte) {
n := argc + 1
- // skip over argv, envp to get to auxv
- for argv_index(argv, n) != nil {
+ argsValid := true
+ if islibrary || isarchive {
+ if !sysLibArgsValid() {
+ argsValid = false
+ }
+ }
+
+ if argsValid {
+ // skip over argv, envp to get to auxv
+ for argv_index(argv, n) != nil {
+ n++
+ }
+
+ // skip NULL separator
n++
+
+ // now argv+n is auxv
+ auxvp := (*[1 << 28]uintptr)(add(unsafe.Pointer(argv), uintptr(n)*goarch.PtrSize))
+
+ if pairs := sysauxv(auxvp[:]); pairs != 0 {
+ auxv = auxvp[: pairs*2 : pairs*2]
+ return
+ }
+ } else {
+ args := unsafe.Pointer(persistentalloc(goarch.PtrSize*4, 0, &memstats.other_sys))
+ // argv pointer
+ *(**byte)(args) = (*byte)(add(args, goarch.PtrSize*1))
+ // argv data
+ *(**byte)(add(args, goarch.PtrSize*1)) = (*byte)(nil) // end argv TODO: READ FROM /proc/
+ *(**byte)(add(args, goarch.PtrSize*2)) = (*byte)(nil) // end envp TODO: READ FROM /proc/
+ *(**byte)(add(args, goarch.PtrSize*3)) = (*byte)(nil) // end auxv TODO: READ FROM /proc/
+ argc = 0
+ argv = (**byte)(args)
+
+ // argc = 0
+ // argv = (**byte)(&[3]*byte{nil, nil, nil})
}
-
- // skip NULL separator
- n++
-
- // now argv+n is auxv
- auxvp := (*[1 << 28]uintptr)(add(unsafe.Pointer(argv), uintptr(n)*goarch.PtrSize))
-
- if pairs := sysauxv(auxvp[:]); pairs != 0 {
- auxv = auxvp[: pairs*2 : pairs*2]
- return
- }
+
// In some situations we don't get a loader-provided
// auxv, such as when loaded as a library on Android.
// Fall back to /proc/self/auxv.
diff --git a/src/runtime/runtime1.go b/src/runtime/runtime1.go
index 5b37d23..81dcb35 100644
--- a/src/runtime/runtime1.go
+++ b/src/runtime/runtime1.go
@@ -56,10 +56,28 @@
argv **byte
)
+// when using -buildmode=c-archive or -buildmode=c-shared on linux
+// we have to first make sure that glibc is being used or else
+// we cannot rely on argc/argv/auxv to be accurate
+func sysLibArgsValid() bool {
+ if _cgo_sys_lib_args_valid != nil {
+ ret := asmcgocall(_cgo_sys_lib_args_valid, nil)
+ if ret != 1 {
+ return false
+ }
+ }
+ return true
+}
+
// nosplit for use in linux startup sysargs.
//
//go:nosplit
func argv_index(argv **byte, i int32) *byte {
+ if islibrary || isarchive {
+ if !sysLibArgsValid() {
+ return nil
+ }
+ }
return *(**byte)(add(unsafe.Pointer(argv), uintptr(i)*goarch.PtrSize))
}
@@ -73,6 +91,13 @@
if GOOS == "windows" {
return
}
+
+ if islibrary || isarchive {
+ if !sysLibArgsValid() {
+ return
+ }
+ }
+
argslice = make([]string, argc)
for i := int32(0); i < argc; i++ {
argslice[i] = gostringnocopy(argv_index(argv, i))
@@ -80,6 +105,13 @@
}
func goenvs_unix() {
+ if islibrary || isarchive {
+ if !sysLibArgsValid() {
+ envs = make([]string, 0)
+ return
+ }
+ }
+
// TODO(austin): ppc64 in dynamic linking mode doesn't
// guarantee env[] will immediately follow argv. Might cause
// problems.
Change information
- M src/cmd/cgo/internal/testcarchive/testdata/libgo/libgo.go
- M src/runtime/cgo.go
- M src/runtime/cgo/callbacks.go
- M src/runtime/cgo/gcc_libinit.c
- M src/runtime/cgo/gcc_libinit_windows.c
- M src/runtime/os_linux.go
- M src/runtime/runtime1.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems.
These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. Otherwise, please address any problems and update the GitHub PR. When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.
Possible problems detected:
1. Lines in the commit message should be wrapped at ~76 characters unless needed for things like URLs or tables. You have a 361 character line.
2. You usually need to reference a bug number for all but trivial or cosmetic fixes. For this repo, the format is usually 'Fixes #12345' or 'Updates #12345' at the end of the commit message. Should you have a bug reference?
The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 5 comments![]( "Open in Gerrit")
runtime: fixes using go .so and .a libraries with musl/uclinuxruntime: don't fetch c-archive/c-shared os.Args on musl/uclinux
I've attempted to get these changes included in the past and have been manually patching my go releases for alpine. I believe the issue raised previously was that in theory we could parse the arguments from /proc but honestly I don't have time to mess with that and don't see the need for a library to be aware of argc/argv unless someone here thinks otherwise.Please add line breaks here and elsewhere. Thanks.
#include <limits.h>Why include <limits.h> ?
// Detect if using glibcThis needs a better comment explaining when it is called and what it does.
#if defined(__GLIBC__) && !defined(__UCLIBC__)What about other operating systems that don't use glibc?
Related details
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Ian Lance Taylor
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Ian Lance Taylor
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Ian Lance Taylor
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Eaton (Gerrit)
Alex Eaton added 3 comments![]( "Open in Gerrit")
#include <limits.h>Why include <limits.h> ?
Good catch, it should have been <features.h>
// Detect if using glibcThis needs a better comment explaining when it is called and what it does.
Pushed with more detail, let me know if that is sufficient
#if defined(__GLIBC__) && !defined(__UCLIBC__)What about other operating systems that don't use glibc?
My understanding is that the DT_INIT_ARRAY is optional per https://www.sco.com/developers/gabi/latest/ch5.dynamic.html#init_fini
So glibc may be the only library that optionally passes these arguments - I know that neither musl/uclibc do however
Related details
- Ian Lance Taylor
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Eaton (Gerrit)
Alex Eaton added 1 comment![]( "Open in Gerrit")
I created an example sandbox repository to demonstrate the issue before/after the patch has been applied https://github.com/1800alex/go-cshared-example
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 2 comments![]( "Open in Gerrit")
#include <limits.h>Alex EatonWhy include <limits.h> ?
Good catch, it should have been <features.h>
<features.h> isn't a standard header file as far as I know. It's unlikely to be present on all operating systems. And it shouldn't be necessary to #include it here, as the other headers are bound to bring it in.
#if defined(__GLIBC__) && !defined(__UCLIBC__)Alex EatonWhat about other operating systems that don't use glibc?
My understanding is that the DT_INIT_ARRAY is optional per https://www.sco.com/developers/gabi/latest/ch5.dynamic.html#init_fini
So glibc may be the only library that optionally passes these arguments - I know that neither musl/uclibc do however
glibc is not the only library that passes these arguments. They are also passed on at least some *BSD systems. I think this function must return 1 _except_ for the case of uclibc.
Related details
- Alex Eaton
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Alex Eaton
- Ian Lance Taylor
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Eaton (Gerrit)
Alex Eaton added 2 comments![]( "Open in Gerrit")
#include <limits.h>Alex EatonWhy include <limits.h> ?
Ian Lance TaylorGood catch, it should have been <features.h>
<features.h> isn't a standard header file as far as I know. It's unlikely to be present on all operating systems. And it shouldn't be necessary to #include it here, as the other headers are bound to bring it in.
My mistake, you are correct - removed
#if defined(__GLIBC__) && !defined(__UCLIBC__)Alex EatonWhat about other operating systems that don't use glibc?
Ian Lance TaylorMy understanding is that the DT_INIT_ARRAY is optional per https://www.sco.com/developers/gabi/latest/ch5.dynamic.html#init_fini
So glibc may be the only library that optionally passes these arguments - I know that neither musl/uclibc do however
glibc is not the only library that passes these arguments. They are also passed on at least some *BSD systems. I think this function must return 1 _except_ for the case of uclibc.
It seems that musl does defined a macro such as __MUSL__ so an explicit check for __GLIBC__ may be required. I pushed a change to wrap this with __linux__ so I think bsd systems would be unaffected
Related details
- Ian Lance Taylor
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Ian Lance Taylor
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Eaton (Gerrit)
Alex Eaton added 1 comment![]( "Open in Gerrit")
#if defined(__GLIBC__) && !defined(__UCLIBC__)Alex EatonWhat about other operating systems that don't use glibc?
Ian Lance TaylorMy understanding is that the DT_INIT_ARRAY is optional per https://www.sco.com/developers/gabi/latest/ch5.dynamic.html#init_fini
So glibc may be the only library that optionally passes these arguments - I know that neither musl/uclibc do however
Alex Eatonglibc is not the only library that passes these arguments. They are also passed on at least some *BSD systems. I think this function must return 1 _except_ for the case of uclibc.
It seems that musl does defined a macro such as __MUSL__ so an explicit check for __GLIBC__ may be required. I pushed a change to wrap this with __linux__ so I think bsd systems would be unaffected
typo - there is NOT a macro to detect MUSL and they did that by design
https://wiki.musl-libc.org/faq.html
Related details
- Ian Lance Taylor
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor voted Commit-Queue+1![]( "Open in Gerrit")
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Lance Taylor (Gerrit)
Ian Lance Taylor added 5 comments![]( "Open in Gerrit")
#include <limits.h>Alex EatonWhy include <limits.h> ?
Ian Lance TaylorGood catch, it should have been <features.h>
Alex Eaton<features.h> isn't a standard header file as far as I know. It's unlikely to be present on all operating systems. And it shouldn't be necessary to #include it here, as the other headers are bound to bring it in.
My mistake, you are correct - removed
Done
// Detect if using glibcAlex EatonThis needs a better comment explaining when it is called and what it does.
Pushed with more detail, let me know if that is sufficient
Done
#if defined(__GLIBC__) && !defined(__UCLIBC__)Alex EatonWhat about other operating systems that don't use glibc?
Ian Lance TaylorMy understanding is that the DT_INIT_ARRAY is optional per https://www.sco.com/developers/gabi/latest/ch5.dynamic.html#init_fini
So glibc may be the only library that optionally passes these arguments - I know that neither musl/uclibc do however
Alex Eatonglibc is not the only library that passes these arguments. They are also passed on at least some *BSD systems. I think this function must return 1 _except_ for the case of uclibc.
Alex EatonIt seems that musl does defined a macro such as __MUSL__ so an explicit check for __GLIBC__ may be required. I pushed a change to wrap this with __linux__ so I think bsd systems would be unaffected
typo - there is NOT a macro to detect MUSL and they did that by design
https://wiki.musl-libc.org/faq.html
This nested #if is somewhat confusing. How about something like
// On Linux systems, uClibc does not pass argc/argv to libraries.
#define hasSharedArgs !defined(__linux__) || (defined(__GLIBC__) && !defined(__UCLIBC__))
return hasSharedArgs;
args := unsafe.Pointer(persistentalloc(goarch.PtrSize*4, 0, &memstats.other_sys))Do we really need to allocate this memory? Perhaps instead anything that might use can check whether the arguments are valid.
- Alex Eaton
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alex Eaton
- Ian Lance Taylor
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quentin Quaadgras (Gerrit)
Quentin Quaadgras added 1 comment![]( "Open in Gerrit")
Is it worth reading the program's arguments from proc/self on Linux?
Related details
- Alex Eaton
- Ian Lance Taylor
- Keith Randall
- Michael Knyszek
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |