x/sys/unix: Setfsuid and Setfsgid have wrong return values #36649

shynie · 2020-01-20T09:24:32Z

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

go version go1.13.1 linux/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output

GO111MODULE=""
GOARCH="amd64"
GOBIN="/home/s/go/bin"
GOCACHE="/home/s/.cache/go-build"
GOENV="/home/s/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/s/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build184928266=/tmp/go-build -gno-record-gcc-switches"

What did you do?

According to the documentation, both setfsuid() and setfsgid() should return the previous filesystem user or group ID which is not the case in go's syscall package. The signatures in syscall package are (for all architectures):

Setfsuid(uid int) (err error)
Setfsgid(gid int) (err error)

What did you expect to see?

Correct ones would be

Setfsuid(uid int) (prev int, err error)
Setfsgid(uid int) (prev int, err error)

Right now there's no way to determine if the call succeeded or not as the err value will always be nil both on successful call and on fail.

Though it can be fixed just by adding an extra return value to the function signatures in syscall_linux_%arch%.go, it would be a breaking change.
Another way is to introduce another version of these functions with correct signatures. Linux uses that approach for its epoll functionality ( epoll_create(2) and epoll_create1(2)). But to be honest, it's even worse than breaking the backwards compatibility in my opinion, as the code will become a total mess after a few 'fixes' like that one.

What did you see instead?

Pretty much meaningless return value after a call that didn't fail due to lack of CAP_SETUID :c

The text was updated successfully, but these errors were encountered:

tklauser · 2020-01-20T10:22:12Z

I'm not sure we should change it in package syscall, though the functions are pretty much unusable as they currently are. But in any case we should fix them in golang.org/x/sys/unix (which should be used instead of syscall).

/cc @ianlancetaylor @bradfitz

shynie · 2020-01-20T10:28:40Z

Yeah, I always forget about golang.org/x/sys/unix, thanks for the comment @tklauser
If it's possible to change that behavior in unix package, I'll prepare the patch. Waiting for comments on that topic.

tklauser · 2020-01-20T10:30:38Z

@shynie Please feel free to send a patch for x/sys/unix. Regarding the syscall change I'd wait for feedback from Ian and Brad.

Thanks!

gopherbot · 2020-01-20T11:24:40Z

Change https://golang.org/cl/215240 mentions this issue: unix: fix Setfsuid and Setfsgid return values

bradfitz · 2020-01-20T15:05:10Z

I reverted the change (in https://go-review.googlesource.com/c/sys/+/215518) because it's a breaking API change and code search suggests it does break programs (and some ones that seem popular).

shynie · 2020-01-20T15:22:49Z

I left a comment on introducing a new name in my CL but as the CL's status by now is 'merged', I'll post it here as well.

Is there any naming conventions for this kind of fixes? Linux itself tends to add '1' to syscall names that would otherwise break backwards compatibility, should we do the same, thus adding something like Setfsgid1 and Setfsuid1?

Also should I create a new CL or it's possible to reopen this one?

tklauser · 2020-01-20T15:36:41Z

Maybe something like SetfsuidRetUid/SetfsgidRetGid? I don't particularly like the 1 suffix as it doesn't add any information on what the function does.

You'll have to create a new CL since the original one was already merged.

shynie · 2020-01-20T15:43:10Z

Yep, that looks better. I'll open a new CL with these changes tomorrow.

bradfitz · 2020-01-20T16:05:16Z

Please wait for @ianlancetaylor to approve any name. Ideally it should match some existing naming convention precedent. That may not be possible, in which case getting the precedent nice for such future cases is worth getting more people's thoughts on.

ianlancetaylor · 2020-01-20T19:44:38Z

I can't think of any other case where we had to add a new function to add a result. There may have been one that I'm forgetting.

I'm OK with adding SetfsuidRetUid and SetfsgidRetGid to golang.org/x/sys/unix. A better name might be Swapfsuid, but we can't expect people looking for the setfsuid system call to find that.

I'm kind of surprised this comes up; as I understand it those system calls are obsolete.

tklauser · 2020-01-21T08:22:28Z

IoctlRetInt and PrctlRetInt already exist and they are at least comparable in the way that they return the return value of the ioctl/prctl syscall (rather than just the error).

gopherbot · 2020-01-21T10:10:11Z

Change https://golang.org/cl/215577 mentions this issue: unix: add new setfsuid/setfsgid wrappers

shynie · 2020-01-21T10:19:15Z

Did it like it was done with ioctl and prctl. But there's two versions of this call (setfsuid and setfsuid32) with the latter being used in 32bit archs, so it wasn't quite possible to write a single wrapper in syscall_linux.go as in ioctl case. Not sure about using //sys syntax for things that aren't real syscalls though.

tklauser · 2020-01-21T10:22:28Z

You could define a const with the syscall numbers depending on the GOARCH which is then used in a single wrapper in syscall_linux.go. We already do this for fcntl, please see the definition of fcntl64Syscall.

gopherbot · 2020-01-21T11:14:08Z

Change https://golang.org/cl/215579 mentions this issue: unix: add new setfsuid/setfsgid wrappers

shynie · 2020-01-21T11:16:26Z

I managed to mess up the CLs and git codereview created a new one after mail. Hope that will be my last moment of shame :D

tklauser changed the title ~~syscall.Setfsuid and syscall.Setfsgid have wrong return values~~ syscall: Setfsuid and Setfsgid have wrong return values Jan 20, 2020

tklauser added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. OS-Linux labels Jan 20, 2020

gopherbot closed this as completed in golang/sys@94506bc Jan 20, 2020

bradfitz reopened this Jan 20, 2020

ianlancetaylor changed the title ~~syscall: Setfsuid and Setfsgid have wrong return values~~ x/sys/unix: Setfsuid and Setfsgid have wrong return values Jan 20, 2020

gopherbot added this to the Unreleased milestone Jan 20, 2020

tklauser added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 21, 2020

gopherbot closed this as completed in golang/sys@e047566 Jan 22, 2020

golang locked and limited conversation to collaborators Jan 21, 2021

gopherbot added the FrozenDueToAge label Jan 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/sys/unix: Setfsuid and Setfsgid have wrong return values #36649

x/sys/unix: Setfsuid and Setfsgid have wrong return values #36649

shynie commented Jan 20, 2020 •

edited

tklauser commented Jan 20, 2020

shynie commented Jan 20, 2020

tklauser commented Jan 20, 2020

gopherbot commented Jan 20, 2020

bradfitz commented Jan 20, 2020

shynie commented Jan 20, 2020

tklauser commented Jan 20, 2020

shynie commented Jan 20, 2020 •

edited

bradfitz commented Jan 20, 2020

ianlancetaylor commented Jan 20, 2020

tklauser commented Jan 21, 2020

gopherbot commented Jan 21, 2020

shynie commented Jan 21, 2020

tklauser commented Jan 21, 2020

gopherbot commented Jan 21, 2020

shynie commented Jan 21, 2020

x/sys/unix: Setfsuid and Setfsgid have wrong return values #36649

x/sys/unix: Setfsuid and Setfsgid have wrong return values #36649

Comments

shynie commented Jan 20, 2020 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

tklauser commented Jan 20, 2020

shynie commented Jan 20, 2020

tklauser commented Jan 20, 2020

gopherbot commented Jan 20, 2020

bradfitz commented Jan 20, 2020

shynie commented Jan 20, 2020

tklauser commented Jan 20, 2020

shynie commented Jan 20, 2020 • edited

bradfitz commented Jan 20, 2020

ianlancetaylor commented Jan 20, 2020

tklauser commented Jan 21, 2020

gopherbot commented Jan 21, 2020

shynie commented Jan 21, 2020

tklauser commented Jan 21, 2020

gopherbot commented Jan 21, 2020

shynie commented Jan 21, 2020

shynie commented Jan 20, 2020 •

edited

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

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

shynie commented Jan 20, 2020 •

edited