From ebcef288343698dd86ff307f5b9c58aa52ce9fdd Mon Sep 17 00:00:00 2001
From: Brian Goff <cpuguy83@gmail.com>
Date: Tue, 21 May 2019 15:34:57 -0700
Subject: [PATCH] Fix error handling for bind mount spec parser.

Errors were being ignored and always telling the user that the path
doesn't exist even if it was some other problem, such as a permission
error.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
---
 volume/mounts/linux_parser.go |  5 +++-
 volume/mounts/parser_test.go  | 50 +++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/volume/mounts/linux_parser.go b/volume/mounts/linux_parser.go
index 035a24a8d9..22ab2d62cf 100644
--- a/volume/mounts/linux_parser.go
+++ b/volume/mounts/linux_parser.go
@@ -82,7 +82,10 @@ func (p *linuxParser) validateMountConfigImpl(mnt *mount.Mount, validateBindSour
 		}
 
 		if validateBindSourceExists {
-			exists, _, _ := currentFileInfoProvider.fileInfo(mnt.Source)
+			exists, _, err := currentFileInfoProvider.fileInfo(mnt.Source)
+			if err != nil {
+				return &errMountConfig{mnt, err}
+			}
 			if !exists {
 				return &errMountConfig{mnt, errBindSourceDoesNotExist(mnt.Source)}
 			}
diff --git a/volume/mounts/parser_test.go b/volume/mounts/parser_test.go
index 27257d62bd..f9b32e5ccf 100644
--- a/volume/mounts/parser_test.go
+++ b/volume/mounts/parser_test.go
@@ -1,6 +1,7 @@
 package mounts // import "github.com/docker/docker/volume/mounts"
 
 import (
+	"errors"
 	"io/ioutil"
 	"os"
 	"runtime"
@@ -8,6 +9,8 @@ import (
 	"testing"
 
 	"github.com/docker/docker/api/types/mount"
+	"gotest.tools/assert"
+	"gotest.tools/assert/cmp"
 )
 
 type parseMountRawTestSet struct {
@@ -477,4 +480,51 @@ func TestParseMountSpec(t *testing.T) {
 			t.Errorf("Expected mount copy data to match. Expected: '%v', Actual: '%v'", c.expected.CopyData, mp.CopyData)
 		}
 	}
+
+}
+
+// always returns the configured error
+// this is used to test error handling
+type mockFiProviderWithError struct{ err error }
+
+func (m mockFiProviderWithError) fileInfo(path string) (bool, bool, error) {
+	return false, false, m.err
+}
+
+// TestParseMountSpecBindWithFileinfoError makes sure that the parser returns
+// the error produced by the fileinfo provider.
+//
+// Some extra context for the future in case of changes and possible wtf are we
+// testing this for:
+//
+// Currently this "fileInfoProvider" returns (bool, bool, error)
+// The 1st bool is "does this path exist"
+// The 2nd bool is "is this path a dir"
+// Then of course the error is an error.
+//
+// The issue is the parser was ignoring the error and only looking at the
+// "does this path exist" boolean, which is always false if there is an error.
+// Then the error returned to the caller was a (slightly, maybe) friendlier
+// error string than what comes from `os.Stat`
+// So ...the caller was always getting an error saying the path doesn't exist
+// even if it does exist but got some other error (like a permission error).
+// This is confusing to users.
+func TestParseMountSpecBindWithFileinfoError(t *testing.T) {
+	previousProvider := currentFileInfoProvider
+	defer func() { currentFileInfoProvider = previousProvider }()
+
+	testErr := errors.New("some crazy error")
+	currentFileInfoProvider = &mockFiProviderWithError{err: testErr}
+
+	p := "/bananas"
+	if runtime.GOOS == "windows" {
+		p = `c:\bananas`
+	}
+	m := mount.Mount{Type: mount.TypeBind, Source: p, Target: p}
+
+	parser := NewParser(runtime.GOOS)
+
+	_, err := parser.ParseMountSpec(m)
+	assert.Assert(t, err != nil)
+	assert.Assert(t, cmp.Contains(err.Error(), "some crazy error"))
 }