/snapd

cmd/snap-update-ns: create missing mount points automatically. #4008

Merged
merged 46 commits into from Oct 27, 2017
Commits
Jump to file or symbol
Failed to load files and symbols.
+7 −16
Split
Viewing a subset of changes. View all
Prev Next

cmd/snap-update-ns: use defer instead of hand-held close 

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
  • Loading branch information...
commit 1a4d21279aedfbad71357ee031cca068af8e4850 @zyga zyga committed Oct 25, 2017
View
4 cmd/snap-update-ns/change_test.go
@@ -237,9 +237,9 @@ func (s *changeSuite) TestPerformMountAutomaticMkdirTarget(c *C) {
`open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`,
`mkdirat 3 "target" 0755`,
`openat 3 "target" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`,
- `close 3`,
`fchown 4 0 0`,
`close 4`,
+ `close 3`,
`mount "/source" "/target" "type" 0 ""`,
})
}
@@ -257,9 +257,9 @@ func (s *changeSuite) TestPerformMountAutomaticMkdirSource(c *C) {
`open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`,
`mkdirat 3 "source" 0755`,
`openat 3 "source" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`,
- `close 3`,
`fchown 4 0 0`,
`close 4`,
+ `close 3`,
`mount "/source" "/target" "type" MS_BIND ""`,
})
}
View
13 cmd/snap-update-ns/utils.go
@@ -76,6 +76,7 @@ func SecureMkdirAllImpl(name string, perm os.FileMode, uid, gid int) error {
if err != nil {
return fmt.Errorf("cannot open root directory: %v", err)
}
@mvo5

mvo5 Oct 25, 2017

Collaborator

Don't we need a defer sysClose(fd) here?

@zyga

zyga Oct 25, 2017

Contributor

Indeed, let me rework this.
+ defer sysClose(fd)
// Split the path by entries and create each element using mkdirat() using
// the parent directory as reference. Each time we open the newly created
@@ -93,21 +94,14 @@ func SecureMkdirAllImpl(name string, perm os.FileMode, uid, gid int) error {
case syscall.EEXIST:
made = false
default:
- if err := sysClose(fd); err != nil {
- return fmt.Errorf("cannot close file descriptor %d: %v", fd, err)
- }
return fmt.Errorf("cannot mkdir path segment %q: %v", segment, err)
}
}
- previousFd := fd
-
fd, err = sysOpenat(fd, segment, openFlags, 0)
@mvo5

mvo5 Oct 25, 2017

Collaborator

Why do we keep previousFd instead of using defer sysClose(fd) ?

@zyga

zyga Oct 25, 2017

Contributor

See below.
- if err := sysClose(previousFd); err != nil {
- return fmt.Errorf("cannot close previous file descriptor %d: %v", fd, err)
- }
if err != nil {
@mvo5

mvo5 Oct 25, 2017

Collaborator

Why do we have the sysClose() between this error check? The error is for the err in 101 so it feels harder to read this way than if the err check is followed immediately after the sysOpenat(). Maybe defer sysClose() would help to make this easier to read?

@zyga

zyga Oct 25, 2017

Contributor

The code is designed to ensure that we only keep two file descriptors around. The one for the parent directory and the one for the entry inside it. Everything is paired correctly.

As for defer, we could do that but it would mean we need to keep more file descriptors open for no good reason.
return fmt.Errorf("cannot open path segment %q: %v", segment, err)
}
+ defer sysClose(fd)
@chipaca

chipaca Oct 25, 2017

Member

I'm a little bit unhappy about this approach. It's probably not worth worrying about, but I worry: this uses a lot more filehandles than necessary. On the other hand, doing the cleanup by hand, even with a helper, is not obviously right to the reader. On the other other hand, we're ignoring all the close errors. Sigh.

@zyga

zyga Oct 25, 2017

Contributor

Look at the history, one of the most recent patches introduces defer, if we pop that we're back to hand-done cleanup logic that only uses a pair of FDs
if made {
// Chown each segment that we made.
if err := sysFchown(fd, uid, gid); err != nil {
@@ -116,9 +110,6 @@ func SecureMkdirAllImpl(name string, perm os.FileMode, uid, gid int) error {
}
}
- if err = sysClose(fd); err != nil {
- return fmt.Errorf("cannot close file descriptor %d: %v", fd, err)
- }
return nil
@chipaca

chipaca Oct 25, 2017

Member

shouldn't we sync everything to disk on success?

@zyga

zyga Oct 25, 2017

Contributor

No, this is not a requirement of this operation.
}
View
6 cmd/snap-update-ns/utils_test.go
@@ -60,12 +60,12 @@ func (s *utilsSuite) TestSecureMkdirAllAbsolute(c *C) {
`open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`,
`mkdirat 3 "abs" 0755`,
`openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`,
- `close 3`,
`fchown 4 123 456`,
`mkdirat 4 "path" 0755`,
`openat 4 "path" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`,
+ `fchown 5 123 456`,
+ `close 5`,
`close 4`,
- `fchown 3 123 456`,
`close 3`,
})
}
@@ -80,9 +80,9 @@ func (s *utilsSuite) TestSecureMkdirAllExistingDirsDontChown(c *C) {
`open "/" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`,
`mkdirat 3 "abs" 0755`,
`openat 3 "abs" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`,
- `close 3`,
`mkdirat 4 "path" 0755`,
`openat 4 "path" O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY 0`,
+ `close 5`,
`close 4`,
`close 3`,
})