Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent double use of git cat-file session. (#29298) #29301

Merged
merged 4 commits into from
Feb 22, 2024

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Feb 21, 2024

Backport #29298
Fixes the reason why #29101 is hard to replicate.
Related #29297

Create a repo with a file with minimum size 4097 bytes (I use 10000) and execute the following code:

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:

func (repo *Repository) CatFileBatch(ctx context.Context) (WriteCloserError, *bufio.Reader, func()) {
if repo.batchCancel == nil || repo.batchReader.Buffered() > 0 {
log.Debug("Opening temporary cat file batch for: %s", repo.Path)
return CatFileBatch(ctx, repo.Path)
}
return repo.batchWriter, repo.batchReader, func() {}
}
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.

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.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 21, 2024
@GiteaBot GiteaBot added this to the 1.21.6 milestone Feb 21, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 21, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 21, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 22, 2024
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 22, 2024
@silverwind silverwind enabled auto-merge (squash) February 22, 2024 02:33
@silverwind silverwind merged commit c0b97d0 into go-gitea:release/v1.21 Feb 22, 2024
27 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 22, 2024
@KN4CK3R KN4CK3R deleted the backport-f74c869 branch February 24, 2024 22:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants