Merge branch 'master' into remove-old-connremove-old-conn

13 files changed, 187 insertions, 30 deletions

diff --git a/cmd/libsnap-confine-private/mount-opt-test.c b/cmd/libsnap-confine-private/mount-opt-test.c
index aff7afee07..440d62a676 100644
--- a/cmd/libsnap-confine-private/mount-opt-test.c
+++ b/cmd/libsnap-confine-private/mount-opt-test.c
@@ -275,6 +275,50 @@ static void test_sc_do_umount(gconstpointer snap_debug)
	}
 }
 
+static bool missing_mount(struct sc_fault_state *state, void *ptr)
+{
+	errno = ENOENT;
+	return true;
+}
+
+static void test_sc_do_optional_mount_missing(void)
+{
+	sc_break("mount", missing_mount);
+	bool ok = sc_do_optional_mount("/foo", "/bar", "ext4", MS_RDONLY, NULL);
+	g_assert_false(ok);
+	sc_reset_faults();
+}
+
+static void test_sc_do_optional_mount_failure(gconstpointer snap_debug)
+{
+	if (g_test_subprocess()) {
+	sc_break("mount", broken_mount);
+	if (GPOINTER_TO_INT(snap_debug) == 1) {
+	g_setenv("SNAP_CONFINE_DEBUG", "1", true);
+	}
+	(void)sc_do_optional_mount("/foo", "/bar", "ext4", MS_RDONLY,
+ NULL);
+
+	g_test_message("expected sc_do_mount not to return");
+	sc_reset_faults();
+	g_test_fail();
+	return;
+	}
+	g_test_trap_subprocess(NULL, 0, 0);
+	g_test_trap_assert_failed();
+	if (GPOINTER_TO_INT(snap_debug) == 0) {
+	g_test_trap_assert_stderr
+ ("cannot perform operation: mount -t ext4 -o ro /foo /bar: Permission denied\n");
+	} else {
+	/* with snap_debug the debug output hides the actual mount commands *but*
+ * they are still shown if there was an error
+ */
+	g_test_trap_assert_stderr
+ ("DEBUG: performing operation: (disabled) use debug build to see details\n"
+ "cannot perform operation: mount -t ext4 -o ro /foo /bar: Permission denied\n");
+	}
+}
+
 static void __attribute__ ((constructor)) init(void)
 {
	g_test_add_func("/mount/sc_mount_opt2str", test_sc_mount_opt2str);
@@ -288,4 +332,12 @@ static void __attribute__ ((constructor)) init(void)
 GINT_TO_POINTER(1), test_sc_do_mount);
	g_test_add_data_func("/mount/sc_do_umount_with_debug",
 GINT_TO_POINTER(1), test_sc_do_umount);
+	g_test_add_func("/mount/sc_do_optional_mount_missing",
+	test_sc_do_optional_mount_missing);
+	g_test_add_data_func("/mount/sc_do_optional_mount_failure",
+ GINT_TO_POINTER(0),
+ test_sc_do_optional_mount_failure);
+	g_test_add_data_func("/mount/sc_do_optional_mount_failure_with_debug",
+ GINT_TO_POINTER(1),
+ test_sc_do_optional_mount_failure);
 }
diff --git a/cmd/libsnap-confine-private/mount-opt.c b/cmd/libsnap-confine-private/mount-opt.c
index 44b0987d9a..2b2c7a1ace 100644
--- a/cmd/libsnap-confine-private/mount-opt.c
+++ b/cmd/libsnap-confine-private/mount-opt.c
@@ -256,9 +256,10 @@ static const char *use_debug_build =
 "(disabled) use debug build to see details";
 #endif
 
-void sc_do_mount(const char *source, const char *target,
- const char *fs_type, unsigned long mountflags,
- const void *data)
+static bool sc_do_mount_ex(const char *source, const char *target,
+ const char *fs_type,
+ unsigned long mountflags, const void *data,
+ bool optional)
 {
	char buf[10000] = { 0 };
	const char *mount_cmd = NULL;
@@ -274,9 +275,11 @@ void sc_do_mount(const char *source, const char *target,
	}
	if (sc_faulty("mount", NULL)
 || mount(source, target, fs_type, mountflags, data) < 0) {
-	// Save errno as ensure can clobber it.
	int saved_errno = errno;
-
+	if (optional && saved_errno == ENOENT) {
+	// The special-cased value that is allowed to fail.
+	return false;
+	}
	// Drop privileges so that we can compute our nice error message
	// without risking an attack on one of the string functions there.
	sc_privs_drop();
@@ -288,6 +291,21 @@ void sc_do_mount(const char *source, const char *target,
	errno = saved_errno;
	die("cannot perform operation: %s", mount_cmd);
	}
+	return true;
+}
+
+void sc_do_mount(const char *source, const char *target,
+ const char *fs_type, unsigned long mountflags,
+ const void *data)
+{
+	(void)sc_do_mount_ex(source, target, fs_type, mountflags, data, false);
+}
+
+bool sc_do_optional_mount(const char *source, const char *target,
+ const char *fs_type, unsigned long mountflags,
+ const void *data)
+{
+	return sc_do_mount_ex(source, target, fs_type, mountflags, data, true);
 }
 
 void sc_do_umount(const char *target, int flags)
diff --git a/cmd/libsnap-confine-private/mount-opt.h b/cmd/libsnap-confine-private/mount-opt.h
index d87eefaf8b..03c50ffd92 100644
--- a/cmd/libsnap-confine-private/mount-opt.h
+++ b/cmd/libsnap-confine-private/mount-opt.h
@@ -18,6 +18,7 @@
 #ifndef SNAP_CONFINE_MOUNT_OPT_H
 #define SNAP_CONFINE_MOUNT_OPT_H
 
+#include <stdbool.h>
 #include <stddef.h>
 
 /**
@@ -68,6 +69,19 @@ void sc_do_mount(const char *source, const char *target,
 const void *data);
 
 /**
+ * A thin wrapper around mount(2) with logging and error checks.
+ *
+ * This variant is allowed to silently fail when mount fails with ENOENT.
+ * That is, it can be used to perform mount operations and if either the source
+ * or the destination is not present, carry on as if nothing had happened.
+ *
+ * The return value indicates if the operation was successful or not.
+ **/
+bool sc_do_optional_mount(const char *source, const char *target,
+ const char *fs_type, unsigned long mountflags,
+ const void *data);
+
+/**
 * A thin wrapper around umount(2) with logging and error checks.
 **/
 void sc_do_umount(const char *target, int flags);
diff --git a/cmd/snap-confine/mount-support.c b/cmd/snap-confine/mount-support.c
index 9456fff861..5bb3d488da 100644
--- a/cmd/snap-confine/mount-support.c
+++ b/cmd/snap-confine/mount-support.c
@@ -202,6 +202,9 @@ struct sc_mount {
	// Alternate path defines the rbind mount "alternative" of path.
	// It exists so that we can make /media on systems that use /run/media.
	const char *altpath;
+	// Optional mount points are not processed unless the source and
+	// destination both exist.
+	bool is_optional;
 };
 
 struct sc_mount_config {
@@ -301,7 +304,17 @@ static void sc_bootstrap_mount_namespace(const struct sc_mount_config *config)
	}
	sc_must_snprintf(dst, sizeof dst, "%s/%s", scratch_dir,
 mnt->path);
-	sc_do_mount(mnt->path, dst, NULL, MS_REC | MS_BIND, NULL);
+	if (mnt->is_optional) {
+	bool ok = sc_do_optional_mount(mnt->path, dst, NULL,
+ MS_REC | MS_BIND, NULL);
+	if (!ok) {
+	// If we cannot mount it, just continue.
+	continue;
+	}
+	} else {
+	sc_do_mount(mnt->path, dst, NULL, MS_REC | MS_BIND,
+ NULL);
+	}
	if (!mnt->is_bidirectional) {
	// Mount events will only propagate inwards to the namespace. This
	// way the running application cannot alter any global state apart
@@ -638,6 +651,10 @@ void sc_populate_mount_ns(struct sc_apparmor *apparmor, int snap_update_ns_fd,
	{"/media", true},	// access to the users removable devices
 #endif	// MERGED_USR
	{"/run/netns", true},	// access to the 'ip netns' network namespaces
+	// The /mnt directory is optional in base snaps to ensure backwards
+	// compatibility with the first version of base snaps that was
+	// released.
+	{"/mnt",.is_optional = true},	// to support the removable-media interface
	{},
	};
	char rootfs_dir[PATH_MAX] = { 0 };
diff --git a/cmd/snap-confine/snap-confine.apparmor.in b/cmd/snap-confine/snap-confine.apparmor.in
index 75d7ce073b..0ed3d5360d 100644
--- a/cmd/snap-confine/snap-confine.apparmor.in
+++ b/cmd/snap-confine/snap-confine.apparmor.in
@@ -187,6 +187,9 @@
 mount options=(rw rbind) /usr/src/ -> /tmp/snap.rootfs_*/usr/src/,
 mount options=(rw rslave) -> /tmp/snap.rootfs_*/usr/src/,
 
+ mount options=(rw rbind) /mnt/ -> /tmp/snap.rootfs_*/mnt/,
+ mount options=(rw rslave) -> /tmp/snap.rootfs_*/mnt/,
+
 # allow making host snap-exec available inside base snaps
 mount options=(rw bind) @LIBEXECDIR@/ -> /tmp/snap.rootfs_*/usr/lib/snapd/,
 mount options=(rw slave) -> /tmp/snap.rootfs_*/usr/lib/snapd/,
diff --git a/interfaces/builtin/removable_media.go b/interfaces/builtin/removable_media.go
index e38226e636..7f20c074d9 100644
--- a/interfaces/builtin/removable_media.go
+++ b/interfaces/builtin/removable_media.go
@@ -1,7 +1,7 @@
 // -*- Mode: Go; indent-tabs-mode: t -*-
 
 /*
- * Copyright (C) 2016 Canonical Ltd
+ * Copyright (C) 2016-2018 Canonical Ltd
 *
 * This program is free software: you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 3 as
@@ -42,6 +42,11 @@ const removableMediaConnectedPlugAppArmor = `
 # Mount points could be in /run/media/<user>/* or /media/<user>/*
 /{,run/}media/*/ r,
 /{,run/}media/*/** rwk,
+
+# Allow read-only access to /mnt to enumerate items.
+/mnt/ r,
+# Allow write access to anything under /mnt
+/mnt/** rwk,
 `
 
 func init() {
diff --git a/interfaces/builtin/removable_media_test.go b/interfaces/builtin/removable_media_test.go
index 47c1742131..6e6f770de0 100644
--- a/interfaces/builtin/removable_media_test.go
+++ b/interfaces/builtin/removable_media_test.go
@@ -87,6 +87,7 @@ func (s *RemovableMediaInterfaceSuite) TestUsedSecuritySystems(c *C) {
	c.Assert(err, IsNil)
	c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.client-snap.other"})
	c.Check(apparmorSpec.SnippetForTag("snap.client-snap.other"), testutil.Contains, "/{,run/}media/*/ r")
+	c.Check(apparmorSpec.SnippetForTag("snap.client-snap.other"), testutil.Contains, "/mnt/** rwk,")
 }
 
 func (s *RemovableMediaInterfaceSuite) TestInterfaces(c *C) {
diff --git a/interfaces/mount/backend.go b/interfaces/mount/backend.go
index 7d18b0ce4a..1ff8932dc2 100644
--- a/interfaces/mount/backend.go
+++ b/interfaces/mount/backend.go
@@ -61,7 +61,7 @@ func (b *Backend) Setup(snapInfo *snap.Info, confinement interfaces.ConfinementO
	if err != nil {
	return fmt.Errorf("cannot obtain mount security snippets for snap %q: %s", snapName, err)
	}
-	spec.(*Specification).AddSnapLayout(snapInfo)
+	spec.(*Specification).AddLayout(snapInfo)
	content := deriveContent(spec.(*Specification), snapInfo)
	// synchronize the content with the filesystem
	glob := fmt.Sprintf("snap.%s.*fstab", snapName)
diff --git a/interfaces/mount/spec.go b/interfaces/mount/spec.go
index 793cf1b648..e186f89590 100644
--- a/interfaces/mount/spec.go
+++ b/interfaces/mount/spec.go
@@ -35,20 +35,20 @@ import (
 // holds internal state that is used by the mount backend during the interface
 // setup process.
 type Specification struct {
-	layoutMountEntries []osutil.MountEntry
-	mountEntries []osutil.MountEntry
-	userMountEntries []osutil.MountEntry
+	layout []osutil.MountEntry
+	general []osutil.MountEntry
+	user []osutil.MountEntry
 }
 
 // AddMountEntry adds a new mount entry.
 func (spec *Specification) AddMountEntry(e osutil.MountEntry) error {
-	spec.mountEntries = append(spec.mountEntries, e)
+	spec.general = append(spec.general, e)
	return nil
 }
 
 //AddUserMountEntry adds a new user mount entry.
 func (spec *Specification) AddUserMountEntry(e osutil.MountEntry) error {
-	spec.userMountEntries = append(spec.userMountEntries, e)
+	spec.user = append(spec.user, e)
	return nil
 }
 
@@ -110,8 +110,8 @@ func mountEntryFromLayout(layout *snap.Layout) osutil.MountEntry {
	return entry
 }
 
-// AddSnapLayout adds mount entries based on the layout of the snap.
-func (spec *Specification) AddSnapLayout(si *snap.Info) {
+// AddLayout adds mount entries based on the layout of the snap.
+func (spec *Specification) AddLayout(si *snap.Info) {
	// TODO: handle layouts in base snaps as well as in this snap.
 
	// walk the layout elements in deterministic order, by mount point name
@@ -123,23 +123,23 @@ func (spec *Specification) AddSnapLayout(si *snap.Info) {
 
	for _, path := range paths {
	entry := mountEntryFromLayout(si.Layout[path])
-	spec.layoutMountEntries = append(spec.layoutMountEntries, entry)
+	spec.layout = append(spec.layout, entry)
	}
 }
 
 // MountEntries returns a copy of the added mount entries.
 func (spec *Specification) MountEntries() []osutil.MountEntry {
-	result := make([]osutil.MountEntry, 0, len(spec.layoutMountEntries)+len(spec.mountEntries))
-	result = append(result, spec.layoutMountEntries...)
-	result = append(result, spec.mountEntries...)
+	result := make([]osutil.MountEntry, 0, len(spec.layout)+len(spec.general))
+	result = append(result, spec.layout...)
+	result = append(result, spec.general...)
	unclashMountEntries(result)
	return result
 }
 
 // UserMountEntries returns a copy of the added user mount entries.
 func (spec *Specification) UserMountEntries() []osutil.MountEntry {
-	result := make([]osutil.MountEntry, len(spec.userMountEntries))
-	copy(result, spec.userMountEntries)
+	result := make([]osutil.MountEntry, len(spec.user))
+	copy(result, spec.user)
	unclashMountEntries(result)
	return result
 }
diff --git a/interfaces/mount/spec_test.go b/interfaces/mount/spec_test.go
index 7bd48bc2c4..c4c40a30b2 100644
--- a/interfaces/mount/spec_test.go
+++ b/interfaces/mount/spec_test.go
@@ -154,7 +154,7 @@ layout:
 
 func (s *specSuite) TestMountEntryFromLayout(c *C) {
	snapInfo := snaptest.MockInfo(c, snapWithLayout, &snap.SideInfo{Revision: snap.R(42)})
-	s.spec.AddSnapLayout(snapInfo)
+	s.spec.AddLayout(snapInfo)
	c.Assert(s.spec.MountEntries(), DeepEquals, []osutil.MountEntry{
	// Layout result is sorted by mount path.
	{Dir: "/etc/foo.conf", Name: "/snap/vanguard/42/foo.conf", Options: []string{"bind", "rw", "x-snapd.kind=file", "x-snapd.origin=layout"}},
@@ -176,7 +176,7 @@ layout:
	entry := osutil.MountEntry{Dir: "/foo", Type: "tmpfs", Name: "tmpfs"}
	s.spec.AddMountEntry(entry)
	s.spec.AddUserMountEntry(entry)
-	s.spec.AddSnapLayout(snapInfo)
+	s.spec.AddLayout(snapInfo)
	c.Assert(s.spec.MountEntries(), DeepEquals, []osutil.MountEntry{
	{Dir: "/foo", Type: "tmpfs", Name: "tmpfs", Options: []string{"x-snapd.origin=layout"}},
	// This is the non-layout entry, it was renamed to "foo-2"
diff --git a/strutil/matchcounter.go b/strutil/matchcounter.go
index 09187967b9..af612b905a 100644
--- a/strutil/matchcounter.go
+++ b/strutil/matchcounter.go
@@ -37,29 +37,31 @@ type MatchCounter struct {
 
	count int
	matches []string
-	partial []byte
+	partial bytes.Buffer
 }
 
 func (w *MatchCounter) Write(p []byte) (int, error) {
	n := len(p)
-	if len(w.partial) > 0 {
+	if w.partial.Len() > 0 {
	idx := bytes.IndexByte(p, '\n')
	if idx < 0 {
-	w.partial = append(w.partial, p...)
+	// no newline yet, carry on accumulating
+	w.partial.Write(p)
	return n, nil
	}
	idx++
-	w.check(append(w.partial, p[:idx]...))
+	w.partial.Write(p[:idx])
+	w.check(w.partial.Bytes())
	p = p[idx:]
-	w.partial = nil
	}
+	w.partial.Reset()
	idx := bytes.LastIndexByte(p, '\n')
	if idx < 0 {
-	w.partial = p
+	w.partial.Write(p)
	return n, nil
	}
	idx++
-	w.partial = p[idx:]
+	w.partial.Write(p[idx:])
	w.check(p[:idx])
	return n, nil
 }
diff --git a/strutil/matchcounter_test.go b/strutil/matchcounter_test.go
index 6efd9e7aa9..a454c1d859 100644
--- a/strutil/matchcounter_test.go
+++ b/strutil/matchcounter_test.go
@@ -115,6 +115,34 @@ func (mcSuite) TestMatchCounterPartials(c *check.C) {
	}
 }
 
+func (mcSuite) TestMatchCounterPartialsReusingBuffer(c *check.C) {
+	// now we know the whole thing matches expected, we check partials
+	buf := []byte(out)
+	expected := []string{
+	"Failed to write /tmp/1/modules/4.4.0-112-generic/modules.symbols, skipping",
+	"Write on output file failed because No space left on device",
+	"writer: failed to write data block 0",
+	}
+
+	for step := 1; step < 100; step++ {
+	wbuf := make([]byte, step)
+	w := &strutil.MatchCounter{Regexp: thisRegexp, N: 3}
+	var i int
+	for i = 0; i+step < len(buf); i += step {
+	copy(wbuf, buf[i:])
+	_, err := w.Write(wbuf)
+	c.Assert(err, check.IsNil, check.Commentf("step:%d i:%d", step, i))
+	}
+	wbuf = wbuf[:len(buf[i:])]
+	copy(wbuf, buf[i:])
+	_, err := w.Write(wbuf)
+	c.Assert(err, check.IsNil, check.Commentf("step:%d tail", step))
+	matches, count := w.Matches()
+	c.Assert(count, check.Equals, 19, check.Commentf("step:%d", step))
+	c.Assert(matches, check.DeepEquals, expected, check.Commentf("step:%d", step))
+	}
+}
+
 func (mcSuite) TestMatchCounterZero(c *check.C) {
	w := &strutil.MatchCounter{Regexp: thisRegexp, N: 0}
	_, err := w.Write([]byte(out))
diff --git a/tests/main/interfaces-removable-media/task.yaml b/tests/main/interfaces-removable-media/task.yaml
index db4c88884a..1340621df4 100644
--- a/tests/main/interfaces-removable-media/task.yaml
+++ b/tests/main/interfaces-removable-media/task.yaml
@@ -22,6 +22,10 @@ prepare: |
 mkdir -p /run/media/testdir
 touch /run/media/testdir/testfile
 
+ echo "Mount a filesystem in /tmp/testing and put some data there"
+ mkdir -p /mnt/testing/
+ mount -t tmpfs none /mnt/testing
+ echo canary > /mnt/testing/data
 
 restore: |
 rm -f call.error
@@ -37,6 +41,9 @@ restore: |
 rm -rf /run/media/
 fi
 
+ umount /mnt/testing
+ rmdir /mnt/testing || true # in case the user had /mnt/testing
+
 execute: |
 echo "The interface is not connected by default"
 snap interfaces -i removable-media | MATCH -- '- +test-snapd-removable-media:removable-media'
@@ -56,6 +63,11 @@ execute: |
 test-snapd-removable-media.sh -c "ls /run/media/testdir"
 test-snapd-removable-media.sh -c "echo test >> /run/media/testdir/testfile"
 
+ echo "And the snap has read-write access to /mnt/testing/data"
+ test-snapd-removable-media.sh -c 'cat /mnt/testing/data' | MATCH canary
+ test-snapd-removable-media.sh -c 'echo modified > /mnt/testing/data'
+ MATCH modified < /mnt/testing/data
+
 if [ "$(snap debug confinement)" = partial ]; then
 exit 0
 fi
@@ -69,3 +81,8 @@ execute: |
 exit 1
 fi
 MATCH "Permission denied" < call.error
+
+ echo "Then /mnt/testing/data is no longer readable"
+ if test-snapd-removable-media.sh -c 'cat /mnt/testing/data'; then
+ echo "/mnt/testing/data should no longer be readable" && exit 1
+ fi