storage: filesystem, Avoid overwriting loose obj files. Fixes go-git#55

Loose object files are content-addressable and imutable. They should be
created on demand and deleted on repacking. However, they should not be
overwritten - assuming the initial file isn't corrupted.

The previous lack of validation meant those files were being overwritten
when in fact they could just be ignored. In Linux, this was a non-issue,
however, in Windows this operation led to Access Denied errors.

Some additional moving parts of this fix:

- [go-billy](go-git/go-billy#187): Align
  behaviour supporting dir.NewObject():
    - Add support for Chmod in polyfill so that ChrootOS is able to
      chmod files.
    - Ensure temporary directories are created for BoundOS to avoid
      errors when trying to create the temporary file used for loose
      files.
- This PR:
    - Ensure that in Windows, packed and loose object files are created
      as read-only, which in this case means setting the flag
      windows.FILE_ATTRIBUTE_READONLY via x/sys/windows.
    - Skip renaming the temporary file into the existing loose object,
      instead simply delete the temporary file.

Relates to:
- Southclaws/sampctl#422
- git-bug/git-bug#1142
- entireio/cli#455

Signed-off-by: Paulo Gomes <pjbgf@linux.com>

Author: pjbgf

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/emirpasic/gods v1.18.1
 	github.com/gliderlabs/ssh v0.3.8
 	github.com/go-git/gcfg/v2 v2.0.2
-	github.com/go-git/go-billy/v6 v6.0.0-20260114122816-19306b749ecc
+	github.com/go-git/go-billy/v6 v6.0.0-20260226131633-45bd0956d66f
 	github.com/go-git/go-git-fixtures/v5 v5.1.2-0.20260122163445-0622d7459a67
 	github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8
 	github.com/kevinburke/ssh_config v1.5.0
@@ -29,9 +29,6 @@ require (
 	github.com/cyphar/filepath-securejoin v0.6.1 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/klauspost/cpuid/v2 v2.3.0 // indirect
-	github.com/kr/pretty v0.3.1 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
-	github.com/rogpeppe/go-internal v1.14.1 // indirect
-	gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -8,7 +8,6 @@ github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPd
 github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs=
 github.com/cloudflare/circl v1.6.3 h1:9GPOhQGF9MCYUeXyMYlqTR6a5gTrgR/fBLXvUgtVcg8=
 github.com/cloudflare/circl v1.6.3/go.mod h1:2eXP6Qfat4O/Yhh8BznvKnJ+uzEoTQ6jVKJRn81BiS4=
-github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/cyphar/filepath-securejoin v0.6.1 h1:5CeZ1jPXEiYt3+Z6zqprSAgSWiggmpVyciv8syjIpVE=
 github.com/cyphar/filepath-securejoin v0.6.1/go.mod h1:A8hd4EnAeyujCJRrICiOWqjS1AX0a9kM5XL+NwKoYSc=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
@@ -20,8 +19,8 @@ github.com/gliderlabs/ssh v0.3.8 h1:a4YXD1V7xMF9g5nTkdfnja3Sxy1PVDCj1Zg4Wb8vY6c=
 github.com/gliderlabs/ssh v0.3.8/go.mod h1:xYoytBv1sV0aL3CavoDuJIQNURXkkfPA/wxQ1pL1fAU=
 github.com/go-git/gcfg/v2 v2.0.2 h1:MY5SIIfTGGEMhdA7d7JePuVVxtKL7Hp+ApGDJAJ7dpo=
 github.com/go-git/gcfg/v2 v2.0.2/go.mod h1:/lv2NsxvhepuMrldsFilrgct6pxzpGdSRC13ydTLSLs=
-github.com/go-git/go-billy/v6 v6.0.0-20260114122816-19306b749ecc h1:rhkjrnRkamkRC7woapp425E4CAH6RPcqsS9X8LA93IY=
-github.com/go-git/go-billy/v6 v6.0.0-20260114122816-19306b749ecc/go.mod h1:X1oe0Z2qMsa9hkar3AAPuL9hu4Mi3ztXEjdqRhr6fcc=
+github.com/go-git/go-billy/v6 v6.0.0-20260226131633-45bd0956d66f h1:Uvbx7nITO3Sd1GdXarX0TbyYmOaSNIJP0mm4LocEyyA=
+github.com/go-git/go-billy/v6 v6.0.0-20260226131633-45bd0956d66f/go.mod h1:ZW9JC5gionMP1kv5uiaOaV23q0FFmNrVOV8VW+y/acc=
 github.com/go-git/go-git-fixtures/v5 v5.1.2-0.20260122163445-0622d7459a67 h1:3hutPZF+/FBjR/9MdsLJ7e1mlt9pwHgwxMW7CrbmWII=
 github.com/go-git/go-git-fixtures/v5 v5.1.2-0.20260122163445-0622d7459a67/go.mod h1:xKt0pNHST9tYHvbiLxSY27CQWFwgIxBJuDrOE0JvbZw=
 github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 h1:f+oWsMOmNPc8JmEHVZIycC7hBoQxHH9pNKQORJNozsQ=
@@ -30,22 +29,15 @@ github.com/kevinburke/ssh_config v1.5.0 h1:3cPZmE54xb5j3G5xQCjSvokqNwU2uW+3ry1+P
 github.com/kevinburke/ssh_config v1.5.0/go.mod h1:q2RIzfka+BXARoNexmF9gkxEX7DmvbW9P4hIVx2Kg4M=
 github.com/klauspost/cpuid/v2 v2.3.0 h1:S4CRMLnYUhGeDFDqkGriYKdfoFlDnMtqTiI/sFzhA9Y=
 github.com/klauspost/cpuid/v2 v2.3.0/go.mod h1:hqwkgyIinND0mEev00jJYCxPNVRVXFQeu1XKlok6oO0=
+github.com/kr/pretty v0.1.0 h1:L/CwN0zerZDmRFUapSPitk6f+Q3+0za1rQkzVuMiMFI=
 github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
-github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI=
-github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
-github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk=
 github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
+github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
-github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
-github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
 github.com/pjbgf/sha1cd v0.5.0 h1:a+UkboSi1znleCDUNT3M5YxjOnN1fz2FhN48FlwCxs0=
 github.com/pjbgf/sha1cd v0.5.0/go.mod h1:lhpGlyHLpQZoxMv8HcgXvZEhcGs0PG/vsZnEJ7H0iCM=
-github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
-github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs=
-github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ=
-github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc=
 github.com/sergi/go-diff v1.4.0 h1:n/SP9D5ad1fORl+llWyN+D6qoUETXNZARKjyY2/KVCw=
 github.com/sergi/go-diff v1.4.0/go.mod h1:A0bzQcvG0E7Rwjx0REVgAGH58e96+X0MeOfepqsbeW4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
@@ -63,9 +55,8 @@ golang.org/x/term v0.40.0/go.mod h1:w2P8uVp06p2iyKKuvXIm7N/y0UCRt3UfJTfZ7oOpglM=
 golang.org/x/text v0.34.0 h1:oL/Qq0Kdaqxa1KbNeMKwQq0reLCCaFtqu2eNuSeNHbk=
 golang.org/x/text v0.34.0/go.mod h1:homfLqTYRFyVYemLBFl5GgL/DWEiH5wcsQ5gSh1yziA=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 h1:YR8cESwS4TdDjEe65xsg0ogRM/Nc3DYOhEAlW+xobZo=
 gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
-gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
-gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=
 gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=

storage/filesystem/dotgit/dotgit_test.go

@@ -1086,3 +1086,61 @@ func (s *SuiteDotGit) TestSetPackedRef() {
 	s.Require().NoError(err)
 	s.Equal(1, looseCount)
 }
+
+func TestIssue55(t *testing.T) {
+	t.Parallel()
+
+	writeObject := func(fs billy.Filesystem) {
+		t.Helper()
+
+		dir := New(fs)
+		err := dir.Initialize()
+		require.NoError(t, err)
+
+		w, err := dir.NewObject()
+		require.NoError(t, err)
+
+		err = w.WriteHeader(plumbing.BlobObject, 14)
+		require.NoError(t, err)
+		n, err := w.Write([]byte("this is a test"))
+		require.NoError(t, err)
+		assert.Equal(t, 14, n)
+
+		assert.Equal(t, "a8a940627d132695a9769df883f85992f0ff4a43", w.Hash().String())
+
+		err = w.Close()
+		require.NoError(t, err)
+	}
+
+	for _, tc := range []struct {
+		name string
+		fs   billy.Filesystem
+	}{
+		{"BoundOS", osfs.New(t.TempDir(), osfs.WithBoundOS())},
+		{"ChrootOS", osfs.New(t.TempDir(), osfs.WithChrootOS())},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			path := filepath.Join("objects", "a8", "a940627d132695a9769df883f85992f0ff4a43")
+
+			writeObject(tc.fs)
+			i, err := tc.fs.Stat(path)
+			require.NoError(t, err)
+			assert.Equal(t, int64(34), i.Size())
+
+			ro, err := isReadOnly(tc.fs, path)
+			require.NoError(t, err)
+			assert.True(t, ro, "file %q is not read-only", path)
+
+			// Recreate the same object.
+			writeObject(tc.fs)
+			i, err = tc.fs.Stat(path)
+			require.NoError(t, err)
+			assert.Equal(t, int64(34), i.Size())
+
+			ro, err = isReadOnly(tc.fs, path)
+			require.NoError(t, err)
+			assert.True(t, ro, "file %q is not read-only", path)
+		})
+	}
+}

storage/filesystem/dotgit/writers.go

@@ -6,7 +6,7 @@ import (
 	"fmt"
 	"hash"
 	"io"
-	"runtime"
+	"os"
 	"sync/atomic"
 
 	"github.com/go-git/go-billy/v6"
@@ -17,7 +17,6 @@ import (
 	"github.com/go-git/go-git/v6/plumbing/format/objfile"
 	"github.com/go-git/go-git/v6/plumbing/format/packfile"
 	"github.com/go-git/go-git/v6/plumbing/format/revfile"
-	"github.com/go-git/go-git/v6/utils/trace"
 )
 
 // PackWriter is a io.Writer that generates the packfile index simultaneously,
@@ -325,22 +324,17 @@ func (w *ObjectWriter) save() error {
 	hex := h.String()
 	file := w.fs.Join(objectsPath, hex[0:2], hex[2:h.HexSize()])
 
+	// Loose objects are content addressable, if they already exist
+	// we can safely delete the temporary file and short-circuit the
+	// operation.
+	if _, err := w.fs.Stat(file); err == nil || os.IsExist(err) {
+		return w.fs.Remove(w.f.Name())
+	}
+
 	if err := w.fs.Rename(w.f.Name(), file); err != nil {
 		return err
 	}
 	fixPermissions(w.fs, file)
 
 	return nil
 }
-
-func fixPermissions(fs billy.Filesystem, path string) {
-	if runtime.GOOS == "windows" {
-		return
-	}
-
-	if chmodFS, ok := fs.(billy.Chmod); ok {
-		if err := chmodFS.Chmod(path, 0o444); err != nil {
-			trace.General.Printf("failed to chmod %s: %v", path, err)
-		}
-	}
-}

storage/filesystem/dotgit/writers_test.go

@@ -4,9 +4,11 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path/filepath"
 	"strconv"
 	"testing"
 
+	"github.com/go-git/go-billy/v6"
 	"github.com/go-git/go-billy/v6/osfs"
 	"github.com/go-git/go-billy/v6/util"
 	fixtures "github.com/go-git/go-git-fixtures/v5"
@@ -166,56 +168,74 @@ func TestPackWriterUnusedNotify(t *testing.T) {
 func TestPackWriterPermissions(t *testing.T) {
 	t.Parallel()
 
-	f := fixtures.Basic().One()
+	for _, tc := range []struct {
+		name string
+		fs   billy.Filesystem
+	}{
+		{"BoundOS", osfs.New(t.TempDir(), osfs.WithBoundOS())},
+		{"ChrootOS", osfs.New(t.TempDir(), osfs.WithChrootOS())},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
 
-	fs := osfs.New(t.TempDir(), osfs.WithBoundOS())
-	dot := New(fs)
-	require.NoError(t, dot.Initialize())
+			f := fixtures.Basic().One()
 
-	w, err := dot.NewObjectPack()
-	require.NoError(t, err)
+			dot := New(tc.fs)
+			require.NoError(t, dot.Initialize())
 
-	_, err = io.Copy(w, f.Packfile())
-	require.NoError(t, err)
+			w, err := dot.NewObjectPack()
+			require.NoError(t, err)
 
-	require.NoError(t, w.Close())
+			_, err = io.Copy(w, f.Packfile())
+			require.NoError(t, err)
 
-	pfPath := fmt.Sprintf("objects/pack/pack-%s.pack", f.PackfileHash)
-	idxPath := fmt.Sprintf("objects/pack/pack-%s.idx", f.PackfileHash)
-	revPath := fmt.Sprintf("objects/pack/pack-%s.rev", f.PackfileHash)
+			require.NoError(t, w.Close())
 
-	stat, err := fs.Stat(pfPath)
-	require.NoError(t, err)
-	assert.Equal(t, os.FileMode(0o444), stat.Mode().Perm())
+			pfPath := filepath.Join("objects", "pack", fmt.Sprintf("pack-%s.pack", f.PackfileHash))
+			idxPath := filepath.Join("objects", "pack", fmt.Sprintf("pack-%s.idx", f.PackfileHash))
 
-	stat, err = fs.Stat(idxPath)
-	require.NoError(t, err)
-	assert.Equal(t, os.FileMode(0o444), stat.Mode().Perm())
+			ro, err := isReadOnly(tc.fs, pfPath)
+			require.NoError(t, err)
+			assert.True(t, ro, "file %q is not read-only", pfPath)
 
-	stat, err = fs.Stat(revPath)
-	require.NoError(t, err)
-	assert.Equal(t, os.FileMode(0o444), stat.Mode().Perm())
+			ro, err = isReadOnly(tc.fs, idxPath)
+			require.NoError(t, err)
+			assert.True(t, ro, "file %q is not read-only", idxPath)
+		})
+	}
 }
 
 func TestObjectWriterPermissions(t *testing.T) {
 	t.Parallel()
 
-	fs := osfs.New(t.TempDir(), osfs.WithBoundOS())
-	dot := New(fs)
-	require.NoError(t, dot.Initialize())
+	for _, tc := range []struct {
+		name string
+		fs   billy.Filesystem
+	}{
+		{"BoundOS", osfs.New(t.TempDir(), osfs.WithBoundOS())},
+		{"ChrootOS", osfs.New(t.TempDir(), osfs.WithChrootOS())},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
 
-	w, err := dot.NewObject()
-	require.NoError(t, err)
+			dot := New(tc.fs)
+			require.NoError(t, dot.Initialize())
 
-	err = w.WriteHeader(plumbing.BlobObject, 14)
-	require.NoError(t, err)
+			w, err := dot.NewObject()
+			require.NoError(t, err)
 
-	_, err = w.Write([]byte("this is a test"))
-	require.NoError(t, err)
+			err = w.WriteHeader(plumbing.BlobObject, 14)
+			require.NoError(t, err)
 
-	require.NoError(t, w.Close())
+			_, err = w.Write([]byte("this is a test"))
+			require.NoError(t, err)
 
-	stat, err := fs.Stat("objects/a8/a940627d132695a9769df883f85992f0ff4a43")
-	require.NoError(t, err)
-	assert.Equal(t, os.FileMode(0o444), stat.Mode().Perm())
+			require.NoError(t, w.Close())
+
+			path := filepath.Join("objects", "a8", "a940627d132695a9769df883f85992f0ff4a43")
+			ro, err := isReadOnly(tc.fs, path)
+			require.NoError(t, err)
+			assert.True(t, ro, "file %q is not read-only", path)
+		})
+	}
 }

storage/filesystem/dotgit/writers_unix.go

@@ -0,0 +1,32 @@
+//go:build !windows
+
+package dotgit
+
+import (
+	"github.com/go-git/go-billy/v6"
+
+	"github.com/go-git/go-git/v6/utils/trace"
+)
+
+const readOnly = 0o444
+
+func fixPermissions(fs billy.Filesystem, path string) {
+	if chmodFS, ok := fs.(billy.Chmod); ok {
+		if err := chmodFS.Chmod(path, readOnly); err != nil {
+			trace.General.Printf("failed to chmod %s: %v", path, err)
+		}
+	}
+}
+
+func isReadOnly(fs billy.Filesystem, path string) (bool, error) {
+	fi, err := fs.Stat(path)
+	if err != nil {
+		return false, err
+	}
+
+	if fi.Mode().Perm() == readOnly {
+		return true, nil
+	}
+
+	return false, nil
+}

storage/filesystem/dotgit/writers_windows.go

@@ -0,0 +1,58 @@
+//go:build windows
+
+package dotgit
+
+import (
+	"fmt"
+	"path/filepath"
+
+	"github.com/go-git/go-billy/v6"
+	"golang.org/x/sys/windows"
+
+	"github.com/go-git/go-git/v6/utils/trace"
+)
+
+func fixPermissions(fs billy.Filesystem, path string) {
+	fullpath := filepath.Join(fs.Root(), path)
+	p, err := windows.UTF16PtrFromString(fullpath)
+	if err != nil {
+		trace.General.Printf("failed to chmod %s: %v", fullpath, err)
+		return
+	}
+
+	attrs, err := windows.GetFileAttributes(p)
+	if err != nil {
+		trace.General.Printf("failed to chmod %s: %v", fullpath, err)
+		return
+	}
+
+	if attrs&windows.FILE_ATTRIBUTE_READONLY != 0 {
+		return
+	}
+
+	err = windows.SetFileAttributes(p,
+		attrs|windows.FILE_ATTRIBUTE_READONLY,
+	)
+	if err != nil {
+		trace.General.Printf("failed to chmod %s: %v", fullpath, err)
+	}
+}
+
+func isReadOnly(fs billy.Filesystem, path string) (bool, error) {
+	fullpath := filepath.Join(fs.Root(), path)
+	p, err := windows.UTF16PtrFromString(fullpath)
+	if err != nil {
+		return false, fmt.Errorf("%w: %q", err, fullpath)
+	}
+
+	attrs, err := windows.GetFileAttributes(p)
+	if err != nil {
+		return false, fmt.Errorf("%w: %q", err, fullpath)
+	}
+
+	if attrs&windows.FILE_ATTRIBUTE_READONLY != 0 {
+		return true, nil
+	}
+
+	return false, nil
+}