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

Jump to bottom

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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.10

Comments

@rfjakob
Copy link

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:

go/src/syscall/syscall_linux_mips64x.go

Line 13 in bcc86d5

// 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

go/src/syscall/ztypes_linux_mips64le.go

Line 133 in bcc86d5

// 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 commented Jan 30, 2018 •
edited

CC @minux @cherrymui

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Jan 30, 2018
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Jan 30, 2018
@rfjakob rfjakob mentioned this issue Jan 30, 2018
Closed
@rfjakob
Copy link
Author

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
Member

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
Member

x/sys/unix also uses getdents64 on MIPS64.

@ianlancetaylor
Copy link
Contributor

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 commented Jan 30, 2018 via email

@tklauser
Copy link
Member

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

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
@rfjakob
syscallcompat: switch from syscall.Getdents to unix.Getdents
26ba810 
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
@lechner
Copy link

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

@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 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

@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 commented Jan 31, 2018

@bradfitz Thank you!

rfjakob added a commit to rfjakob/go-fuse that referenced this issue Jan 31, 2018
@rfjakob
Switch from syscall.Getdents to unix.Getdents
c0c2c6c 
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
@rfjakob
Switch from syscall.Getdents to unix.Getdents
19acbd2 
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.
@rfjakob rfjakob mentioned this issue Jan 31, 2018
Merged
hanwen pushed a commit to hanwen/go-fuse that referenced this issue Feb 2, 2018
@rfjakob @hanwen
Switch from syscall.Getdents to unix.Getdents
9060525 
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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
8 participants
@bradfitz @minux @lechner @rfjakob @tklauser @ianlancetaylor @gopherbot @cherrymui