search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.96k stars 17.66k forks source link

os/user: add illumos getgroups support #14709

Open bradfitz opened 8 years ago

bradfitz commented 8 years ago

Solaris is broken after the os/user groups change:

http://build.golang.org/log/26a14fbc0126ab1ed65eaeffe18362689b13a54f

...
net
os/user
# os/user
Undefined           first referenced
 symbol                 in file
getgrouplist                        $WORK/os/user/_obj/lookup_unix.cgo2.o
ld: fatal: symbol referencing errors. No output written to $WORK/os/user/_obj/_cgo_.o
collect2: error: ld returned 1 exit status
crypto/x509
net/textproto
log/syslog
...

Let me know if you need help testing on Solaris. It's not yet a trybot, due to lack of time.

bradfitz commented 8 years ago

Apparently Solaris didn't have getgrouplist until recently. I found elsewhere that "getgrouplist() is provided by the latest Solaris 11.2 SRU 11.2.10.5.0, released 15 May 2015. It will be available in all future updates/releases." But apparently our builders don't have that.

@4ad, advice?

bradfitz commented 8 years ago

Actually, we should just not try to build it & test it. I'll send out a CL. We can keep this bug open to add support later.

bradfitz commented 8 years ago

Or rather, I'll wait for @zombiezen to finish his pending CLs, since my change would necessarily split lookup_unix.go into two halves probably (user lookup vs group lookup, with different build tags), or maybe add a group_solaris.go file with an import "C" and a fake getgrouplist impl in it just for Solaris.

Ross, let me know if you prefer to tackle this.

4ad commented 8 years ago

We will have to implement getgrouplist for Solaris ourselves. In the meantime we can stub it.

zombiezen commented 8 years ago

I can roll in the stub as part of https://golang.org/cl/20348, since I'm already splitting out the OS-specific functionality.

bradfitz commented 8 years ago

Great, thanks.

gopherbot commented 8 years ago

CL https://golang.org/cl/20348 mentions this issue.

zombiezen commented 8 years ago

Just to point out, the other group-related functionality should still work on Solaris (let me know if it doesn't). getgrouplist is annoyingly non-standard. I'm contemplating whether the internal listGroups function should just parse /etc/groups itself in the future because it will ironically be more portable.

bradfitz commented 8 years ago

Parsing /etc/groups as a fallback seems fine too.

bradfitz commented 8 years ago

Repurposing this bug to add Solaris support, since this is the bug number references in the t.Skip text now.

binarycrusader commented 8 years ago

Some versions of Solaris support this function, so ideally, if there is a sanctioned way to see if we can import the symbol from libc, and if it's nil, then fallback to the other functionality, that seems like the best option.

minux commented 8 years ago

use dlsym?

ideally we should use weak symbols, but our linker doesn't support that.

ianlancetaylor commented 8 years ago

We are talking about cgo here, and my understanding is that Solaris doesn't support internal linking anyhow. So I think we could use a weak reference in the C code.

binarycrusader commented 8 years ago

The internal linker works just fine on Solaris. Not sure I understand?

ianlancetaylor commented 8 years ago

Sorry, I expect it is me that does not understand.

binarycrusader commented 8 years ago

I guess I was assuming that cgo import dynamic would simply assign nil for symbols it can't find:

//go:cgo_import_dynamic libc_Getgrouplist getgrouplist "libc.so"

My assumption was we could just check for nil in the wrapper function, if not nil, call the function, and if nil, fallback to other logic.

But I don't know if that's possible.

minux commented 8 years ago

A little clarification.

The current os/user on other Unix platforms uses cgo to access libc functions, however, on Solaris, every program is dynamic linked, and even pure Go program to access libc functions by using //go:cgo_import_dynamic.

That is, pure Go programs on Solaris will use internal linking, but are still able to call arbitrary libc functions. My comment about lack of weak symbols is primarily about this case.

Also note that solaris cgo also supports internal linking, so I'm afraid we can't use weak symbols if still want to be able to internal linking on programs that uses os/user.

In summary, I propose we just use dlsym at runtime.

matejb commented 7 years ago

Stumbled on this issue yesterday while working on the application for Illumnos based OS. And made the in-app version of the function that parses /etc/group.

And I see there are existing private functions for /etc/group parsing inside user package. I can use these to make User.GroupIds work for Solaris. If that would be acceptable?

4ad commented 7 years ago

No, we need to do what @minux said in his last reply.

andy-js commented 4 years ago

I'm not sure why nobody suggested implementing this using getgrent. There's no actual need to have getgrouplist.

andy-js commented 4 years ago

See: https://github.com/golang/go/pull/40667

gopherbot commented 3 years ago

Change https://golang.org/cl/315278 mentions this issue: os/user: implement (*User).GroupIds on solaris

tklauser commented 3 years ago

https://golang.org/cl/315278 added support for solaris, repurposing this issue to add support on illumos.

kolyshkin commented 3 years ago

https://go-review.googlesource.com/c/go/+/330753 implements /etc/passwd parsing, and unconditionally enables that for illumos. As far as I understand this lands in Go 1.18.

Ideally, what's needed (on top of the above, for Illumos) is a dynamic check if getgrouplist libc function is available, and using it.

gopherbot commented 3 years ago

Change https://golang.org/cl/330753 mentions this issue: os/user: implement go native GroupIds