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: Getdirentries needs a 64-bit base #32498

Closed
randall77 opened this issue Jun 8, 2019 · 6 comments
Closed

syscall: Getdirentries needs a 64-bit base #32498

randall77 opened this issue Jun 8, 2019 · 6 comments

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Jun 8, 2019

Currently the signature of syscall.Getdirentries is this:

func Getdirentries(fd int, buf []byte, basep *uintptr) (n int, err error)

Unfortunately, on freebsd12, the system call used to implement this writes 64 bits to *basep.
On 32-bit platforms, this causes the system call to overwrite the word at basep+4, which can be a random stack slot, causing badness.

On freebsd11, the manpage says:

int getdirentries(int fd, char *buf, int nbytes, long *basep);

On freebsd12, the manpage says:

int getdirentries(int fd, char *buf, size_t nbytes, off_t *basep);

off_t is 64 bits even on 32 bit platforms.

So at least on freebsd, we need to change the signature to:

func Getdirentries(fd int, buf []byte, basep *uint64) (n int, err error)

I think we should change it everywhere (freebsd, netbsd, openbsd, dragonfly, darwin), just for consistency.
This will be a breaking change. Fortunately it is in package syscall.

The other option to avoid a breaking change is to have the freebsd 12 implementation allocate a uint64, copy from *basep to the low word of the uint64, pass the address of the uint64 to the system call, copy the low word of the uint64 back to *basep, and then check and return an error if the high word of the uint64 is not zero (it seems to be a virtual file offset, so it usually has a zero high part).

I suspect we'll encounter this more frequently in the future, it might be worth biting the bullet now.

Update #6980

Thoughts? @iant @rsc

We've run into this before, perhaps on Darwin? The following comment is in syscall.ReadDirent where it calls out to syscall.Getdirentries:

// Final argument is (basep *uintptr) and the syscall doesn't take nil.
// 64 bits should be enough. (32 bits isn't even on 386). Since the
// actual system call is getdirentries64, 64 is a good guess.

So someone (I think @rsc or @r) found this a long time ago. So ReadDirent has been fixed but the underlying Getdirentries hasn't.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 8, 2019

Change https://golang.org/cl/181377 mentions this issue: syscall: fix Getdirentries on 32-bit freebsd 12

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 8, 2019

Change https://golang.org/cl/181378 mentions this issue: Revert "Revert "cmd/compile,runtime: allocate defer records on the stack""

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Jun 8, 2019

I implemented the second option as a CL. Mostly just so I could reinstate the stack-allocated defer CL which depends on this issue being fixed.

@paulzhol
Copy link
Member

@paulzhol paulzhol commented Jun 9, 2019

I think we should change it everywhere (freebsd, netbsd, openbsd, dragonfly, darwin), just for consistency.
This will be a breaking change. Fortunately it is in package syscall.

FreeBSD 12 switched to 64-bit inodes, I missed the long => off_t change here.

OpenBSD dropped getdirentries in 5.5:

Replaced getdirentries(2) with getdents(2), vastly improving the performance and memory usage of telldir(3).

NetBSD still uses long *basep, and it is marked deprecated in the manpage:

This interface is provided for compatibility only and has been obsoleted by getdents(2).

On both OpenBSD and NetBSD, we ignore basep is altogether:

//sys getdents(fd int, buf []byte) (n int, err error)
func Getdirentries(fd int, buf []byte, basep *uintptr) (n int, err error) {
	return getdents(fd, buf)
}

Dragonfly also has a long *basep parameter, but the BUGS section is very instructive:

Unfortunately, *basep is only 32 bits wide on 32 bit platforms and may
not be wide enough to accommodate the directory position cookie. Modern
users should use lseek(2). to retrieve and set the seek position within
the directory. The seek offset is 64 bits wide on all platforms.

@gopherbot gopherbot closed this in daf944a Jun 10, 2019
gopherbot pushed a commit that referenced this issue Jun 10, 2019
…ack""

This reverts CL 180761

Reason for revert: Reinstate the stack-allocated defer CL.

There was nothing wrong with the CL proper, but stack allocation of defers exposed two other issues.

Issue #32477: Fix has been submitted as CL 181258.
Issue #32498: Possible fix is CL 181377 (not submitted yet).

Change-Id: I32b3365d5026600069291b068bbba6cb15295eb3
Reviewed-on: https://go-review.googlesource.com/c/go/+/181378
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 10, 2019

Change https://golang.org/cl/181500 mentions this issue: x/sys/unix: fix Getdirentries on 32-bit freebsd 12

gopherbot pushed a commit to golang/sys that referenced this issue Jun 10, 2019
On freebsd 12, the system call for getdirentries writes 64 bits to
*basep, even on 32-bit systems. Accomodate that by providing a uint64
to the system call and copy the base to/from that uint64.
The uint64 seems to be a virtual file offset, so failing if the high
bits are not zero should be fine for reasonable-sized directories.

Update golang/go#32498

Change-Id: I4451894aff4e353c9f009c06ad2fdd5578dfd9f8
Reviewed-on: https://go-review.googlesource.com/c/sys/+/181500
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 15, 2019

Change https://golang.org/cl/182319 mentions this issue: unix: fix Getdirentries emulation using Getdents on netbsd, openbsd

gopherbot pushed a commit to golang/sys that referenced this issue Jun 16, 2019
Call Seek if basep is not nil to read the current dir offset.
Return EIO error if the offset doesn't fit into a 32-bit uintptr.
Make Getdents public.

Update golang/go#32498

Change-Id: Idfbc48d3fc3a6cc8a979242681e8882d39998285
Reviewed-on: https://go-review.googlesource.com/c/sys/+/182319
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
@golang golang locked and limited conversation to collaborators Jun 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.