Merge lp:~chipaca/snappy/fix-1438420 into lp:~snappy-dev/snappy/snappy-moved-to-github

[John Lenton]

Stop chowning the snapfile.

[John Lenton (chipaca)]

Talked with Sergio about ownership and readibility of the snap, we've come up with a solution, implementing it now.

[Michael Vogt (mvo)]

Thanks for doing this! One inline comment about a possible way to make it simpler.

[lp:~chipaca/snappy/fix-1438420]

337. By John Lenton on 2015-04-13

removed ClickDeb.Path

338. By John Lenton on 2015-04-13

close things you open

[Michael Vogt (mvo)]

Thanks for doing this work, not only does it fix the bug, I think the code looks much nicer now than the previous version. I really like the clickdeb.Create() and clickdeb.Open() in build/unpack, its much eaiser to read this way.

review: Approve

=== modified file 'clickdeb/deb.go'
--- clickdeb/deb.go	2015-04-08 16:11:33 +0000
+++ clickdeb/deb.go	2015-04-13 21:00:55 +0000
@@ -105,7 +105,35 @@
 // ClickDeb provides support for the "click" containers (a special kind of
 // deb package)
 type ClickDeb struct {
-	Path string
+	file *os.File
+}
+
+// Open calls os.Open and uses that file for the backing file.
+func Open(path string) (*ClickDeb, error) {
+	f, err := os.Open(path)
+	if err != nil {
+		return nil, err
+	}
+	return &ClickDeb{f}, nil
+}
+
+// Create calls os.Create and uses that file for the backing file.
+func Create(path string) (*ClickDeb, error) {
+	f, err := os.Create(path)
+	if err != nil {
+		return nil, err
+	}
+	return &ClickDeb{f}, nil
+}
+
+// Name returns the Name of the backing file
+func (d *ClickDeb) Name() string {
+	return d.file.Name()
+}
+
+// Close closes the backing file
+func (d *ClickDeb) Close() error {
+	return d.file.Close()
 }
 
 // ControlMember returns the content of the given control member file
@@ -125,13 +153,12 @@
 //
 // Confused? look at ControlMember and MetaMember, which this generalises.
 func (d *ClickDeb) member(arMember, tarMember string) (content []byte, err error) {
-	file, err := os.Open(d.Path)
-	if err != nil {
+
+	if _, err := d.file.Seek(0, 0); err != nil {
 		return nil, err
 	}
-	defer file.Close()
 
-	arReader := ar.NewReader(file)
+	arReader := ar.NewReader(d.file)
 	dataReader, err := skipToArMember(arReader, arMember)
 	if err != nil {
 		return nil, err
@@ -161,13 +188,11 @@
 func (d *ClickDeb) Unpack(targetDir string) error {
 	var err error
 
-	file, err := os.Open(d.Path)
-	if err != nil {
+	if _, err := d.file.Seek(0, 0); err != nil {
 		return err
 	}
-	defer file.Close()
 
-	arReader := ar.NewReader(file)
+	arReader := ar.NewReader(d.file)
 	dataReader, err := skipToArMember(arReader, "data.tar")
 	if err != nil {
 		return err
@@ -327,13 +352,6 @@
 func (d *ClickDeb) Build(sourceDir string, dataTarFinishedCallback func(dataName string) error) error {
 	var err error
 
-	// create file
-	file, err := os.Create(d.Path)
-	if err != nil {
-		return err
-	}
-	defer file.Close()
-
 	// tmp
 	tempdir, err := ioutil.TempDir("", "data")
 	if err != nil {
@@ -365,7 +383,7 @@
 	}
 
 	// create ar
-	arWriter := ar.NewWriter(file)
+	arWriter := ar.NewWriter(d.file)
 	arWriter.WriteGlobalHeader()
 
 	// debian magic

=== modified file 'clickdeb/deb_test.go'
--- clickdeb/deb_test.go	2015-03-30 12:08:58 +0000
+++ clickdeb/deb_test.go	2015-04-13 21:00:55 +0000
@@ -86,19 +86,21 @@
 	builddir := makeTestDebDir(c)
 
 	debDir := c.MkDir()
-	d := ClickDeb{Path: filepath.Join(debDir, "foo_1.0_all.deb")}
-	err := d.Build(builddir, nil)
-	c.Assert(err, IsNil)
-	c.Assert(helpers.FileExists(d.Path), Equals, true)
+	path := filepath.Join(debDir, "foo_1.0_all.deb")
+	d, err := Create(path)
+	c.Assert(err, IsNil)
+	err = d.Build(builddir, nil)
+	c.Assert(err, IsNil)
+	c.Assert(helpers.FileExists(path), Equals, true)
 
 	// control
-	cmd := exec.Command("dpkg-deb", "-I", d.Path)
+	cmd := exec.Command("dpkg-deb", "-I", path)
 	output, err := cmd.CombinedOutput()
 	c.Assert(err, IsNil)
 	c.Assert(strings.Contains(string(output), "Package: foo\n"), Equals, true)
 
 	// data
-	cmd = exec.Command("dpkg-deb", "-c", d.Path)
+	cmd = exec.Command("dpkg-deb", "-c", path)
 	output, err = cmd.CombinedOutput()
 	c.Assert(err, IsNil)
 	c.Assert(strings.Contains(string(output), "./usr/bin/foo"), Equals, true)
@@ -108,7 +110,8 @@
 func (s *ClickDebTestSuite) TestSnapDebControlMember(c *C) {
 	debName := makeTestDeb(c, "gzip")
 
-	d := ClickDeb{Path: debName}
+	d, err := Open(debName)
+	c.Assert(err, IsNil)
 	content, err := d.ControlMember("control")
 	c.Assert(err, IsNil)
 	c.Assert(string(content), Equals, string(testDebControl))
@@ -116,7 +119,8 @@
 
 func (s *ClickDebTestSuite) TestSnapDebMetaMember(c *C) {
 	debName := makeTestDeb(c, "gzip")
-	d := ClickDeb{Path: debName}
+	d, err := Open(debName)
+	c.Assert(err, IsNil)
 	yaml, err := d.MetaMember("package.yaml")
 	c.Assert(err, IsNil)
 	c.Assert(string(yaml), Equals, "name: foo")
@@ -127,8 +131,9 @@
 
 	for _, comp := range []string{"gzip", "bzip2", "xz"} {
 		debName := makeTestDeb(c, comp)
-		d := ClickDeb{Path: debName}
-		err := d.Unpack(targetDir)
+		d, err := Open(debName)
+		c.Assert(err, IsNil)
+		err = d.Unpack(targetDir)
 		c.Assert(err, IsNil)
 		expectedFile := filepath.Join(targetDir, "usr", "bin", "foo")
 		c.Assert(helpers.FileExists(expectedFile), Equals, true)

=== modified file 'cmd/snappy/cmd_internal_unpack.go'
--- cmd/snappy/cmd_internal_unpack.go	2015-04-01 03:11:44 +0000
+++ cmd/snappy/cmd_internal_unpack.go	2015-04-13 21:00:55 +0000
@@ -90,6 +90,12 @@
 
 func unpackAndDropPrivs(snapFile, targetDir, rootDir string) error {
 
+	d, err := clickdeb.Open(snapFile)
+	if err != nil {
+		return err
+	}
+	defer d.Close()
+
 	if helpers.ShouldDropPrivs() {
 
 		passFile := passwdFile(rootDir, "passwd")
@@ -108,10 +114,8 @@
 			return err
 		}
 
-		for _, p := range []string{snapFile, targetDir} {
-			if err := os.Chown(p, uid, gid); err != nil {
-				return err
-			}
+		if err := os.Chown(targetDir, uid, gid); err != nil {
+			return err
 		}
 
 		// run prctl(PR_SET_NO_NEW_PRIVS)
@@ -137,8 +141,6 @@
 		}
 	}
 
-	d := clickdeb.ClickDeb{Path: snapFile}
-
 	return d.Unpack(targetDir)
 }
 

=== modified file 'snappy/build.go'
--- snappy/build.go	2015-04-08 18:15:27 +0000
+++ snappy/build.go	2015-04-13 21:00:55 +0000
@@ -507,7 +507,12 @@
 	}
 
 	// build it
-	d := clickdeb.ClickDeb{Path: snapName}
+	d, err := clickdeb.Create(snapName)
+	if err != nil {
+		return "", err
+	}
+	defer d.Close()
+
 	err = d.Build(buildDir, func(dataTar string) error {
 		// write hashes of the files plus the generated data tar
 		return writeHashes(buildDir, dataTar)

=== modified file 'snappy/click.go'
--- snappy/click.go	2015-04-08 18:53:49 +0000
+++ snappy/click.go	2015-04-13 21:00:55 +0000
@@ -631,12 +631,12 @@
 		return ErrUnpackHelperNotFound
 	}
 
-	cmd := exec.Command(privHelper, "internal-unpack", d.Path, instDir, globalRootDir)
+	cmd := exec.Command(privHelper, "internal-unpack", d.Name(), instDir, globalRootDir)
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 	if err := cmd.Run(); err != nil {
 		return &ErrUnpackFailed{
-			snapFile: d.Path,
+			snapFile: d.Name(),
 			instDir:  instDir,
 			origErr:  err,
 		}
@@ -662,7 +662,11 @@
 		//return SnapAuditError
 	}
 
-	d := &clickdeb.ClickDeb{Path: snapFile}
+	d, err := clickdeb.Open(snapFile)
+	if err != nil {
+		return "", err
+	}
+	defer d.Close()
 	manifestData, err := d.ControlMember("manifest")
 	if err != nil {
 		log.Printf("Snap inspect failed: %s", snapFile)

=== modified file 'snappy/click_test.go'
--- snappy/click_test.go	2015-04-08 18:53:49 +0000
+++ snappy/click_test.go	2015-04-13 21:00:55 +0000
@@ -882,7 +882,9 @@
 	c.Assert(writeDebianControl(tmpdir, m), IsNil)
 	c.Assert(writeClickManifest(tmpdir, m), IsNil)
 	snapName := fmt.Sprintf("%s_%s_all.snap", m.Name, m.Version)
-	d := &clickdeb.ClickDeb{Path: snapName}
+	d, err := clickdeb.Create(snapName)
+	c.Assert(err, IsNil)
+	defer d.Close()
 	c.Assert(d.Build(tmpdir, func(dataTar string) error {
 		return writeHashes(tmpdir, dataTar)
 	}), IsNil)