Merge pull request #8729 from hashicorp/b-assorted-fixes-20200824

Assorted small fixes

acl/policy.go

@@ -238,7 +238,7 @@ func Parse(rules string) (*Policy, error) {
 	}
 
 	// Attempt to parse
-	if err := hcl.Decode(p, rules); err != nil {
+	if err := hclDecode(p, rules); err != nil {
 		return nil, fmt.Errorf("Failed to parse ACL Policy: %v", err)
 	}
 
@@ -312,3 +312,14 @@ func Parse(rules string) (*Policy, error) {
 	}
 	return p, nil
 }
+
+// hclDecode wraps hcl.Decode function but handles any unexpected panics
+func hclDecode(p *Policy, rules string) (err error) {
+	defer func() {
+		if rerr := recover(); rerr != nil {
+			err = fmt.Errorf("invalid acl policy: %v", rerr)
+		}
+	}()
+
+	return hcl.Decode(p, rules)
+}

acl/policy_test.go

@@ -327,3 +327,16 @@ func TestParse(t *testing.T) {
 		})
 	}
 }
+
+func TestParse_BadInput(t *testing.T) {
+	inputs := []string{
+		`namespace "\500" {}`,
+	}
+
+	for i, c := range inputs {
+		t.Run(fmt.Sprintf("%d: %v", i, c), func(t *testing.T) {
+			_, err := Parse(c)
+			assert.Error(t, err)
+		})
+	}
+}

client/allocdir/alloc_dir.go

@@ -117,12 +117,13 @@ func NewAllocDir(logger hclog.Logger, allocDir string) *AllocDir {
 // Copy an AllocDir and all of its TaskDirs. Returns nil if AllocDir is
 // nil.
 func (d *AllocDir) Copy() *AllocDir {
-	d.mu.RLock()
-	defer d.mu.RUnlock()
-
 	if d == nil {
 		return nil
 	}
+
+	d.mu.RLock()
+	defer d.mu.RUnlock()
+
 	dcopy := &AllocDir{
 		AllocDir:  d.AllocDir,
 		SharedDir: d.SharedDir,
@@ -429,6 +430,7 @@ func detectContentType(fileInfo os.FileInfo, path string) string {
 			if err == nil {
 				contentType = http.DetectContentType(fileBytes)
 			}
+			f.Close()
 		}
 	}
 	// Special case json files

nomad/job_endpoint.go

@@ -1930,7 +1930,7 @@ func validateDispatchRequest(req *structs.JobDispatchRequest, job *structs.Job)
 
 	// Check if the metadata is a set
 	keys := make(map[string]struct{}, len(req.Meta))
-	for k := range keys {
+	for k := range req.Meta {
 		if _, ok := keys[k]; ok {
 			return fmt.Errorf("Duplicate key %q in passed metadata", k)
 		}

nomad/structs/structs.go

@@ -7455,9 +7455,12 @@ func (ta *TaskArtifact) Hash() string {
 }
 
 // PathEscapesAllocDir returns if the given path escapes the allocation
-// directory. The prefix allows adding a prefix if the path will be joined, for
-// example a "task/local" prefix may be provided if the path will be joined
-// against that prefix.
+// directory.
+//
+// The prefix is to joined to the path (e.g. "task/local"), and this function
+// checks if path escapes the alloc dir, NOT the prefix directory within the alloc dir.
+// With prefix="task/local", it will return false for "../secret", but
+// true for "../../../../../../root" path; only the latter escapes the alloc dir
 func PathEscapesAllocDir(prefix, path string) (bool, error) {
 	// Verify the destination doesn't escape the tasks directory
 	alloc, err := filepath.Abs(filepath.Join("/", "alloc-dir/", "alloc-id/"))