diff mbox series

[PATCHv7,2/6] go: fix sigaction usage on i386 platforms

Message ID 20250910212855.2480561-3-rs@ti.com
State New
Headers show
Series Display manager proposal for x11 and wayland | expand

Commit Message

Randolph Sapp Sept. 10, 2025, 9:28 p.m. UTC
From: Randolph Sapp <rs@ti.com>

In upstream the following commit [1] was submitted to resolve issues
with sigaction being used in linked libraries with cgo applications.

runtime: when using cgo on 386, call C sigaction function

This should address the previously reported issues with pam
authentication of go applications on related x86 platforms.

[1] https://github.com/golang/go/commit/c5737dc21bbac9fbefc35ac9313e66291d66b382

Signed-off-by: Randolph Sapp <rs@ti.com>
---
 meta/recipes-devtools/go/go-1.25.0.inc        |   1 +
 ...ng-cgo-on-386-call-C-sigaction-funct.patch | 247 ++++++++++++++++++
 2 files changed, 248 insertions(+)
 create mode 100644 meta/recipes-devtools/go/go/0001-runtime-when-using-cgo-on-386-call-C-sigaction-funct.patch

Comments

patchtest@automation.yoctoproject.org Sept. 10, 2025, 9:48 p.m. UTC | #1
Thank you for your submission. Patchtest identified one
or more issues with the patch. Please see the log below for
more information:

---
Testing patch /home/patchtest/share/mboxes/PATCHv7-2-6-go-fix-sigaction-usage-on-i386-platforms.patch

FAIL: test Signed-off-by presence: A patch file has been added without a Signed-off-by tag: '0001-runtime-when-using-cgo-on-386-call-C-sigaction-funct.patch' (test_patch.TestPatch.test_signed_off_by_presence)

PASS: test CVE tag format (test_patch.TestPatch.test_cve_tag_format)
PASS: test Signed-off-by presence (test_mbox.TestMbox.test_signed_off_by_presence)
PASS: test Upstream-Status presence (test_patch.TestPatch.test_upstream_status_presence_format)
PASS: test author valid (test_mbox.TestMbox.test_author_valid)
PASS: test commit message presence (test_mbox.TestMbox.test_commit_message_presence)
PASS: test commit message user tags (test_mbox.TestMbox.test_commit_message_user_tags)
PASS: test max line length (test_metadata.TestMetadata.test_max_line_length)
PASS: test mbox format (test_mbox.TestMbox.test_mbox_format)
PASS: test non-AUH upgrade (test_mbox.TestMbox.test_non_auh_upgrade)
PASS: test shortlog format (test_mbox.TestMbox.test_shortlog_format)
PASS: test shortlog length (test_mbox.TestMbox.test_shortlog_length)
PASS: test target mailing list (test_mbox.TestMbox.test_target_mailing_list)

SKIP: pretest pylint: No python related patches, skipping test (test_python_pylint.PyLint.pretest_pylint)
SKIP: pretest src uri left files: No modified recipes, skipping pretest (test_metadata.TestMetadata.pretest_src_uri_left_files)
SKIP: test CVE check ignore: No modified recipes or older target branch, skipping test (test_metadata.TestMetadata.test_cve_check_ignore)
SKIP: test bugzilla entry format: No bug ID found (test_mbox.TestMbox.test_bugzilla_entry_format)
SKIP: test lic files chksum modified not mentioned: No modified recipes, skipping test (test_metadata.TestMetadata.test_lic_files_chksum_modified_not_mentioned)
SKIP: test lic files chksum presence: No added recipes, skipping test (test_metadata.TestMetadata.test_lic_files_chksum_presence)
SKIP: test license presence: No added recipes, skipping test (test_metadata.TestMetadata.test_license_presence)
SKIP: test pylint: No python related patches, skipping test (test_python_pylint.PyLint.test_pylint)
SKIP: test series merge on head: Merge test is disabled for now (test_mbox.TestMbox.test_series_merge_on_head)
SKIP: test src uri left files: No modified recipes, skipping pretest (test_metadata.TestMetadata.test_src_uri_left_files)
SKIP: test summary presence: No added recipes, skipping test (test_metadata.TestMetadata.test_summary_presence)

---

Please address the issues identified and
submit a new revision of the patch, or alternatively, reply to this
email with an explanation of why the patch should be accepted. If you
believe these results are due to an error in patchtest, please submit a
bug at https://bugzilla.yoctoproject.org/ (use the 'Patchtest' category
under 'Yocto Project Subprojects'). For more information on specific
failures, see: https://wiki.yoctoproject.org/wiki/Patchtest. Thank
you!
Mathieu Dubois-Briand Sept. 12, 2025, 6:10 a.m. UTC | #2
On Wed Sep 10, 2025 at 11:48 PM CEST, Patchtest via lists.openembedded.org wrote:
> Thank you for your submission. Patchtest identified one
> or more issues with the patch. Please see the log below for
> more information:
>
> ---
> Testing patch /home/patchtest/share/mboxes/PATCHv7-2-6-go-fix-sigaction-usage-on-i386-platforms.patch
>
> FAIL: test Signed-off-by presence: A patch file has been added without a Signed-off-by tag: '0001-runtime-when-using-cgo-on-386-call-C-sigaction-funct.patch' (test_patch.TestPatch.test_signed_off_by_presence)
>

Hi Randolph,

Have you seen the above message? I believe this is indeed an issue with
this patch, so I haven't took your series so far. Any thought?

Thanks,
Mathieu
diff mbox series

Patch

diff --git a/meta/recipes-devtools/go/go-1.25.0.inc b/meta/recipes-devtools/go/go-1.25.0.inc
index f562fbb34b..1558b4623d 100644
--- a/meta/recipes-devtools/go/go-1.25.0.inc
+++ b/meta/recipes-devtools/go/go-1.25.0.inc
@@ -16,5 +16,6 @@  SRC_URI += "\
     file://0009-go-Filter-build-paths-on-staticly-linked-arches.patch \
     file://0010-cmd-go-clear-GOROOT-for-func-ldShared-when-trimpath-.patch \
     file://0011-cmd-link-stop-forcing-binutils-gold-dependency-on-aa.patch \
+    file://0001-runtime-when-using-cgo-on-386-call-C-sigaction-funct.patch \
 "
 SRC_URI[main.sha256sum] = "4bd01e91297207bfa450ea40d4d5a93b1b531a5e438473b2a06e18e077227225"
diff --git a/meta/recipes-devtools/go/go/0001-runtime-when-using-cgo-on-386-call-C-sigaction-funct.patch b/meta/recipes-devtools/go/go/0001-runtime-when-using-cgo-on-386-call-C-sigaction-funct.patch
new file mode 100644
index 0000000000..d063e850a8
--- /dev/null
+++ b/meta/recipes-devtools/go/go/0001-runtime-when-using-cgo-on-386-call-C-sigaction-funct.patch
@@ -0,0 +1,247 @@ 
+From c5737dc21bbac9fbefc35ac9313e66291d66b382 Mon Sep 17 00:00:00 2001
+From: Ian Lance Taylor <iant@golang.org>
+Date: Fri, 5 Sep 2025 22:24:37 -0700
+Subject: [PATCH] runtime: when using cgo on 386, call C sigaction function
+
+On 386 the C sigaction function assumes that the caller does not set
+the SA_RESTORER flag. It does not copy the C sa_restorer field to
+the kernel sa_restorer field. The effect is that the kernel sees
+the SA_RESTORER flag but a NULL sa_restorer field, and the program
+crashes when returning from a signal handler.
+
+On the other hand, the C sigaction function will return the SA_RESTORER
+flag and the sa_restorer field stored in the kernel.
+
+This means that if the Go runtime installs a signal handler,
+with SA_RESTORER as is required when calling the kernel,
+and the Go program calls C code that calls the C sigaction function
+to query the current signal handler, that C code will get a result
+that it can't pass back to sigaction.
+
+This CL fixes the problem by using the C sigaction function
+for 386 programs that use cgo. This reuses the functionality
+used on amd64 and other GOARCHs to support the race detector.
+
+See #75253, or runtime/testdata/testprogcgo/eintr.go, for sample
+code that used to fail on 386. No new test case is required,
+we just remove the skip we used to have for eintr.go.
+
+Fixes #75253
+
+Change-Id: I803059b1fb9e09e9fbb43f68eccb6a59a92c2991
+Reviewed-on: https://go-review.googlesource.com/c/go/+/701375
+LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
+Reviewed-by: Cherry Mui <cherryyz@google.com>
+Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
+Auto-Submit: Ian Lance Taylor <iant@golang.org>
+Upstream-Status: Backport [https://github.com/golang/go/commit/c5737dc21bbac9fbefc35ac9313e66291d66b382]
+---
+ src/runtime/cgo/gcc_sigaction.c |  8 ++++----
+ src/runtime/cgo/sigaction.go    |  2 +-
+ src/runtime/cgo_sigaction.go    |  6 +++++-
+ src/runtime/crash_cgo_test.go   | 11 -----------
+ src/runtime/os_freebsd.go       |  6 ++++++
+ src/runtime/os_linux.go         | 18 +++++++++++++++++-
+ src/runtime/sigaction.go        |  2 +-
+ src/runtime/sys_linux_386.s     | 19 +++++++++++++++++++
+ 8 files changed, 53 insertions(+), 19 deletions(-)
+
+diff --git a/src/runtime/cgo/gcc_sigaction.c b/src/runtime/cgo/gcc_sigaction.c
+index 7cbef7db11..ad48a88dc1 100644
+--- a/src/runtime/cgo/gcc_sigaction.c
++++ b/src/runtime/cgo/gcc_sigaction.c
+@@ -2,7 +2,7 @@
+ // Use of this source code is governed by a BSD-style
+ // license that can be found in the LICENSE file.
+ 
+-//go:build linux && (amd64 || arm64 || loong64 || ppc64le)
++//go:build linux && (386 || amd64 || arm64 || loong64 || ppc64le)
+ 
+ #include <errno.h>
+ #include <stddef.h>
+@@ -17,7 +17,7 @@
+ // to and from struct sigaction — are specific to ${goos}/${goarch}.
+ typedef struct {
+ 	uintptr_t handler;
+-	uint64_t flags;
++	unsigned long flags;
+ #ifdef __loongarch__
+ 	uint64_t mask;
+ 	uintptr_t restorer;
+@@ -57,7 +57,7 @@ x_cgo_sigaction(intptr_t signum, const go_sigaction_t *goact, go_sigaction_t *ol
+ 				sigaddset(&act.sa_mask, (int)(i+1));
+ 			}
+ 		}
+-		act.sa_flags = (int)(goact->flags & ~(uint64_t)SA_RESTORER);
++		act.sa_flags = (int)(goact->flags & ~(unsigned long)SA_RESTORER);
+ 	}
+ 
+ 	ret = sigaction((int)signum, goact ? &act : NULL, oldgoact ? &oldact : NULL);
+@@ -79,7 +79,7 @@ x_cgo_sigaction(intptr_t signum, const go_sigaction_t *goact, go_sigaction_t *ol
+ 				oldgoact->mask |= (uint64_t)(1)<<i;
+ 			}
+ 		}
+-		oldgoact->flags = (uint64_t)oldact.sa_flags;
++		oldgoact->flags = (unsigned long)oldact.sa_flags;
+ 	}
+ 
+ 	_cgo_tsan_release();
+diff --git a/src/runtime/cgo/sigaction.go b/src/runtime/cgo/sigaction.go
+index dc3f5fd255..90034bad32 100644
+--- a/src/runtime/cgo/sigaction.go
++++ b/src/runtime/cgo/sigaction.go
+@@ -2,7 +2,7 @@
+ // Use of this source code is governed by a BSD-style
+ // license that can be found in the LICENSE file.
+ 
+-//go:build (linux && (amd64 || arm64 || loong64 || ppc64le)) || (freebsd && amd64)
++//go:build (linux && (386 || amd64 || arm64 || loong64 || ppc64le)) || (freebsd && amd64)
+ 
+ package cgo
+ 
+diff --git a/src/runtime/cgo_sigaction.go b/src/runtime/cgo_sigaction.go
+index 5c644587f0..f725dbef4d 100644
+--- a/src/runtime/cgo_sigaction.go
++++ b/src/runtime/cgo_sigaction.go
+@@ -3,8 +3,10 @@
+ // license that can be found in the LICENSE file.
+ 
+ // Support for sanitizers. See runtime/cgo/sigaction.go.
++// Also used on linux/386 to clear the SA_RESTORER flag
++// when using cgo; see issue #75253.
+ 
+-//go:build (linux && (amd64 || arm64 || loong64 || ppc64le)) || (freebsd && amd64)
++//go:build (linux && (386 || amd64 || arm64 || loong64 || ppc64le)) || (freebsd && amd64)
+ 
+ package runtime
+ 
+@@ -42,6 +44,8 @@ func sigaction(sig uint32, new, old *sigactiont) {
+ 
+ 		var ret int32
+ 
++		fixSigactionForCgo(new)
++
+ 		var g *g
+ 		if mainStarted {
+ 			g = getg()
+diff --git a/src/runtime/crash_cgo_test.go b/src/runtime/crash_cgo_test.go
+index c5d7303261..b77ff8dafd 100644
+--- a/src/runtime/crash_cgo_test.go
++++ b/src/runtime/crash_cgo_test.go
+@@ -842,17 +842,6 @@ func TestEINTR(t *testing.T) {
+ 	switch runtime.GOOS {
+ 	case "plan9", "windows":
+ 		t.Skipf("no EINTR on %s", runtime.GOOS)
+-	case "linux":
+-		if runtime.GOARCH == "386" {
+-			// On linux-386 the Go signal handler sets
+-			// a restorer function that is not preserved
+-			// by the C sigaction call in the test,
+-			// causing the signal handler to crash when
+-			// returning the normal code. The test is not
+-			// architecture-specific, so just skip on 386
+-			// rather than doing a complicated workaround.
+-			t.Skip("skipping on linux-386; C sigaction does not preserve Go restorer")
+-		}
+ 	}
+ 	if runtime.GOOS == "freebsd" && race.Enabled {
+ 		t.Skipf("race + cgo freebsd not supported. See https://go.dev/issue/73788.")
+diff --git a/src/runtime/os_freebsd.go b/src/runtime/os_freebsd.go
+index ab859cfb47..68d895b95d 100644
+--- a/src/runtime/os_freebsd.go
++++ b/src/runtime/os_freebsd.go
+@@ -457,6 +457,12 @@ func sysSigaction(sig uint32, new, old *sigactiont) {
+ 	}
+ }
+ 
++// fixSigactionForCgo is needed for Linux.
++//
++//go:nosplit
++func fixSigactionForCgo(new *sigactiont) {
++}
++
+ // asmSigaction is implemented in assembly.
+ //
+ //go:noescape
+diff --git a/src/runtime/os_linux.go b/src/runtime/os_linux.go
+index c9d25a5be8..f9fe1b5f33 100644
+--- a/src/runtime/os_linux.go
++++ b/src/runtime/os_linux.go
+@@ -486,7 +486,8 @@ func setsig(i uint32, fn uintptr) {
+ 	sigfillset(&sa.sa_mask)
+ 	// Although Linux manpage says "sa_restorer element is obsolete and
+ 	// should not be used". x86_64 kernel requires it. Only use it on
+-	// x86.
++	// x86. Note that on 386 this is cleared when using the C sigaction
++	// function via cgo; see fixSigactionForCgo.
+ 	if GOARCH == "386" || GOARCH == "amd64" {
+ 		sa.sa_restorer = abi.FuncPCABI0(sigreturn__sigaction)
+ 	}
+@@ -562,6 +563,21 @@ func sysSigaction(sig uint32, new, old *sigactiont) {
+ //go:noescape
+ func rt_sigaction(sig uintptr, new, old *sigactiont, size uintptr) int32
+ 
++// fixSigactionForCgo is called when we are using cgo to call the
++// C sigaction function. On 386 the C function does not expect the
++// SA_RESTORER flag to be set, and in some cases will fail if it is set:
++// it will pass the SA_RESTORER flag to the kernel without passing
++// the sa_restorer field. Since the C function will handle SA_RESTORER
++// for us, we need not pass it. See issue #75253.
++//
++//go:nosplit
++func fixSigactionForCgo(new *sigactiont) {
++	if GOARCH == "386" && new != nil {
++		new.sa_flags &^= _SA_RESTORER
++		new.sa_restorer = 0
++	}
++}
++
+ func getpid() int
+ func tgkill(tgid, tid, sig int)
+ 
+diff --git a/src/runtime/sigaction.go b/src/runtime/sigaction.go
+index 2027ae80bf..1a99f7f3ec 100644
+--- a/src/runtime/sigaction.go
++++ b/src/runtime/sigaction.go
+@@ -2,7 +2,7 @@
+ // Use of this source code is governed by a BSD-style
+ // license that can be found in the LICENSE file.
+ 
+-//go:build (linux && !amd64 && !arm64 && !loong64 && !ppc64le) || (freebsd && !amd64)
++//go:build (linux && !386 && !amd64 && !arm64 && !loong64 && !ppc64le) || (freebsd && !amd64)
+ 
+ package runtime
+ 
+diff --git a/src/runtime/sys_linux_386.s b/src/runtime/sys_linux_386.s
+index d53be243fe..8e832687e0 100644
+--- a/src/runtime/sys_linux_386.s
++++ b/src/runtime/sys_linux_386.s
+@@ -410,6 +410,25 @@ TEXT runtime·rt_sigaction(SB),NOSPLIT,$0
+ 	MOVL	AX, ret+16(FP)
+ 	RET
+ 
++// Call the function stored in _cgo_sigaction using the GCC calling convention.
++TEXT runtime·callCgoSigaction(SB),NOSPLIT,$0-16
++	MOVL	_cgo_sigaction(SB), AX
++	MOVL	sig+0(FP), BX
++	MOVL	new+4(FP), CX
++	MOVL	old+8(FP), DX
++	MOVL	SP, SI // align stack to call C function
++	SUBL	$32, SP
++	ANDL	$~15, SP
++	MOVL	BX, 0(SP)
++	MOVL	CX, 4(SP)
++	MOVL	DX, 8(SP)
++	MOVL	SI, 12(SP)
++	CALL	AX
++	MOVL	12(SP), BX
++	MOVL	BX, SP
++	MOVL	AX, ret+12(FP)
++	RET
++
+ TEXT runtime·sigfwd(SB),NOSPLIT,$12-16
+ 	MOVL	fn+0(FP), AX
+ 	MOVL	sig+4(FP), BX
+-- 
+2.51.0
+