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

x/sys/unix: Pselect does not call pselect6 properly on Linux #61251

Closed
rittneje opened this issue Jul 10, 2023 · 8 comments
Closed

x/sys/unix: Pselect does not call pselect6 properly on Linux #61251

rittneje opened this issue Jul 10, 2023 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Linux
Milestone

Comments

@rittneje
Copy link

unix.Pselect is implemented via the pselect6 syscall. The Linux man page says the following:

The final argument of the pselect6() system call is not a sigset_t * pointer, but is instead a structure of the form:

   struct {
       const sigset_t *ss;     /* Pointer to signal set */
       size_t          ss_len; /* Size (in bytes) of object pointed
                                  to by 'ss' */
   };

However, the unix package does not follow this.

func Pselect(nfd int, r *FdSet, w *FdSet, e *FdSet, timeout *Timespec, sigmask *Sigset_t) (n int, err error) {
	r0, _, e1 := Syscall6(SYS_PSELECT6, uintptr(nfd), uintptr(unsafe.Pointer(r)), uintptr(unsafe.Pointer(w)), uintptr(unsafe.Pointer(e)), uintptr(unsafe.Pointer(timeout)), uintptr(unsafe.Pointer(sigmask)))
	n = int(r0)
	if e1 != 0 {
		err = errnoErr(e1)
	}
	return
}
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 10, 2023
@gopherbot gopherbot added this to the Unreleased milestone Jul 10, 2023
@bcmills
Copy link
Contributor

bcmills commented Jul 10, 2023

@rittneje, do you want to send a CL to fix this?

(CC @ianlancetaylor @bradfitz @tklauser per https://dev.golang.org/owners)

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 10, 2023
@rittneje
Copy link
Author

@bcmills unix.Pselect is auto-generated code, so I'm not sure how to fix it.

@bcmills
Copy link
Contributor

bcmills commented Jul 10, 2023

unix.Pselect is generated from a directive in syscall_linux.go.

I expect the general form of the fix would be:

  1. Change the existing directive to have a new, unexported name (pselect6) with the correct signature for the kernel syscall and re-run the generator, something like:
type pselect6Sigset_t {
	ss *Sigset_t
	ssLen uintptr  // Size (in bytes) of object pointed to by ss.
};
//sys pselect6(nfd int, r *FdSet, w *FdSet, e *FdSet, timeout *Timespec, sigmask *pselect6Sigset_t) (n int, err error)
  1. Add a new, manually-written func Pselect that implements the POSIX signature in terms of the new (Linux-specific) function:
func Pselect(nfd int, r *FdSet, w *FdSet, e *FdSet, timeout *Timespec, sigmask *Sigset_t) (n int, err error) {
	// Per https://man7.org/linux/man-pages/man2/select.2.html#NOTES,
	// “The Linux pselect6() system call modifies its timeout argument.
	// [Not modifying the argument] is the behavior required by POSIX.1-2001.”
	var mutableTimeout *Timespec
	if timeout != nil {
		mutableTimeout = new(Timespec)
		*mutableTimeout = *timeout
	}

	// “The final argument of the pselect6() system call is not a
	// sigset_t * pointer, but is instead a structure …”
	var kernelMask *pselect6Sigset_t
	if sigmask != nil {
		kernelMask = &pselect6Sigset_t{
			ss: sigmask,
			ssLen: unsafe.Sizeof(*sigmask),
		}
	}

	return pselect6(nfd, r, w, e, mutableTimeout, kernelMask)
}

Plus a regression test, of course. 🙃

@bcmills bcmills added OS-Linux NeedsFix The path to resolution is known, but the work has not been done. labels Jul 10, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 10, 2023
@rittneje
Copy link
Author

Not sure I will be able to submit the patch as it will be somewhat complicated.

Unfortunately, ss_len is not just sizeof(sigset_t) because that would be too easy. From traversing through glibc, these are the defines that are relevant:

#define __WORDSIZE	64 // or 32
#define ULONG_WIDTH __WORDSIZE
#define UCHAR_WIDTH 8

#define ALIGN_DOWN(base, size)	((base) & -((__typeof__ (base)) (size)))

#define ALIGN_UP(base, size)	ALIGN_DOWN ((base) + (size) - 1, (size))

#define __NSIG_WORDS (ALIGN_UP ((_NSIG - 1), ULONG_WIDTH) / ULONG_WIDTH)

#define __NSIG_BYTES (__NSIG_WORDS * (ULONG_WIDTH / UCHAR_WIDTH))

You then need to pass _NSIG_BYTES for ss_len. (On my machine it works out to be 8.)

Somehow x/sys/unix already knows _NSIG, so at least part of the work is done. Not sure exactly how that works though.

In terms of regression testing, this currently fails with an error "invalid argument":

	v := &unix.Sigset_t{}
	v.Val[0] = ^v.Val[0]
	n, err := unix.Pselect(0, nil, nil, nil, &unix.Timespec{Sec: 1}, v)

After correcting it to call pselect6 properly it succeeds. (I'm not sure how unix.Sigset_t is intended to be used since it doesn't appear sigaddset, etc. were mapped.)

@bcmills
Copy link
Contributor

bcmills commented Jul 11, 2023

I think this is how to compute ss_len: https://go.dev/play/p/p6sodpgiXoB

@mknyszek
Copy link
Contributor

@rittneje In triage here, so I added the "help wanted" since I wasn't sure if you're pursuing the patch for this. Please feel free to let me know if you are, and I'll drop the label. Thanks!

@mauri870
Copy link
Member

Just pushed a CL: https://go-review.googlesource.com/c/sys/+/510195

@mknyszek
Copy link
Contributor

This is fixed, it looks like! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Linux
Projects
None yet
Development

No branches or pull requests

5 participants