Skip to content

Commit 5290e52

Browse files
pjbgfpjbgf
committed
storage: filesystem, Avoid overwriting loose obj files. Fixes #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 <[email protected]>
1 parent 5d20a62 commit 5290e52

7 files changed

Lines changed: 212 additions & 47 deletions

File tree

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ require (
1313
github.com/emirpasic/gods v1.18.1
1414
github.com/gliderlabs/ssh v0.3.8
1515
github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376
16-
github.com/go-git/go-billy/v5 v5.7.0
16+
github.com/go-git/go-billy/v5 v5.8.0
1717
github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399
1818
github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8
1919
github.com/google/go-cmp v0.7.0

‎go.sum‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ github.com/gliderlabs/ssh v0.3.8 h1:a4YXD1V7xMF9g5nTkdfnja3Sxy1PVDCj1Zg4Wb8vY6c=
2525
github.com/gliderlabs/ssh v0.3.8/go.mod h1:xYoytBv1sV0aL3CavoDuJIQNURXkkfPA/wxQ1pL1fAU=
2626
github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 h1:+zs/tPmkDkHx3U66DAb0lQFJrpS6731Oaa12ikc+DiI=
2727
github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376/go.mod h1:an3vInlBmSxCcxctByoQdvwPiA7DTK7jaaFDBTtu0ic=
28-
github.com/go-git/go-billy/v5 v5.7.0 h1:83lBUJhGWhYp0ngzCMSgllhUSuoHP1iEWYjsPl9nwqM=
29-
github.com/go-git/go-billy/v5 v5.7.0/go.mod h1:/1IUejTKH8xipsAcdfcSAlUlo2J7lkYV8GTKxAT/L3E=
28+
github.com/go-git/go-billy/v5 v5.8.0 h1:I8hjc3LbBlXTtVuFNJuwYuMiHvQJDq1AT6u4DwDzZG0=
29+
github.com/go-git/go-billy/v5 v5.8.0/go.mod h1:RpvI/rw4Vr5QA+Z60c6d6LXH0rYJo0uD5SqfmrrheCY=
3030
github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399 h1:eMje31YglSBqCdIqdhKBW8lokaMrL3uTkpGYlE2OOT4=
3131
github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399/go.mod h1:1OCfN199q1Jm3HZlxleg+Dw/mwps2Wbk9frAWm+4FII=
3232
github.com/golang/groupcache v0.0.0-20241129210726-2c02b8208cf8 h1:f+oWsMOmNPc8JmEHVZIycC7hBoQxHH9pNKQORJNozsQ=

‎storage/filesystem/dotgit/dotgit_test.go‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/go-git/go-git/v5/plumbing"
1919
"github.com/go-git/go-git/v5/storage"
2020
"github.com/stretchr/testify/assert"
21+
"github.com/stretchr/testify/require"
2122
. "gopkg.in/check.v1"
2223
)
2324

@@ -1094,3 +1095,61 @@ func (s *SuiteDotGit) TestSetPackedRef(c *C) {
10941095
c.Assert(err, IsNil)
10951096
c.Assert(looseCount, Equals, 1)
10961097
}
1098+
1099+
func TestIssue55(t *testing.T) {
1100+
t.Parallel()
1101+
1102+
writeObject := func(fs billy.Filesystem) {
1103+
t.Helper()
1104+
1105+
dir := New(fs)
1106+
err := dir.Initialize()
1107+
require.NoError(t, err)
1108+
1109+
w, err := dir.NewObject()
1110+
require.NoError(t, err)
1111+
1112+
err = w.WriteHeader(plumbing.BlobObject, 14)
1113+
require.NoError(t, err)
1114+
n, err := w.Write([]byte("this is a test"))
1115+
require.NoError(t, err)
1116+
assert.Equal(t, 14, n)
1117+
1118+
assert.Equal(t, "a8a940627d132695a9769df883f85992f0ff4a43", w.Hash().String())
1119+
1120+
err = w.Close()
1121+
require.NoError(t, err)
1122+
}
1123+
1124+
for _, tc := range []struct {
1125+
name string
1126+
fs billy.Filesystem
1127+
}{
1128+
{"BoundOS", osfs.New(t.TempDir(), osfs.WithBoundOS())},
1129+
{"ChrootOS", osfs.New(t.TempDir(), osfs.WithChrootOS())},
1130+
} {
1131+
t.Run(tc.name, func(t *testing.T) {
1132+
t.Parallel()
1133+
path := filepath.Join("objects", "a8", "a940627d132695a9769df883f85992f0ff4a43")
1134+
1135+
writeObject(tc.fs)
1136+
i, err := tc.fs.Stat(path)
1137+
require.NoError(t, err)
1138+
assert.Equal(t, int64(34), i.Size())
1139+
1140+
ro, err := isReadOnly(tc.fs, path)
1141+
require.NoError(t, err)
1142+
assert.True(t, ro, "file %q is not read-only", path)
1143+
1144+
// Recreate the same object.
1145+
writeObject(tc.fs)
1146+
i, err = tc.fs.Stat(path)
1147+
require.NoError(t, err)
1148+
assert.Equal(t, int64(34), i.Size())
1149+
1150+
ro, err = isReadOnly(tc.fs, path)
1151+
require.NoError(t, err)
1152+
assert.True(t, ro, "file %q is not read-only", path)
1153+
})
1154+
}
1155+
}

‎storage/filesystem/dotgit/writers.go‎

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,14 @@ package dotgit
33
import (
44
"fmt"
55
"io"
6-
"runtime"
6+
"os"
77
"sync/atomic"
88

99
"github.com/go-git/go-git/v5/plumbing"
1010
"github.com/go-git/go-git/v5/plumbing/format/idxfile"
1111
"github.com/go-git/go-git/v5/plumbing/format/objfile"
1212
"github.com/go-git/go-git/v5/plumbing/format/packfile"
1313
"github.com/go-git/go-git/v5/plumbing/hash"
14-
"github.com/go-git/go-git/v5/utils/trace"
1514

1615
"github.com/go-git/go-billy/v5"
1716
)
@@ -291,22 +290,17 @@ func (w *ObjectWriter) save() error {
291290
hex := w.Hash().String()
292291
file := w.fs.Join(objectsPath, hex[0:2], hex[2:hash.HexSize])
293292

293+
// Loose objects are content addressable, if they already exist
294+
// we can safely delete the temporary file and short-circuit the
295+
// operation.
296+
if _, err := w.fs.Stat(file); err == nil || os.IsExist(err) {
297+
return w.fs.Remove(w.f.Name())
298+
}
299+
294300
if err := w.fs.Rename(w.f.Name(), file); err != nil {
295301
return err
296302
}
297303
fixPermissions(w.fs, file)
298304

299305
return nil
300306
}
301-
302-
func fixPermissions(fs billy.Filesystem, path string) {
303-
if runtime.GOOS == "windows" {
304-
return
305-
}
306-
307-
if chmodFS, ok := fs.(billy.Chmod); ok {
308-
if err := chmodFS.Chmod(path, 0o444); err != nil {
309-
trace.General.Printf("failed to chmod %s: %v", path, err)
310-
}
311-
}
312-
}

‎storage/filesystem/dotgit/writers_test.go‎

Lines changed: 55 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@ import (
44
"fmt"
55
"io"
66
"os"
7+
"path/filepath"
78
"strconv"
89
"testing"
910

11+
"github.com/go-git/go-billy/v5"
1012
"github.com/go-git/go-billy/v5/osfs"
1113
"github.com/go-git/go-billy/v5/util"
1214
"github.com/go-git/go-git/v5/plumbing"
@@ -142,51 +144,74 @@ func (s *SuiteDotGit) TestPackWriterUnusedNotify(c *C) {
142144
func TestPackWriterPermissions(t *testing.T) {
143145
t.Parallel()
144146

145-
f := fixtures.Basic().One()
147+
for _, tc := range []struct {
148+
name string
149+
fs billy.Filesystem
150+
}{
151+
{"BoundOS", osfs.New(t.TempDir(), osfs.WithBoundOS())},
152+
{"ChrootOS", osfs.New(t.TempDir(), osfs.WithChrootOS())},
153+
} {
154+
t.Run(tc.name, func(t *testing.T) {
155+
t.Parallel()
146156

147-
fs := osfs.New(t.TempDir(), osfs.WithBoundOS())
148-
dot := New(fs)
149-
require.NoError(t, dot.Initialize())
157+
f := fixtures.Basic().One()
150158

151-
w, err := dot.NewObjectPack()
152-
require.NoError(t, err)
159+
dot := New(tc.fs)
160+
require.NoError(t, dot.Initialize())
153161

154-
_, err = io.Copy(w, f.Packfile())
155-
require.NoError(t, err)
162+
w, err := dot.NewObjectPack()
163+
require.NoError(t, err)
156164

157-
require.NoError(t, w.Close())
165+
_, err = io.Copy(w, f.Packfile())
166+
require.NoError(t, err)
158167

159-
pfPath := fmt.Sprintf("objects/pack/pack-%s.pack", f.PackfileHash)
160-
idxPath := fmt.Sprintf("objects/pack/pack-%s.idx", f.PackfileHash)
168+
require.NoError(t, w.Close())
161169

162-
stat, err := fs.Stat(pfPath)
163-
require.NoError(t, err)
164-
assert.Equal(t, os.FileMode(0o444), stat.Mode().Perm())
170+
pfPath := filepath.Join("objects", "pack", fmt.Sprintf("pack-%s.pack", f.PackfileHash))
171+
idxPath := filepath.Join("objects", "pack", fmt.Sprintf("pack-%s.idx", f.PackfileHash))
165172

166-
stat, err = fs.Stat(idxPath)
167-
require.NoError(t, err)
168-
assert.Equal(t, os.FileMode(0o444), stat.Mode().Perm())
173+
ro, err := isReadOnly(tc.fs, pfPath)
174+
require.NoError(t, err)
175+
assert.True(t, ro, "file %q is not read-only", pfPath)
176+
177+
ro, err = isReadOnly(tc.fs, idxPath)
178+
require.NoError(t, err)
179+
assert.True(t, ro, "file %q is not read-only", idxPath)
180+
})
181+
}
169182
}
170183

171184
func TestObjectWriterPermissions(t *testing.T) {
172185
t.Parallel()
173186

174-
fs := osfs.New(t.TempDir(), osfs.WithBoundOS())
175-
dot := New(fs)
176-
require.NoError(t, dot.Initialize())
187+
for _, tc := range []struct {
188+
name string
189+
fs billy.Filesystem
190+
}{
191+
{"BoundOS", osfs.New(t.TempDir(), osfs.WithBoundOS())},
192+
{"ChrootOS", osfs.New(t.TempDir(), osfs.WithChrootOS())},
193+
} {
194+
t.Run(tc.name, func(t *testing.T) {
195+
t.Parallel()
196+
197+
dot := New(tc.fs)
198+
require.NoError(t, dot.Initialize())
177199

178-
w, err := dot.NewObject()
179-
require.NoError(t, err)
200+
w, err := dot.NewObject()
201+
require.NoError(t, err)
180202

181-
err = w.WriteHeader(plumbing.BlobObject, 14)
182-
require.NoError(t, err)
203+
err = w.WriteHeader(plumbing.BlobObject, 14)
204+
require.NoError(t, err)
183205

184-
_, err = w.Write([]byte("this is a test"))
185-
require.NoError(t, err)
206+
_, err = w.Write([]byte("this is a test"))
207+
require.NoError(t, err)
186208

187-
require.NoError(t, w.Close())
209+
require.NoError(t, w.Close())
188210

189-
stat, err := fs.Stat("objects/a8/a940627d132695a9769df883f85992f0ff4a43")
190-
require.NoError(t, err)
191-
assert.Equal(t, os.FileMode(0o444), stat.Mode().Perm())
211+
path := filepath.Join("objects", "a8", "a940627d132695a9769df883f85992f0ff4a43")
212+
ro, err := isReadOnly(tc.fs, path)
213+
require.NoError(t, err)
214+
assert.True(t, ro, "file %q is not read-only", path)
215+
})
216+
}
192217
}

‎storage/filesystem/dotgit/writers_unix.go‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
//go:build !windows
2+
3+
package dotgit
4+
5+
import (
6+
"github.com/go-git/go-billy/v5"
7+
"github.com/go-git/go-git/v5/utils/trace"
8+
)
9+
10+
func fixPermissions(fs billy.Filesystem, path string) {
11+
if chmodFS, ok := fs.(billy.Chmod); ok {
12+
if err := chmodFS.Chmod(path, 0o444); err != nil {
13+
trace.General.Printf("failed to chmod %s: %v", path, err)
14+
}
15+
}
16+
}
17+
18+
func isReadOnly(fs billy.Filesystem, path string) (bool, error) {
19+
fi, err := fs.Stat(path)
20+
if err != nil {
21+
return false, err
22+
}
23+
24+
if fi.Mode().Perm() == 0o444 {
25+
return true, nil
26+
}
27+
28+
return false, nil
29+
}

‎storage/filesystem/dotgit/writers_windows.go‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
//go:build windows
2+
3+
package dotgit
4+
5+
import (
6+
"fmt"
7+
"path/filepath"
8+
9+
"github.com/go-git/go-billy/v5"
10+
"github.com/go-git/go-git/v5/utils/trace"
11+
"golang.org/x/sys/windows"
12+
)
13+
14+
func fixPermissions(fs billy.Filesystem, path string) {
15+
fullpath := filepath.Join(fs.Root(), path)
16+
p, err := windows.UTF16PtrFromString(fullpath)
17+
if err != nil {
18+
trace.General.Printf("failed to chmod %s: %v", fullpath, err)
19+
return
20+
}
21+
22+
attrs, err := windows.GetFileAttributes(p)
23+
if err != nil {
24+
trace.General.Printf("failed to chmod %s: %v", fullpath, err)
25+
return
26+
}
27+
28+
if attrs&windows.FILE_ATTRIBUTE_READONLY != 0 {
29+
return
30+
}
31+
32+
err = windows.SetFileAttributes(p,
33+
attrs|windows.FILE_ATTRIBUTE_READONLY,
34+
)
35+
36+
if err != nil {
37+
trace.General.Printf("failed to chmod %s: %v", fullpath, err)
38+
}
39+
}
40+
41+
func isReadOnly(fs billy.Filesystem, path string) (bool, error) {
42+
fullpath := filepath.Join(fs.Root(), path)
43+
p, err := windows.UTF16PtrFromString(fullpath)
44+
if err != nil {
45+
return false, fmt.Errorf("%w: %q", err, fullpath)
46+
}
47+
48+
attrs, err := windows.GetFileAttributes(p)
49+
if err != nil {
50+
return false, fmt.Errorf("%w: %q", err, fullpath)
51+
}
52+
53+
if attrs&windows.FILE_ATTRIBUTE_READONLY != 0 {
54+
return true, nil
55+
}
56+
57+
return false, nil
58+
}

0 commit comments

Comments
 (0)