x/mod: fix handling of vendored packages with '/vendor' in non-top-le…

…vel paths This CL address a bug in the handling of vendored packages where the '/vendor' element appears in non-top level import paths within a module zip file. This issue arose from an incorrect offset calculation that caused non-vendored packages to be incorrectly identified as vendored. This CL corrects the offset calculation for Go 1.24 and beyond. Fixes golang/go#37397 Change-Id: Ibf1751057e8383c7b82f1622674204597e735b7c Reviewed-on: https://go-review.googlesource.com/c/mod/+/619175 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
golang · Oct 10, 2024 · c8a7319 · c8a7319
1 parent 9cd0e4c
commit c8a7319
Show file tree

Hide file tree

Showing 12 changed files with 67 additions and 35 deletions.
diff --git a/zip/testdata/check_dir/various.txt b/zip/testdata/check_dir/various.txt
@@ -6,6 +6,7 @@ $work/vendor/modules.txt
 omitted:
 $work/.hg_archival.txt: file is inserted by 'hg archive' and is always omitted
 $work/.git: directory is a version control repository
+$work/pkg/vendor/vendor.go: file is in vendor directory
 $work/sub: directory is in another module
 $work/vendor/x/y: file is in vendor directory
 
@@ -20,3 +21,4 @@ $work/invalid.go': malformed file path "invalid.go'": invalid char '\''
 -- sub/go.mod --
 -- .hg_archival.txt --
 -- .git/x --
+-- pkg/vendor/vendor.go --
diff --git a/zip/testdata/check_dir/various_go123.txt b/zip/testdata/check_dir/various_go123.txt
@@ -7,6 +7,7 @@ $work/vendor/modules.txt
 omitted:
 $work/.hg_archival.txt: file is inserted by 'hg archive' and is always omitted
 $work/.git: directory is a version control repository
+$work/pkg/vendor/vendor.go: file is in vendor directory
 $work/sub: directory is in another module
 $work/vendor/x/y: file is in vendor directory
 
@@ -20,4 +21,5 @@ go 1.23
 -- vendor/x/y --
 -- sub/go.mod --
 -- .hg_archival.txt --
--- .git/x --
+-- .git/x --
+-- pkg/vendor/vendor.go --
diff --git a/zip/testdata/check_dir/various_go124.txt b/zip/testdata/check_dir/various_go124.txt
@@ -1,6 +1,7 @@
 -- want --
 valid:
 $work/go.mod
+$work/pkg/vendor/vendor.go
 $work/valid.go
 
 omitted:
@@ -22,3 +23,4 @@ go 1.24
 go 1.23
 -- .hg_archival.txt --
 -- .git/x --
+-- pkg/vendor/vendor.go --
diff --git a/zip/testdata/check_files/various.txt b/zip/testdata/check_files/various.txt
@@ -7,6 +7,7 @@ omitted:
 vendor/x/y: file is in vendor directory
 sub/go.mod: file is in another module
 .hg_archival.txt: file is inserted by 'hg archive' and is always omitted
+pkg/vendor/vendor.go: file is in vendor directory
 
 invalid:
 not/../clean: file path is not clean
@@ -25,3 +26,4 @@ valid.go: multiple entries for file "valid.go"
 duplicate
 -- valid.go --
 another duplicate
+-- pkg/vendor/vendor.go --
diff --git a/zip/testdata/check_files/various_go123.txt b/zip/testdata/check_files/various_go123.txt
@@ -8,6 +8,7 @@ omitted:
 vendor/x/y: file is in vendor directory
 sub/go.mod: file is in another module
 .hg_archival.txt: file is inserted by 'hg archive' and is always omitted
+pkg/vendor/vendor.go: file is in vendor directory
 
 invalid:
 not/../clean: file path is not clean
@@ -26,3 +27,4 @@ go 1.23
 duplicate
 -- valid.go --
 another duplicate
+-- pkg/vendor/vendor.go --
diff --git a/zip/testdata/check_files/various_go124.txt b/zip/testdata/check_files/various_go124.txt
@@ -2,6 +2,7 @@
 valid:
 valid.go
 go.mod
+pkg/vendor/vendor.go
 
 omitted:
 vendor/x/y: file is in vendor directory
@@ -27,3 +28,4 @@ go 1.23
 duplicate
 -- valid.go --
 another duplicate
+-- pkg/vendor/vendor.go --
diff --git a/zip/testdata/create/exclude_vendor.txt b/zip/testdata/create/exclude_vendor.txt
@@ -12,3 +12,6 @@ see comment in isVendoredPackage and golang.org/issue/31562.
 excluded
 -- sub/vendor/sub.txt --
 excluded
+-- pkg/vendor/vendor.go --
+excluded
+see comment in isVendoredPackage and golang.org/issue/37397
diff --git a/zip/testdata/create/exclude_vendor_go124.txt b/zip/testdata/create/exclude_vendor_go124.txt
@@ -1,6 +1,6 @@
 path=example.com/m
 version=v1.0.0
-hash=h1:WxQ7ERgHLILawPCiuSwXp5UHul7CUSRRftO8b7GTnV8=
+hash=h1:mJR1q75yiMK6+CDw6DuhMS4v4hNoLXqkyk2Cph4VS8Q=
 -- go.mod --
 module example.com/m
 
@@ -13,3 +13,6 @@ see comment in isVendoredPackage and golang.org/issue/31562.
 excluded
 -- sub/vendor/sub.txt --
 excluded
+-- pkg/vendor/vendor.go --
+included
+see comment in isVendoredPackage and golang.org/issue/37397
diff --git a/zip/testdata/create_from_dir/exclude_vendor.txt b/zip/testdata/create_from_dir/exclude_vendor.txt
@@ -12,3 +12,6 @@ see comment in isVendoredPackage and golang.org/issue/31562.
 excluded
 -- sub/vendor/sub.txt --
 excluded
+-- pkg/vendor/vendor.go --
+excluded
+see comment in isVendoredPackage and golang.org/issue/37397
diff --git a/zip/testdata/create_from_dir/exclude_vendor_go124.txt b/zip/testdata/create_from_dir/exclude_vendor_go124.txt
@@ -1,6 +1,6 @@
 path=example.com/m
 version=v1.0.0
-hash=h1:WxQ7ERgHLILawPCiuSwXp5UHul7CUSRRftO8b7GTnV8=
+hash=h1:mJR1q75yiMK6+CDw6DuhMS4v4hNoLXqkyk2Cph4VS8Q=
 -- go.mod --
 module example.com/m
 
@@ -13,3 +13,6 @@ see comment in isVendoredPackage and golang.org/issue/31562.
 excluded
 -- sub/vendor/sub.txt --
 excluded
+-- pkg/vendor/vendor.go --
+included
+see comment in isVendoredPackage and golang.org/issue/37397
diff --git a/zip/vendor_test.go b/zip/vendor_test.go
@@ -18,43 +18,40 @@ var pre124 []string = []string{
 	"go1.9",
 }
 
+var after124 []string = []string{"go1.24.0", "go1.24", "go1.99.0"}
+
+var allVers []string = append(pre124, after124...)
+
 func TestIsVendoredPackage(t *testing.T) {
 	for _, tc := range []struct {
-		path          string
-		want          bool
-		falsePositive bool // is this case affected by https://golang.org/issue/37397?
-		versions      []string
+		path     string
+		want     bool
+		versions []string
 	}{
-		{path: "vendor/foo/foo.go", want: true, versions: pre124},
-		{path: "pkg/vendor/foo/foo.go", want: true, versions: pre124},
-		{path: "longpackagename/vendor/foo/foo.go", want: true, versions: pre124},
-		{path: "vendor/vendor.go", want: false, versions: pre124},
-		{path: "vendor/foo/modules.txt", want: true, versions: pre124},
-		{path: "modules.txt", want: false, versions: pre124},
-		{path: "vendor/amodules.txt", want: false, versions: pre124},
+		{path: "vendor/foo/foo.go", want: true, versions: allVers},
+		{path: "pkg/vendor/foo/foo.go", want: true, versions: allVers},
+		{path: "longpackagename/vendor/foo/foo.go", want: true, versions: allVers},
+		{path: "vendor/vendor.go", want: false, versions: allVers},
+		{path: "vendor/foo/modules.txt", want: true, versions: allVers},
+		{path: "modules.txt", want: false, versions: allVers},
+		{path: "vendor/amodules.txt", want: false, versions: allVers},
 
 		// These test cases were affected by https://golang.org/issue/63395
 		{path: "vendor/modules.txt", want: false, versions: pre124},
-		{path: "vendor/modules.txt", want: true, versions: []string{"go1.24.0", "go1.24", "go1.99.0"}},
+		{path: "vendor/modules.txt", want: true, versions: after124},
 
-		// We ideally want these cases to be false, but they are affected by
-		// https://golang.org/issue/37397, and if we fix them we will invalidate
-		// existing module checksums. We must leave them as-is-for now.
-		{path: "pkg/vendor/vendor.go", falsePositive: true},
-		{path: "longpackagename/vendor/vendor.go", falsePositive: true},
+		// These test cases were affected by https://golang.org/issue/37397
+		{path: "pkg/vendor/vendor.go", want: true, versions: pre124},
+		{path: "pkg/vendor/vendor.go", want: false, versions: after124},
+		{path: "longpackagename/vendor/vendor.go", want: true, versions: pre124},
+		{path: "longpackagename/vendor/vendor.go", want: false, versions: after124},
 	} {
 		for _, v := range tc.versions {
 			got := isVendoredPackage(tc.path, v)
 			want := tc.want
-			if tc.falsePositive {
-				want = true
-			}
 			if got != want {
 				t.Errorf("isVendoredPackage(%q, %s) = %t; want %t", tc.path, v, got, tc.want)
 			}
-			if tc.falsePositive {
-				t.Logf("(Expected a false-positive due to https://golang.org/issue/37397.)")
-			}
 		}
 	}
 }
diff --git a/zip/zip.go b/zip/zip.go
@@ -786,8 +786,6 @@ func (fi dataFileInfo) Sys() interface{}   { return nil }
 // in a package whose import path contains (but does not end with) the component
 // "vendor".
 //
-// Unfortunately, isVendoredPackage reports false positives for files in any
-// non-top-level package whose import path ends in "vendor".
 // The 'vers' parameter specifies the Go version declared in the module's
 // go.mod file and must be a valid Go version according to the
 // go/version.IsValid function.
@@ -803,13 +801,27 @@ func isVendoredPackage(name string, vers string) bool {
 	if strings.HasPrefix(name, "vendor/") {
 		i += len("vendor/")
 	} else if j := strings.Index(name, "/vendor/"); j >= 0 {
-		// This offset looks incorrect; this should probably be
+		// Calculate the correct starting position within the import path
+		// to determine if a package is vendored.
 		//
-		// 	i = j + len("/vendor/")
+		// Due to a bug in Go versions before 1.24
+		// (see https://golang.org/issue/37397), the "/vendor/" prefix within
+		// a package path was not always correctly interpreted.
 		//
-		// (See https://golang.org/issue/31562 and https://golang.org/issue/37397.)
-		// Unfortunately, we can't fix it without invalidating module checksums.
-		i += len("/vendor/")
+		// This bug affected how vendored packages were identified in cases like:
+		//
+		//   - "pkg/vendor/vendor.go"   (incorrectly identified as vendored in pre-1.24)
+		//   - "pkg/vendor/foo/foo.go" (correctly identified as vendored)
+		//
+		// To correct this, in Go 1.24 and later, we skip the entire "/vendor/" prefix
+		// when it's part of a nested package path (as in the first example above).
+		// In earlier versions, we only skipped the length of "/vendor/", leading
+		// to the incorrect behavior.
+		if version.Compare(vers, "go1.24") >= 0 {
+			i = j + len("/vendor/")
+		} else {
+			i += len("/vendor/")
+		}
 	} else {
 		return false
 	}
@@ -950,8 +962,7 @@ func listFilesInDir(dir string) (files []File, omitted []FileError, err error) {
 		}
 		slashPath := filepath.ToSlash(relPath)
 
-		// Skip some subdirectories inside vendor, but maintain bug
-		// golang.org/issue/31562, described in isVendoredPackage.
+		// Skip some subdirectories inside vendor.
 		// We would like Create and CreateFromDir to produce the same result
 		// for a set of files, whether expressed as a directory tree or zip.
 		if isVendoredPackage(slashPath, vers) {