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

os/user: TestGroupIds on macOS fails #21067

Closed
nadiasvertex opened this issue Jul 18, 2017 · 13 comments
Closed

os/user: TestGroupIds on macOS fails #21067

nadiasvertex opened this issue Jul 18, 2017 · 13 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Milestone

Comments

@nadiasvertex
Copy link
Contributor

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

go version go1.8.3 darwin/amd64

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

$ uname -a
Darwin USD25MC10EF8JC 16.4.0 Darwin Kernel Version 16.4.0: Thu Dec 22 22:53:21 PST 2016; root:xnu-3789.41.3~3/RELEASE_X86_64 x86_64

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/csnelson/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/74/y_rffr315mn3j9d5v0q_8w9hwlsb1s/T/go-build117999026=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Building the latest Go Compiler source (from source control):

$ ./all.bash

What did you expect to see?

Success.

What did you see instead?

ok  	os/signal	4.480s
--- FAIL: TestGroupIds (0.00s)
	user_test.go:143: &{Uid:960277561 Gid:1367992244 Username:csnelson Name:Nelson HomeDir:/Users/csnelson}.GroupIds(): user: list groups for csnelson failed (changed groups?)
FAIL
FAIL	os/user	0.034s
@jessfraz jessfraz changed the title TestGroupIds on macOS fails os/user: TestGroupIds on macOS fails Jul 18, 2017
@kevinburke kevinburke self-assigned this Jul 18, 2017
@kevinburke
Copy link
Contributor

kevinburke commented Jul 18, 2017

When you say "Building the latest Go Compiler source (from source control)" were you trying to build the source for Go 1.8.3, or the source for tip? If tip, which commit?

At the top of the all.bash output, it should say "Building Go toolchain using (x)", what is x when you get this failure?

@kevinburke
Copy link
Contributor

kevinburke commented Jul 18, 2017

What happens when you run "groups csnelson" at the command line? It looks like this is the affected code in listgroups_unix.go

	rv := getGroupList(nameC, userGID, &gidsC[0], &n)
	if rv == -1 {
		// More than initial buffer, but now n contains the correct size.
		const maxGroups = 2048
		if n > maxGroups {
			return nil, fmt.Errorf("user: list groups for %s: member of more than %d groups", u.Username, maxGroups)
		}
		gidsC = make([]C.gid_t, n)
		rv := getGroupList(nameC, userGID, &gidsC[0], &n)
		if rv == -1 {
			return nil, fmt.Errorf("user: list groups for %s failed (changed groups?)", u.Username)
		}
	}

@kevinburke
Copy link
Contributor

kevinburke commented Jul 18, 2017

I can't reproduce this one at the moment, so hoping we can find out what's the difference between the way that I'm running it and the way that you're running it.

@nadiasvertex
Copy link
Contributor Author

nadiasvertex commented Jul 19, 2017

@kevinburke I am building tip.
$ git log
commit e9b9dfe
Author: Alexey Palazhchenko alexey.palazhchenko@gmail.com
Date: Tue Jul 18 07:18:35 2017 +0300

$ ./all.bash

Building Go bootstrap tool.

cmd/dist

Building Go toolchain using /usr/local/go.

$ /usr/local/go/bin/go version
go version go1.8.3 darwin/amd64

Running groups csnelson comes back with almost 300 space separated names, which output I probably cannot post here for security reasons.

@kevinburke
Copy link
Contributor

can you try editing the code in listgroups_unix.go as follows, and rerun all.bash and see what we get? I don't know how to debug this otherwise as it's tough for me to reproduce.

The gidsC output is not important - I don't want you to reveal the groups - but I would like to know if the list has empty entries or full entries.

	rv := getGroupList(nameC, userGID, &gidsC[0], &n)
	if rv == -1 {
		fmt.Println("running get group list fallback, n:", n)
		// More than initial buffer, but now n contains the correct size.
		const maxGroups = 2048
		if n > maxGroups {
			return nil, fmt.Errorf("user: list groups for %s: member of more than %d groups", u.Username, maxGroups)
		}
		gidsC = make([]C.gid_t, n)
		rv := getGroupList(nameC, userGID, &gidsC[0], &n)
		fmt.Printf("len gidsC: %d, gidsC: %#v\n", len(gidsC), gidsC)
		fmt.Println("new n:", n)
		if rv == -1 {
			return nil, fmt.Errorf("user: list groups for %s failed (changed groups?)", u.Username)
		}
	}

@nadiasvertex
Copy link
Contributor Author

Sure, that's no problem:

running get group list fallback, n: 256
len gidsC: 256, gidsC: []user._Ctype_gid_t{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}
new n: 256
--- FAIL: TestGroupIds (0.00s)
	user_test.go:143: &{Uid:960277561 Gid:1367992244 Username:csnelson Name:Nelson HomeDir:/Users/csnelson}.GroupIds(): user: list groups for csnelson failed (changed groups?)
FAIL
FAIL	os/user	0.043s

@kevinburke
Copy link
Contributor

Interesting. So getgrouplist (the C call) is supposed to populate N with the number of groups to fetch. You say that list has 300 members, but getgrouplist only returns 256; I'm guessing that it's returning -1 because we can't handle more than 256 members in a group...

Now the question is whether that's a C constraint or whether that's a Go constraint.

@kevinburke
Copy link
Contributor

Oh, huh. 256 is the initial value for n, so maybe it's not getting changed at all by the initial getGroupList call.

Here's the manpage for getgrouplist:

RETURN VALUES
     The getgrouplist() function returns 0 on success.  If the size of the group list is too small to hold all the user's groups, getgrouplist() returns -1 to
     indicate failure.  In this case, the group array will be filled with as many groups as will fit.

Compare that with the Linux getgrouplist description:

RETURN VALUE
       If the number of groups of which user is a member is less than or equal to *ngroups, then the value *ngroups is returned.

       If the user is a member of more than *ngroups groups, then getgrouplist() returns -1.  In this case, the value returned in *ngroups can be  used  to  resize  the  buffer
       passed to a further call getgrouplist().

So maybe on Macs, getgrouplist does not set n correctly if there are more groups than in the array? What happens if you change the C.int(256) to, say, C.int(1000), does it work?

@nadiasvertex
Copy link
Contributor Author

It does in fact work fine if I set c.int(1000)

@nadiasvertex
Copy link
Contributor Author

nadiasvertex commented Jul 20, 2017

The following patch fixes the problem for me.

diff --git a/src/os/user/listgroups_unix.go b/src/os/user/listgroups_unix.go
index db952c6..772b96d 100644
--- a/src/os/user/listgroups_unix.go
+++ b/src/os/user/listgroups_unix.go
@@ -28,21 +28,31 @@ func listGroups(u *User) ([]string, error) {
        nameC := C.CString(u.Username)
        defer C.free(unsafe.Pointer(nameC))
 
-       n := C.int(256)
+       array_size := 256
+       n := C.int(array_size)
        gidsC := make([]C.gid_t, n)
        rv := getGroupList(nameC, userGID, &gidsC[0], &n)
-       if rv == -1 {
+       for rv == -1 {
+               fmt.Println("running get group list fallback, n:", n)
+               if n != C.int(array_size) {
+                       array_size = int(n)
+               } else {
+                       array_size *= 2
+               }
+
                // More than initial buffer, but now n contains the correct size.
                const maxGroups = 2048
-               if n > maxGroups {
+               if array_size > maxGroups {
                        return nil, fmt.Errorf("user: list groups for %s: member of more than %d groups", u.Username, maxGroups)
                }
+
+               n = C.int(array_size)
                gidsC = make([]C.gid_t, n)
-               rv := getGroupList(nameC, userGID, &gidsC[0], &n)
-               if rv == -1 {
-                       return nil, fmt.Errorf("user: list groups for %s failed (changed groups?)", u.Username)
-               }
+               rv = getGroupList(nameC, userGID, &gidsC[0], &n)
+               fmt.Printf("len gidsC: %d, gidsC: %#v\n", len(gidsC), gidsC)
+               fmt.Println("new n:", n)
        }
+
        gidsC = gidsC[:n]
        gids := make([]string, 0, n)
        for _, g := range gidsC[:n] {

@bradfitz bradfitz added NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin labels Jul 20, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Jul 20, 2017
@kevinburke
Copy link
Contributor

(FYI, it's easy to reproduce this by changing 256 to, say, 2).

@kevinburke
Copy link
Contributor

@nadiasvertex Thanks for sending the patch, I'll tweak it and submit it shortly.

@gopherbot
Copy link

Change https://golang.org/cl/51892 mentions this issue: os/user: fix darwin GetGroupIds for n > 256

@golang golang locked and limited conversation to collaborators Nov 22, 2018
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. OS-Darwin
Projects
None yet
Development

No branches or pull requests

4 participants