Use *at() syscalls for Rmdir, Chmod, Chown and Mkdir #179
Conversation
Fixes the same problem as described in 72b9758, except for directories instead of device nodes.
| return syscall.Unlinkat(dirfd, path) | ||
| // Unlinkat syscall. In old versions the 'flags' argument was missing, so | ||
| // manually call it by using the corresponding syscall number. | ||
| func Unlinkat(dirfd int, path string, flags int) (err error) { |
There was a problem hiding this comment.
The syscall itself? Or do you mean tests which use Rmdir in a FUSE mount?
There was a problem hiding this comment.
Yes the syscall itself.
The Go stdlib uses https://golang.org/src/syscall/mksyscall.pl to autogenerate the wrappers, and different prototypes for amd64 ( https://golang.org/src/syscall/syscall_linux_amd64.go ) and the rest ( https://golang.org/src/syscall/syscall_linux.go ). Looks hard enough to get right (on 32 bit and 64 bit!) to warrant a test
There was a problem hiding this comment.
(I will add a test for syscallat as well)
| @@ -24,22 +24,22 @@ import ( | |||
|
|
|||
| const dsStoreName = ".DS_Store" | |||
|
|
|||
| func (fs *FS) mkdirWithIv(cPath string, mode uint32) error { | |||
| func (fs *FS) mkdirWithIv(dirfd *os.File, cName string, mode uint32) error { | |||
There was a problem hiding this comment.
Needs a comment explaining the arguments
| @@ -75,16 +75,17 @@ func fdReadDirIV(fd *os.File) (iv []byte, err error) { | |||
| // WriteDirIV - create diriv file inside "dir" (absolute ciphertext path) | |||
| // This function is exported because it is used from pathfs_frontend, main, | |||
| // and also the automated tests. | |||
| func WriteDirIV(dir string) error { | |||
| func WriteDirIV(dirfd *os.File, dir string) error { | |||
There was a problem hiding this comment.
Function comment needs to be updated and dirfd explained.
Question: If dirfd != nil, can "dir" contain slashes? Should we check + panic?
There was a problem hiding this comment.
OK
It wouldn't be a problem if it contains slashes. Not sure if we should be too strict here.
There was a problem hiding this comment.
It would work because the syscall accepts it, but is it a bug in gocryptfs if that happens?
|
I'm going to make the darwin syscalls testable on linux, and I don't want to cause merge conflicts, so I'll just pull this now. My nitpicks can be fixed later. Thanks! |
|
As I see you have already pushed your changes, looks awesome! :) |
Patch 1 fixes an existing TODO in the code.
Patch 2 and 3 changes the implementation of Chmod and Chown, such that
openBackingPathand *at() syscalls are used. The general idea would be that in the long term, we try to useopenBackingPatheverywhere. When we later want to harden gocryptfs against symlink race-conditions (maybe optional, since it probably will be a bit slower), it is sufficient to change only a single function, instead of adding checks everywhere in the code.Patch 4 should fix the remaining issue from #177.