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: wrong Dirent struct on mips64le #23624

Closed
rfjakob opened this issue Jan 30, 2018 · 13 comments
Closed

syscall: wrong Dirent struct on mips64le #23624

rfjakob opened this issue Jan 30, 2018 · 13 comments

Comments

@rfjakob
Copy link

@rfjakob rfjakob commented Jan 30, 2018

The maintainer of the Debian package just reported that gocryptfs is broken on mips64le: rfjakob/gocryptfs#200 . The reason seems to be that syscall.Dirent does not match the used getdents syscall. Analysis below:

On mips64le, getdents is used instead of getdents64:

// Linux introduced getdents64 syscall for N64 ABI only in 3.10

	_SYS_getdents  = SYS_GETDENTS

And getdents returns linux_dirent, which defined in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/readdir.c?h=v4.15#n147 :

struct linux_dirent {
	unsigned long	d_ino;
	unsigned long	d_off;
	unsigned short	d_reclen;
	char		d_name[1];
};

In Go mips64le, syscall.Dirent is defined here

// Note: on mips64, we're using the getdents syscall,
:

type Dirent struct {
	Ino       uint64
	Off       int64
	Reclen    uint16
	Name      [256]int8
	Type      uint8
	Pad_cgo_0 [5]byte
}

And this looks wrong, because it has a Type field, which the the linux_dirent struct has not.

@ianlancetaylor ianlancetaylor changed the title Wrong syscall.Dirent struct on mips64le syscall: wrong Dirent struct on mips64le Jan 30, 2018
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 30, 2018

@rfjakob
Copy link
Author

@rfjakob rfjakob commented Jan 30, 2018

PS: I believe this only affects people who parse the Getdents output themselves. gocryptfs does for performance, so does https://github.com/golang/tools/blob/master/imports/fastwalk_unix.go , but probably not much else. The fix for gocryptfs will be to switch over to unix.Getdents from x/sys/unix, which does not have this problem.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jan 30, 2018

We should probably move to use getdents64 on MIPS64, just like all other architectures. At the time the code was written, getdents64 was rather new in MIPS64 Linux kernel. But it is not new anymore, and we probably do require newer kernels.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jan 30, 2018

x/sys/unix also uses getdents64 on MIPS64.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 30, 2018

I would certainly encourage anybody who wants to call Getdents themselves to use golang.org/x/sys/unix rather than syscall. But of course syscall should at least be internally consistent.

@minux
Copy link
Member

@minux minux commented Jan 30, 2018

@tklauser
Copy link
Member

@tklauser tklauser commented Jan 31, 2018

Yeah, I think we can fix this (with a api-breaking change) now as 3.10 is
almost 6 years old.

According to https://github.com/golang/go/wiki/MinimumRequirements the minimum kernel version required for linux/mips64le is 4.8, so that shouldn't be a problem.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 31, 2018

Change https://golang.org/cl/91055 mentions this issue: syscall: use SYS_GETDENTS64 on linux/mips64{,le}

rfjakob added a commit to rfjakob/gocryptfs that referenced this issue Jan 31, 2018
On mips64le, syscall.Getdents() and struct syscall.Dirent do
not fit together, causing our Getdents implementation to
return garbage ( #200
and golang/go#23624 ).

Switch to unix.Getdents which does not have this problem -
the next Go release with the syscall package fixes is too
far away, and will take time to trickle into distros.
@bradfitz bradfitz modified the milestones: Go1.11, Go1.10 Jan 31, 2018
@gopherbot gopherbot closed this in d929e40 Jan 31, 2018
@lechner
Copy link

@lechner lechner commented Jan 31, 2018

Hi, we use syscall in other places as well. Older documentation encourages external packages to use x/sys/unix instead. Is the recommendation still valid? Thank you!

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 31, 2018

@lechner, x/sys/unix is gets new stuff and is updated more regularly. You can use syscall if it meets your needs, but it doesn't change often (ideally never), and when it does it's tied to releases. So using x/sys/unix is almost always a better option.

@lechner
Copy link

@lechner lechner commented Jan 31, 2018

@bradfitz, which module would you recommend for Go-Fuse, a package that provides the filesystem-in-userspace (FUSE) bindings used by gocryptfs, please? Thank you!

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 31, 2018

@lechner, whatever the Go-Fuse authors are using is fine, if their code is working. If they hit problems and they're using syscall, the answer might be to not use syscall.

@lechner
Copy link

@lechner lechner commented Jan 31, 2018

@bradfitz Thank you!

rfjakob added a commit to rfjakob/go-fuse that referenced this issue Jan 31, 2018
On mips64le, syscall.Getdents() and struct syscall.Dirent do
not fit together, causing our parseDirents() implementation to
return garbage ( see rfjakob/gocryptfs#200
and golang/go#23624 ).

Switch to unix.Getdents which does not have this problem -
the next Go release with the syscall package fixes is too
far away, and will take time to trickle into distros.
rfjakob added a commit to rfjakob/go-fuse that referenced this issue Jan 31, 2018
On mips64le, syscall.Getdents() and struct syscall.Dirent do
not fit together, causing our parseDirents() implementation to
return garbage ( see rfjakob/gocryptfs#200
and golang/go#23624 ).

Switch to unix.Getdents which does not have this problem -
the next Go release with the syscall package fixes is too
far away, and will take time to trickle into distros.

This issue has been reported by Debian maintainer
Felix Lechner.
hanwen added a commit to hanwen/go-fuse that referenced this issue Feb 2, 2018
On mips64le, syscall.Getdents() and struct syscall.Dirent do
not fit together, causing our parseDirents() implementation to
return garbage ( see rfjakob/gocryptfs#200
and golang/go#23624 ).

Switch to unix.Getdents which does not have this problem -
the next Go release with the syscall package fixes is too
far away, and will take time to trickle into distros.

This issue has been reported by Debian maintainer
Felix Lechner.
@golang golang locked and limited conversation to collaborators Jan 31, 2019
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
8 participants
You can’t perform that action at this time.