[Merge] lp:~sergiusens/goget-ubuntu-touch/mountFix into lp:goget-ubuntu-touch

[Michael Vogt]

Review: Needs Information

I like how you extracted the common code into mount(). One (important) inline question though.

Diff comments:

> === modified file 'diskimage/common.go'
> --- diskimage/common.go	2015-03-30 03:57:01 +0000
> +++ diskimage/common.go	2015-03-31 13:49:03 +0000
> @@ -9,6 +9,7 @@
>  
>  import (
>  	"fmt"
> +	"io/ioutil"
>  	"os"
>  	"os/exec"
>  	"path/filepath"
> @@ -131,6 +132,42 @@
>  	return strings.TrimSpace(string(out)), err
>  }
>  
> +func mount(partitions []partition) (baseMount string, err error) {
> +	baseMount, err = ioutil.TempDir(os.TempDir(), "diskimage")
> +	if err != nil {
> +		return "", err
> +	}
> +
> +	// We change the mode so snappy can unpack as non root
> +	if err := os.Chmod(baseMount, 0777); err != nil {

0777 sounds dangerous here, I understand why this is done, but it also allows a potential attacker on a multi-user system to wait for this mount to appear and write stuff to it. Or am I missing something? Maybe it could be a more targeted Chmod()?

> +		return "", err
> +	}
> +
> +	//Remove Mountpoint if we fail along the way
> +	defer func() {
> +		if err != nil {
> +			os.Remove(baseMount)
> +		}
> +	}()
> +
> +	for _, part := range partitions {
> +		if part.fs == fsNone {
> +			continue
> +		}
> +
> +		mountpoint := filepath.Join(baseMount, string(part.dir))
> +		if err := os.MkdirAll(mountpoint, 0777); err != nil {
> +			return "", err
> +		}
> +		if out, err := exec.Command("mount", filepath.Join("/dev/mapper", part.loop), mountpoint).CombinedOutput(); err != nil {
> +			return "", fmt.Errorf("unable to mount dir to create system image: %s", out)
> +		}
> +	}
> +
> +	return baseMount, nil
> +
> +}
> +
>  func printOut(args ...interface{}) {
>  	if debugPrint {
>  		fmt.Println(args...)
> 
> === modified file 'diskimage/core_grub.go'
> --- diskimage/core_grub.go	2015-03-21 00:13:02 +0000
> +++ diskimage/core_grub.go	2015-03-31 13:49:03 +0000
> @@ -12,7 +12,6 @@
>  	"errors"
>  	"fmt"
>  	"io"
> -	"io/ioutil"
>  	"os"
>  	"os/exec"
>  	"path/filepath"
> @@ -56,31 +55,12 @@
>  GRUB_RECORDFAIL_TIMEOUT=0
>  `
>  
> -func (img *CoreGrubImage) Mount() (err error) {
> -	img.baseMount, err = ioutil.TempDir(os.TempDir(), "core-grub-disk")
> +func (img *CoreGrubImage) Mount() error {
> +	baseMount, err := mount(img.parts)
>  	if err != nil {
> -		return errors.New(fmt.Sprintf("Unable to create temp dir to create system image: %s", err))
> -	}
> -	//Remove Mountpoint if we fail along the way
> -	defer func() {
> -		if err != nil {
> -			os.Remove(img.baseMount)
> -		}
> -	}()
> -
> -	for _, part := range img.parts {
> -		if part.fs == fsNone {
> -			continue
> -		}
> -
> -		mountpoint := filepath.Join(img.baseMount, string(part.dir))
> -		if err := os.MkdirAll(mountpoint, 0755); err != nil {
> -			return err
> -		}
> -		if out, err := exec.Command("mount", filepath.Join("/dev/mapper", part.loop), mountpoint).CombinedOutput(); err != nil {
> -			return fmt.Errorf("unable to mount dir to create system image: %s", out)
> -		}
> -	}
> +		return err
> +	}
> +	img.baseMount = baseMount
>  
>  	return nil
>  }
> 
> === modified file 'diskimage/core_uboot.go'
> --- diskimage/core_uboot.go	2015-03-30 03:50:16 +0000
> +++ diskimage/core_uboot.go	2015-03-31 13:49:03 +0000
> @@ -87,27 +87,12 @@
>  	}
>  }
>  
> -func (img *CoreUBootImage) Mount() (err error) {
> -	img.baseMount, err = ioutil.TempDir(os.TempDir(), "core-uboot-disk")
> +func (img *CoreUBootImage) Mount() error {
> +	baseMount, err := mount(img.parts)
>  	if err != nil {
> -		return errors.New(fmt.Sprintf("Unable to create temp dir to create system image: %s", err))
> -	}
> -	//Remove Mountpoint if we fail along the way
> -	defer func() {
> -		if err != nil {
> -			os.Remove(img.baseMount)
> -		}
> -	}()
> -
> -	for _, part := range img.parts {
> -		mountpoint := filepath.Join(img.baseMount, string(part.dir))
> -		if err := os.MkdirAll(mountpoint, 0755); err != nil {
> -			return err
> -		}
> -		if out, err := exec.Command("mount", filepath.Join("/dev/mapper", part.loop), mountpoint).CombinedOutput(); err != nil {
> -			return fmt.Errorf("unable to mount dir to create system image: %s", out)
> -		}
> -	}
> +		return err
> +	}
> +	img.baseMount = baseMount
>  
>  	return nil
>  }
> 


-- 
https://code.launchpad.net/~sergiusens/goget-ubuntu-touch/mountFix/+merge/254761
Your team Ubuntu Phablet Team is subscribed to branch lp:goget-ubuntu-touch.