Skip to content

Commit 33fb820

Browse files
committed
Prevent double use of git cat-file session. (go-gitea#29298)
Fixes the reason why go-gitea#29101 is hard to replicate. Related go-gitea#29297 Create a repo with a file with minimum size 4097 bytes (I use 10000) and execute the following code: ```go gitRepo, err := gitrepo.OpenRepository(db.DefaultContext, <repo>) assert.NoError(t, err) commit, err := gitRepo.GetCommit(<sha>) assert.NoError(t, err) entry, err := commit.GetTreeEntryByPath(<file>) assert.NoError(t, err) b := entry.Blob() // Create a reader r, err := b.DataAsync() assert.NoError(t, err) defer r.Close() // Create a second reader r2, err := b.DataAsync() assert.NoError(t, err) // Should be no error but is ErrNotExist defer r2.Close() ``` The problem is the check in `CatFileBatch`: https://github.com/go-gitea/gitea/blob/79217ea63c1f77de7ca79813ae45950724e63d02/modules/git/repo_base_nogogit.go#L81-L87 `Buffered() > 0` is used to check if there is a "operation" in progress at the moment. This is a problem because we can't control the internal buffer in the `bufio.Reader`. The code above demonstrates a sequence which initiates an operation for which the code thinks there is no active processing. The second call to `DataAsync()` therefore reuses the existing instances instead of creating a new batch reader.
1 parent c4a86b2 commit 33fb820

2 files changed

Lines changed: 59 additions & 6 deletions

File tree

modules/git/repo_base_nogogit.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,12 @@ type Repository struct {
2323

2424
gpgSettings *GPGSettings
2525

26+
batchInUse bool
2627
batchCancel context.CancelFunc
2728
batchReader *bufio.Reader
2829
batchWriter WriteCloserError
2930

31+
checkInUse bool
3032
checkCancel context.CancelFunc
3133
checkReader *bufio.Reader
3234
checkWriter WriteCloserError
@@ -68,23 +70,28 @@ func OpenRepository(ctx context.Context, repoPath string) (*Repository, error) {
6870

6971
// CatFileBatch obtains a CatFileBatch for this repository
7072
func (repo *Repository) CatFileBatch(ctx context.Context) (WriteCloserError, *bufio.Reader, func()) {
71-
if repo.batchCancel == nil || repo.batchReader.Buffered() > 0 {
73+
if repo.batchCancel == nil || repo.batchInUse {
7274
log.Debug("Opening temporary cat file batch for: %s", repo.Path)
7375
return CatFileBatch(ctx, repo.Path)
7476
}
75-
return repo.batchWriter, repo.batchReader, func() {}
77+
repo.batchInUse = true
78+
return repo.batchWriter, repo.batchReader, func() {
79+
repo.batchInUse = false
80+
}
7681
}
7782

7883
// CatFileBatchCheck obtains a CatFileBatchCheck for this repository
7984
func (repo *Repository) CatFileBatchCheck(ctx context.Context) (WriteCloserError, *bufio.Reader, func()) {
80-
if repo.checkCancel == nil || repo.checkReader.Buffered() > 0 {
81-
log.Debug("Opening temporary cat file batch-check: %s", repo.Path)
85+
if repo.checkCancel == nil || repo.checkInUse {
86+
log.Debug("Opening temporary cat file batch-check for: %s", repo.Path)
8287
return CatFileBatchCheck(ctx, repo.Path)
8388
}
84-
return repo.checkWriter, repo.checkReader, func() {}
89+
repo.checkInUse = true
90+
return repo.checkWriter, repo.checkReader, func() {
91+
repo.checkInUse = false
92+
}
8593
}
8694

87-
// Close this repository, in particular close the underlying gogitStorage if this is not nil
8895
func (repo *Repository) Close() (err error) {
8996
if repo == nil {
9097
return nil
@@ -94,12 +101,14 @@ func (repo *Repository) Close() (err error) {
94101
repo.batchReader = nil
95102
repo.batchWriter = nil
96103
repo.batchCancel = nil
104+
repo.batchInUse = false
97105
}
98106
if repo.checkCancel != nil {
99107
repo.checkCancel()
100108
repo.checkCancel = nil
101109
repo.checkReader = nil
102110
repo.checkWriter = nil
111+
repo.checkInUse = false
103112
}
104113
repo.LastCommitCache = nil
105114
repo.tagCache = nil

tests/integration/git_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package integration
55

66
import (
7+
"bytes"
78
"encoding/hex"
89
"fmt"
910
"math/rand"
@@ -25,9 +26,11 @@ import (
2526
user_model "code.gitea.io/gitea/models/user"
2627
gitea_context "code.gitea.io/gitea/modules/context"
2728
"code.gitea.io/gitea/modules/git"
29+
"code.gitea.io/gitea/modules/gitrepo"
2830
"code.gitea.io/gitea/modules/lfs"
2931
"code.gitea.io/gitea/modules/setting"
3032
api "code.gitea.io/gitea/modules/structs"
33+
files_service "code.gitea.io/gitea/services/repository/files"
3134
"code.gitea.io/gitea/tests"
3235

3336
"github.com/stretchr/testify/assert"
@@ -848,3 +851,44 @@ func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, baseBranch, headB
848851
t.Run("CheckoutMasterAgain", doGitCheckoutBranch(dstPath, "master"))
849852
}
850853
}
854+
855+
func TestDataAsync_Issue29101(t *testing.T) {
856+
onGiteaRun(t, func(t *testing.T, u *url.URL) {
857+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
858+
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
859+
860+
resp, err := files_service.ChangeRepoFiles(db.DefaultContext, repo, user, &files_service.ChangeRepoFilesOptions{
861+
Files: []*files_service.ChangeRepoFile{
862+
{
863+
Operation: "create",
864+
TreePath: "test.txt",
865+
ContentReader: bytes.NewReader(make([]byte, 10000)),
866+
},
867+
},
868+
OldBranch: repo.DefaultBranch,
869+
NewBranch: repo.DefaultBranch,
870+
})
871+
assert.NoError(t, err)
872+
873+
sha := resp.Commit.SHA
874+
875+
gitRepo, err := gitrepo.OpenRepository(db.DefaultContext, repo)
876+
assert.NoError(t, err)
877+
878+
commit, err := gitRepo.GetCommit(sha)
879+
assert.NoError(t, err)
880+
881+
entry, err := commit.GetTreeEntryByPath("test.txt")
882+
assert.NoError(t, err)
883+
884+
b := entry.Blob()
885+
886+
r, err := b.DataAsync()
887+
assert.NoError(t, err)
888+
defer r.Close()
889+
890+
r2, err := b.DataAsync()
891+
assert.NoError(t, err)
892+
defer r2.Close()
893+
})
894+
}

0 commit comments

Comments
 (0)