Skip to content

Commit 3ae4c48

Browse files
zeripathtechknowlogick
zeripath
and
techknowlogick
authored
Prevent hang in git cat-file if the repository is not a valid repository (Partial #17991) (#17992)
* Prevent hang in git cat-file if the repository is not a valid repository (Partial #17991) Unfortunately it appears that if git cat-file is run in an invalid repository it will hang until stdin is closed. This will result in deadlocked /pulls pages and dangling git cat-file calls if a broken repository is tried to be reviewed or pulls exists for a broken repository. Signed-off-by: Andrew Thornton <art27@cantab.net> * placate lint Signed-off-by: Andrew Thornton <art27@cantab.net> * fix compilation bug Signed-off-by: Andrew Thornton <art27@cantab.net> * Add the missing directories to the testrepos * fixup! Add the missing directories to the testrepos * and ensure that all of the other places have the objects directories too Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: techknowlogick <techknowlogick@gitea.io>
1 parent 3a77465 commit 3ae4c48

File tree

10 files changed

+150
-2
lines changed

10 files changed

+150
-2
lines changed

‎integrations/integration_test.go

+39
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,26 @@ func prepareTestEnv(t testing.TB, skip ...int) func() {
251251
assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
252252

253253
assert.NoError(t, util.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath))
254+
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
255+
if err != nil {
256+
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
257+
}
258+
for _, ownerDir := range ownerDirs {
259+
if !ownerDir.Type().IsDir() {
260+
continue
261+
}
262+
repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name()))
263+
if err != nil {
264+
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
265+
}
266+
for _, repoDir := range repoDirs {
267+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755)
268+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755)
269+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755)
270+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755)
271+
}
272+
}
273+
254274
return deferFn
255275
}
256276

@@ -529,4 +549,23 @@ func resetFixtures(t *testing.T) {
529549
assert.NoError(t, models.LoadFixtures())
530550
assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
531551
assert.NoError(t, util.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath))
552+
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
553+
if err != nil {
554+
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
555+
}
556+
for _, ownerDir := range ownerDirs {
557+
if !ownerDir.Type().IsDir() {
558+
continue
559+
}
560+
repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name()))
561+
if err != nil {
562+
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
563+
}
564+
for _, repoDir := range repoDirs {
565+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755)
566+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755)
567+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755)
568+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755)
569+
}
570+
}
532571
}

‎integrations/migration-test/migration_test.go

+19
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,25 @@ func initMigrationTest(t *testing.T) func() {
6161
assert.True(t, len(setting.RepoRootPath) != 0)
6262
assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
6363
assert.NoError(t, util.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath))
64+
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
65+
if err != nil {
66+
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
67+
}
68+
for _, ownerDir := range ownerDirs {
69+
if !ownerDir.Type().IsDir() {
70+
continue
71+
}
72+
repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name()))
73+
if err != nil {
74+
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
75+
}
76+
for _, repoDir := range repoDirs {
77+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755)
78+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755)
79+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755)
80+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755)
81+
}
82+
}
6483

6584
git.CheckLFSVersion()
6685
setting.InitDBConfig()

‎models/migrations/migrations_test.go

+19
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,25 @@ func prepareTestEnv(t *testing.T, skip int, syncModels ...interface{}) (*xorm.En
205205

206206
assert.NoError(t, com.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"),
207207
setting.RepoRootPath))
208+
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
209+
if err != nil {
210+
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
211+
}
212+
for _, ownerDir := range ownerDirs {
213+
if !ownerDir.Type().IsDir() {
214+
continue
215+
}
216+
repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name()))
217+
if err != nil {
218+
assert.NoError(t, err, "unable to read the new repo root: %v\n", err)
219+
}
220+
for _, repoDir := range repoDirs {
221+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755)
222+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755)
223+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755)
224+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755)
225+
}
226+
}
208227

209228
if err := deleteDB(); err != nil {
210229
t.Errorf("unable to reset database: %v", err)

‎models/unit_tests.go

+37
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,26 @@ func MainTest(m *testing.M, pathToGiteaRoot string) {
8787
fatalTestError("util.CopyDir: %v\n", err)
8888
}
8989

90+
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
91+
if err != nil {
92+
fatalTestError("unable to read the new repo root: %v\n", err)
93+
}
94+
for _, ownerDir := range ownerDirs {
95+
if !ownerDir.Type().IsDir() {
96+
continue
97+
}
98+
repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name()))
99+
if err != nil {
100+
fatalTestError("unable to read the new repo root: %v\n", err)
101+
}
102+
for _, repoDir := range repoDirs {
103+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755)
104+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755)
105+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755)
106+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755)
107+
}
108+
}
109+
90110
exitStatus := m.Run()
91111
if err = util.RemoveAll(setting.RepoRootPath); err != nil {
92112
fatalTestError("util.RemoveAll: %v\n", err)
@@ -128,6 +148,23 @@ func PrepareTestEnv(t testing.TB) {
128148
assert.NoError(t, util.RemoveAll(setting.RepoRootPath))
129149
metaPath := filepath.Join(giteaRoot, "integrations", "gitea-repositories-meta")
130150
assert.NoError(t, util.CopyDir(metaPath, setting.RepoRootPath))
151+
152+
ownerDirs, err := os.ReadDir(setting.RepoRootPath)
153+
assert.NoError(t, err)
154+
for _, ownerDir := range ownerDirs {
155+
if !ownerDir.Type().IsDir() {
156+
continue
157+
}
158+
repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name()))
159+
assert.NoError(t, err)
160+
for _, repoDir := range repoDirs {
161+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755)
162+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755)
163+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755)
164+
_ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755)
165+
}
166+
}
167+
131168
base.SetupGiteaRoot() // Makes sure GITEA_ROOT is set
132169
}
133170

‎modules/git/batch_reader.go

+14
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,20 @@ type WriteCloserError interface {
2727
CloseWithError(err error) error
2828
}
2929

30+
// EnsureValidGitRepository runs git rev-parse in the repository path - thus ensuring that the repository is a valid repository.
31+
// Run before opening git cat-file.
32+
// This is needed otherwise the git cat-file will hang for invalid repositories.
33+
func EnsureValidGitRepository(ctx context.Context, repoPath string) error {
34+
stderr := strings.Builder{}
35+
err := NewCommandContext(ctx, "rev-parse").
36+
SetDescription(fmt.Sprintf("%s rev-parse [repo_path: %s]", GitExecutable, repoPath)).
37+
RunInDirFullPipeline(repoPath, nil, &stderr, nil)
38+
if err != nil {
39+
return ConcatenateError(err, (&stderr).String())
40+
}
41+
return nil
42+
}
43+
3044
// CatFileBatchCheck opens git cat-file --batch-check in the provided repo and returns a stdin pipe, a stdout reader and cancel function
3145
func CatFileBatchCheck(repoPath string) (WriteCloserError, *bufio.Reader, func()) {
3246
batchStdinReader, batchStdinWriter := io.Pipe()

‎modules/git/repo_base_nogogit.go

+5
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,11 @@ func OpenRepository(repoPath string) (*Repository, error) {
4343
return nil, errors.New("no such file or directory")
4444
}
4545

46+
// Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first!
47+
if err := EnsureValidGitRepository(DefaultContext, repoPath); err != nil {
48+
return nil, err
49+
}
50+
4651
repo := &Repository{
4752
Path: repoPath,
4853
tagCache: newObjectCache(),

‎modules/git/repo_commit_nogogit.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,10 @@ func (repo *Repository) ResolveReference(name string) (string, error) {
3838
func (repo *Repository) GetRefCommitID(name string) (string, error) {
3939
wr, rd, cancel := repo.CatFileBatchCheck()
4040
defer cancel()
41-
_, _ = wr.Write([]byte(name + "\n"))
41+
_, err := wr.Write([]byte(name + "\n"))
42+
if err != nil {
43+
return "", err
44+
}
4245
shaBs, _, _, err := ReadBatchLine(rd)
4346
if IsErrNotExist(err) {
4447
return "", ErrNotExist{name, ""}

‎modules/indexer/code/bleve.go

+6
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,12 @@ func (b *BleveIndexer) Index(repo *models.Repository, sha string, changes *repoC
276276
batch := gitea_bleve.NewFlushingBatch(b.indexer, maxBatchSize)
277277
if len(changes.Updates) > 0 {
278278

279+
// Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first!
280+
if err := git.EnsureValidGitRepository(git.DefaultContext, repo.RepoPath()); err != nil {
281+
log.Error("Unable to open git repo: %s for %-v: %v", repo.RepoPath(), repo, err)
282+
return err
283+
}
284+
279285
batchWriter, batchReader, cancel := git.CatFileBatch(repo.RepoPath())
280286
defer cancel()
281287

‎modules/indexer/code/elastic_search.go

+5
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,11 @@ func (b *ElasticSearchIndexer) addDelete(filename string, repo *models.Repositor
248248
func (b *ElasticSearchIndexer) Index(repo *models.Repository, sha string, changes *repoChanges) error {
249249
reqs := make([]elastic.BulkableRequest, 0)
250250
if len(changes.Updates) > 0 {
251+
// Now because of some insanity with git cat-file not immediately failing if not run in a valid git directory we need to run git rev-parse first!
252+
if err := git.EnsureValidGitRepository(git.DefaultContext, repo.RepoPath()); err != nil {
253+
log.Error("Unable to open git repo: %s for %-v: %v", repo.RepoPath(), repo, err)
254+
return err
255+
}
251256

252257
batchWriter, batchReader, cancel := git.CatFileBatch(repo.RepoPath())
253258
defer cancel()

‎services/pull/pull.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -713,7 +713,8 @@ func GetIssuesLastCommitStatus(issues models.IssueList) (map[int64]*models.Commi
713713
if !ok {
714714
gitRepo, err = git.OpenRepository(issue.Repo.RepoPath())
715715
if err != nil {
716-
return nil, err
716+
log.Error("Cannot open git repository %-v for issue #%d[%d]. Error: %v", issue.Repo, issue.Index, issue.ID, err)
717+
continue
717718
}
718719
gitRepos[issue.RepoID] = gitRepo
719720
}

0 commit comments

Comments
 (0)