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

syscall: on linux 386 doesn't support syscalls that don't fail #22924

Closed
chipaca opened this issue Nov 29, 2017 · 17 comments
Closed

syscall: on linux 386 doesn't support syscalls that don't fail #22924

chipaca opened this issue Nov 29, 2017 · 17 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@chipaca
Copy link
Contributor

chipaca commented Nov 29, 2017

What version of Go are you using (go version)?

1.6.2 and 1.9.2

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

linux/386 and linux/arm.

What did you do?

with this built as hightest,

package main

import (
	"fmt"
	"syscall"
)

func main() {
	r0, _, e := syscall.RawSyscall(syscall.SYS_GETUID32, 0, 0, 0)
	fmt.Println(uint32(r0), "/", e)
}

and a user with a high uid,

sudo useradd --uid "$(( 0xfffffffe ))" --shell /bin/sh hightest

(alternatively if you can't useradd, use chown to a numerical id, and chmod u+s, and change the code to use GETEUID instead of GETUID)

compare the output of sudo -u hightest id -u and sudo -u hightest hightest.

What did you expect to see?

4294967294 / errno 0

What did you see instead?

4294967295 / no such file or directory

The problem seems to be that in syscall/asm_linux_386.s, if the returned value is not less than 0xfffff001 it's treated as an error.

@bradfitz bradfitz changed the title syscall on linux 386 doesn't support syscalls that don't fail syscall: on linux 386 doesn't support syscalls that don't fail Nov 29, 2017
@bradfitz bradfitz added this to the Go1.11 milestone Nov 29, 2017
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 29, 2017
@bradfitz
Copy link
Contributor

/cc @ianlancetaylor

@ianlancetaylor
Copy link
Contributor

The same thing happens on amd64 if there are any syscalls that can return a valid value larger than 0xfffffffffffff001.

@ianlancetaylor
Copy link
Contributor

OK, I see. The code in the syscall package is correct for syscalls that can return an error. However, there are certain syscalls that do not return an error. For those, we should not check the result in this way. Those syscalls are:

  • clock_gettime (amd64 only, I think)
  • getegid
  • getegid32 (386 only)
  • geteuid
  • geteuid32 (386 only)
  • getgid
  • getgid32 (386 only)
  • getpgrp
  • getpid
  • getppid
  • getuid
  • getuid32 (386 only)
  • personality (386 only, I think)
  • setfsgid32 (386 only)
  • setfsuid32 (386 only)
  • umask

@ianlancetaylor
Copy link
Contributor

Clearly we should fix Getuid, et. al., in the syscall package. It's less clear whether we should add a new API function such as SyscallNoError to the syscall package, or reserve that for x/sys/unix.

@ianlancetaylor ianlancetaylor added release-blocker NeedsFix The path to resolution is known, but the work has not been done. labels Nov 29, 2017
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 29, 2017
@bradfitz
Copy link
Contributor

x/sys/unix for sure.

We can add [more] comments to syscall pointing to x/sys/unix where needed.

@gopherbot
Copy link

Change https://golang.org/cl/84485 mentions this issue: syscall: support syscalls without error return on Linux

@tklauser tklauser self-assigned this Dec 18, 2017
@tklauser
Copy link
Member

I submitted https://golang.org/cl/84485 with a possible fix in syscall, marked for go 1.11 (but not ready for review yet). I'd like to add a test, but I couldn't yet figure out a proper way to add one based on the reproducer provided in this issue. Will hopefully find some time to work on that during the holiday season.

@tklauser
Copy link
Member

tklauser commented Dec 19, 2017

Tested on linux/386 (via gomote) with https://golang.org/cl/84485 and the following program:

package main

import (
	"fmt"
	"syscall"
)

func main() {
	// r0, _, e := syscall.RawSyscall(syscall.SYS_GETUID32, 0, 0, 0)
	// fmt.Println(uint32(r0), "/", e)
	u := syscall.Getuid()
	fmt.Println(uint32(u))
}

and indeed it print 4294967294 as expected.

If anyone has an idea on how to turn the provided reproducer into a nice test, any hints would be appreciated.

I'll also work on a corresponding implementation for x/sys/unix, adding an exported RawSyscallNoError so the following program should also print 4294967294 / 0

package main

import (
	"fmt"
	"golang.org/x/sys/unix"
)

func main() {
	r0, _, e := unix.RawSyscallNoError(syscall.SYS_GETUID32, 0, 0, 0)
	fmt.Println(uint32(r0), "/", e)
}

@chipaca
Copy link
Contributor Author

chipaca commented Dec 19, 2017

How is the syscall package tested? This isn't the only syscall that'd need special privileges to test the corner cases, I'm sure.

@tklauser
Copy link
Member

tklauser commented Dec 19, 2017

The syscall is tested via the tests in syscall/*_test.go and some functionality is also implicitly tested via the os package tests. There are also some tests regarding syscall in test/ (e.g. test/fixedbugs/issue15002.go). But all of these run as a non-privileged user AFAIK.

As for the syscall to use (from @ianlancetaylor's list in #22924 (comment)), I think only the gete?[gu]id, the personality (which seems to be available on all GOARCHs btw) and the umask syscalls let userspace influence what value the kernel will return. And umaskwill probably not even allow us to set a high enough value to trigger the issue.

@bradfitz
Copy link
Contributor

@chipaca, historically the syscall package has had few explicit tests. Most tests had been implicit ones, tested via upper layers working correctly. That's not ideal and has slowly changed, but there's still not a lot of explicit testing. It also doesn't change often, so manual testing has so far sufficed. (Again, not ideal.)

@tklauser, some of of builders do run as root, but most of our Linux ones run as a sort of "fake root" in Kubernetes that is UID 0 but with a lot fewer capabilities enabled. We could re-enable a Linux builder in a VM running as root if needed, and then we can test for being in that VM via testenv.Builder().

@gopherbot
Copy link

Change https://golang.org/cl/85756 mentions this issue: unix: add SyscallNoError and RawSyscallNoError on Linux

gopherbot pushed a commit to golang/sys that referenced this issue Jan 3, 2018
Add the SyscallNoError and RawSyscallNoError wrapper functions which are
used for syscalls that don't return an error. Also convert all applicable
syscalls to use these wrappers.

Updates golang/go#22924

Change-Id: Ie143abc99de8d12d36b67ac278e08d7565f73ce0
Reviewed-on: https://go-review.googlesource.com/85756
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/90475 mentions this issue: unix: use SyscallNoError and RawSyscallNoError on Linux only

gopherbot pushed a commit to golang/sys that referenced this issue Jan 30, 2018
The SyscallNoError and RawSyscallNoError functions are only needed on
Linux. Make sure they are not used within the generated zsyscall_*.go
files if $GOOS != "linux".

Updates golang/go#22924

Change-Id: I3b2aa8624895b0527dcc832b2945a8d591f0ba1a
Reviewed-on: https://go-review.googlesource.com/90475
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
tklauser added a commit to tklauser/go that referenced this issue Jan 30, 2018
Add the rawSyscallNoError wrapper function which is used for Linux
syscalls that don't return an error and convert all applicable
occurences of RawSyscall to use it instead.

Fixes golang#22924

Change-Id: Iff1eddb54573d459faa01471f10398b3d38528dd
@tklauser
Copy link
Member

Patchset 4 of https://golang.org/cl/84485 now implements the alternative geteuid test as suggested by @chipaca. Currently it only checks whether it is run as root via if os.Getuid() != 0 but with a TODO(tklauser) to eventually change it as @bradfitz suggested, in case we need "non-fake" root. Let's see what the trybots think...

@tklauser
Copy link
Member

The trybots look happy but I'm not sure how to check whether they actually ran the test in question (TestSyscallNoError). @bradfitz, is there a way to check?

FWIW, the test passes locally when run as root on linux/arm or linux/386.

@bradfitz
Copy link
Contributor

@gopherbot
Copy link

Change https://golang.org/cl/118655 mentions this issue: syscall: support Linux syscalls without error return on mipsx/mips64x

gopherbot pushed a commit that referenced this issue Jun 13, 2018
Like on other architectures, use rawSyscallNoError for Linux syscalls
that don't return an error and convert all applicable occurences of
RawSyscall to use it instead.

This was missed in CL 84485 because mkall.sh doesn't support
mipsx/mips64x, so add the corresponding entries as well.

Updates #22924

Change-Id: I762cbee0827140b9890c4a10830e0b4cd33de92f
Reviewed-on: https://go-review.googlesource.com/118655
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
dna2github pushed a commit to dna2fork/go that referenced this issue Jun 14, 2018
Like on other architectures, use rawSyscallNoError for Linux syscalls
that don't return an error and convert all applicable occurences of
RawSyscall to use it instead.

This was missed in CL 84485 because mkall.sh doesn't support
mipsx/mips64x, so add the corresponding entries as well.

Updates golang#22924

Change-Id: I762cbee0827140b9890c4a10830e0b4cd33de92f
Reviewed-on: https://go-review.googlesource.com/118655
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
dmitshur added a commit to gopherjs/gopherjs that referenced this issue Jun 29, 2018
Go 1.11 added the rawSyscallNoError wrapper function which is used
for Linux syscalls that don't return an error and converted all
applicable occurences of RawSyscall to use it instead.
This change implements it for GopherJS on Linux.

Also implement unsetenv_c, analogous to setenv_c, so that os.Unsetenv
can be supported with Node.js, just like os.Setenv already was.

Fixes:

	$ GOOS=linux gopherjs test --short io
	Error: runtime error: native function not implemented: syscall.rawSyscallNoError
	FAIL	io	1.036s

	$ goexec -compiler=gopherjs 'os.Unsetenv("TEST")'
	Error: runtime error: native function not implemented: syscall.unsetenv_c

Follows golang/go@36951a9.
Updates golang/go#22924.
dmitshur added a commit to gopherjs/gopherjs that referenced this issue Aug 21, 2018
Go 1.11 added the rawSyscallNoError wrapper function which is used
for Linux syscalls that don't return an error and converted all
applicable occurences of RawSyscall to use it instead.
This change implements it for GopherJS on Linux.

Also implement unsetenv_c, analogous to setenv_c, so that os.Unsetenv
can be supported with Node.js, just like os.Setenv already was.

Fixes:

	$ GOOS=linux gopherjs test --short io
	Error: runtime error: native function not implemented: syscall.rawSyscallNoError
	FAIL	io	1.036s

	$ goexec -compiler=gopherjs 'os.Unsetenv("TEST")'
	Error: runtime error: native function not implemented: syscall.unsetenv_c

Follows golang/go@36951a9.
Updates golang/go#22924.
@golang golang locked and limited conversation to collaborators Jun 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants