From b07f390b1c31ea123de3586b170f1452a0913dda Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 15 Dec 2021 12:08:03 +0000 Subject: [PATCH 1/6] 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 --- modules/git/batch_reader.go | 11 +++++++++++ modules/git/repo_base_nogogit.go | 5 +++++ modules/git/repo_commit_nogogit.go | 5 ++++- modules/indexer/code/bleve.go | 6 ++++++ modules/indexer/code/elastic_search.go | 5 +++++ services/pull/pull.go | 3 ++- 6 files changed, 33 insertions(+), 2 deletions(-) diff --git a/modules/git/batch_reader.go b/modules/git/batch_reader.go index 8e3c23251b635..34e67ad619222 100644 --- a/modules/git/batch_reader.go +++ b/modules/git/batch_reader.go @@ -27,6 +27,17 @@ type WriteCloserError interface { CloseWithError(err error) error } +func EnsureValidGitRepository(ctx context.Context, repoPath string) error { + stderr := strings.Builder{} + err := NewCommandContext(ctx, "rev-parse"). + SetDescription(fmt.Sprintf("%s rev-parse [repo_path: %s]", GitExecutable, repoPath)). + RunInDirFullPipeline(repoPath, nil, &stderr, nil) + if err != nil { + return ConcatenateError(err, (&stderr).String()) + } + return nil +} + // CatFileBatchCheck opens git cat-file --batch-check in the provided repo and returns a stdin pipe, a stdout reader and cancel function func CatFileBatchCheck(repoPath string) (WriteCloserError, *bufio.Reader, func()) { batchStdinReader, batchStdinWriter := io.Pipe() diff --git a/modules/git/repo_base_nogogit.go b/modules/git/repo_base_nogogit.go index 22c4dfdcb3b8d..806887c59b328 100644 --- a/modules/git/repo_base_nogogit.go +++ b/modules/git/repo_base_nogogit.go @@ -43,6 +43,11 @@ func OpenRepository(repoPath string) (*Repository, error) { return nil, errors.New("no such file or directory") } + // 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! + if err := EnsureValidGitRepository(ctx, repoPath); err != nil { + return nil, err + } + repo := &Repository{ Path: repoPath, tagCache: newObjectCache(), diff --git a/modules/git/repo_commit_nogogit.go b/modules/git/repo_commit_nogogit.go index b7e49a6501b17..b109bedee5044 100644 --- a/modules/git/repo_commit_nogogit.go +++ b/modules/git/repo_commit_nogogit.go @@ -38,7 +38,10 @@ func (repo *Repository) ResolveReference(name string) (string, error) { func (repo *Repository) GetRefCommitID(name string) (string, error) { wr, rd, cancel := repo.CatFileBatchCheck() defer cancel() - _, _ = wr.Write([]byte(name + "\n")) + _, err := wr.Write([]byte(name + "\n")) + if err != nil { + return "", err + } shaBs, _, _, err := ReadBatchLine(rd) if IsErrNotExist(err) { return "", ErrNotExist{name, ""} diff --git a/modules/indexer/code/bleve.go b/modules/indexer/code/bleve.go index 4f8ced623214a..55e9a7cb06ca9 100644 --- a/modules/indexer/code/bleve.go +++ b/modules/indexer/code/bleve.go @@ -276,6 +276,12 @@ func (b *BleveIndexer) Index(repo *models.Repository, sha string, changes *repoC batch := gitea_bleve.NewFlushingBatch(b.indexer, maxBatchSize) if len(changes.Updates) > 0 { + // 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! + if err := git.EnsureValidGitRepository(git.DefaultContext, repo.RepoPath()); err != nil { + log.Error("Unable to open git repo: %s for %-v: %v", repo.RepoPath(), repo, err) + return err + } + batchWriter, batchReader, cancel := git.CatFileBatch(repo.RepoPath()) defer cancel() diff --git a/modules/indexer/code/elastic_search.go b/modules/indexer/code/elastic_search.go index 9e5fe506e5e24..822e45807bf9a 100644 --- a/modules/indexer/code/elastic_search.go +++ b/modules/indexer/code/elastic_search.go @@ -248,6 +248,11 @@ func (b *ElasticSearchIndexer) addDelete(filename string, repo *models.Repositor func (b *ElasticSearchIndexer) Index(repo *models.Repository, sha string, changes *repoChanges) error { reqs := make([]elastic.BulkableRequest, 0) if len(changes.Updates) > 0 { + // 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! + if err := git.EnsureValidGitRepository(git.DefaultContext, repo.RepoPath()); err != nil { + log.Error("Unable to open git repo: %s for %-v: %v", repo.RepoPath(), repo, err) + return err + } batchWriter, batchReader, cancel := git.CatFileBatch(repo.RepoPath()) defer cancel() diff --git a/services/pull/pull.go b/services/pull/pull.go index 6b3acd2004254..b938dd1f70ee6 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -713,7 +713,8 @@ func GetIssuesLastCommitStatus(issues models.IssueList) (map[int64]*models.Commi if !ok { gitRepo, err = git.OpenRepository(issue.Repo.RepoPath()) if err != nil { - return nil, err + log.Error("Cannot open git repository %-v for issue #%d[%d]. Error: %v", issue.Repo, issue.Index, issue.ID, err) + continue } gitRepos[issue.RepoID] = gitRepo } From 58947c1fcf9561c3c75351fd82b85c8fb59f1750 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 15 Dec 2021 12:21:58 +0000 Subject: [PATCH 2/6] placate lint Signed-off-by: Andrew Thornton --- modules/git/batch_reader.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/git/batch_reader.go b/modules/git/batch_reader.go index 34e67ad619222..7a2ebcfc57a3e 100644 --- a/modules/git/batch_reader.go +++ b/modules/git/batch_reader.go @@ -27,6 +27,9 @@ type WriteCloserError interface { CloseWithError(err error) error } +// EnsureValidGitRepository runs git rev-parse in the repository path - thus ensuring that the repository is a valid repository. +// Run before opening git cat-file. +// This is needed otherwise the git cat-file will hang for invalid repositories. func EnsureValidGitRepository(ctx context.Context, repoPath string) error { stderr := strings.Builder{} err := NewCommandContext(ctx, "rev-parse"). From 86e036d0db67f40762262fbef656a29e16c36895 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 15 Dec 2021 12:40:51 +0000 Subject: [PATCH 3/6] fix compilation bug Signed-off-by: Andrew Thornton --- modules/git/repo_base_nogogit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/repo_base_nogogit.go b/modules/git/repo_base_nogogit.go index 806887c59b328..4e5b1db112b16 100644 --- a/modules/git/repo_base_nogogit.go +++ b/modules/git/repo_base_nogogit.go @@ -44,7 +44,7 @@ func OpenRepository(repoPath string) (*Repository, error) { } // 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! - if err := EnsureValidGitRepository(ctx, repoPath); err != nil { + if err := EnsureValidGitRepository(DefaultContext, repoPath); err != nil { return nil, err } From fcaea41afe591b1499819158905f335f73b78771 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 15 Dec 2021 13:56:42 +0000 Subject: [PATCH 4/6] Add the missing directories to the testrepos --- models/unit_tests.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/models/unit_tests.go b/models/unit_tests.go index f8d6819333611..c4293d8ec818d 100644 --- a/models/unit_tests.go +++ b/models/unit_tests.go @@ -87,6 +87,26 @@ func MainTest(m *testing.M, pathToGiteaRoot string) { fatalTestError("util.CopyDir: %v\n", err) } + ownerDirs, err := os.ReadDir(pathToGiteaRoot) + if err != nil { + fatalTestError("unable to read the new repo root: %v\n", err) + } + for _, ownerDir := range ownerDirs { + if !ownerDir.Type().IsDir() { + continue + } + repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name())) + if err != nil { + fatalTestError("unable to read the new repo root: %v\n", err) + } + for _, repoDir := range repoDirs { + _ = os.MkdirAll(filepath.Join(pathToGiteaRoot, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755) + _ = os.MkdirAll(filepath.Join(pathToGiteaRoot, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755) + _ = os.MkdirAll(filepath.Join(pathToGiteaRoot, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755) + _ = os.MkdirAll(filepath.Join(pathToGiteaRoot, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755) + } + } + exitStatus := m.Run() if err = util.RemoveAll(setting.RepoRootPath); err != nil { fatalTestError("util.RemoveAll: %v\n", err) @@ -128,6 +148,23 @@ func PrepareTestEnv(t testing.TB) { assert.NoError(t, util.RemoveAll(setting.RepoRootPath)) metaPath := filepath.Join(giteaRoot, "integrations", "gitea-repositories-meta") assert.NoError(t, util.CopyDir(metaPath, setting.RepoRootPath)) + + ownerDirs, err := os.ReadDir(setting.RepoRootPath) + assert.NoError(t, err) + for _, ownerDir := range ownerDirs { + if !ownerDir.Type().IsDir() { + continue + } + repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name())) + assert.NoError(t, err) + for _, repoDir := range repoDirs { + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755) + } + } + base.SetupGiteaRoot() // Makes sure GITEA_ROOT is set } From 4c1b7b66934b26fab8860fd4ac82877d72bd446f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 15 Dec 2021 14:36:52 +0000 Subject: [PATCH 5/6] fixup! Add the missing directories to the testrepos --- models/unit_tests.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/models/unit_tests.go b/models/unit_tests.go index c4293d8ec818d..03c9a2022ac50 100644 --- a/models/unit_tests.go +++ b/models/unit_tests.go @@ -87,7 +87,7 @@ func MainTest(m *testing.M, pathToGiteaRoot string) { fatalTestError("util.CopyDir: %v\n", err) } - ownerDirs, err := os.ReadDir(pathToGiteaRoot) + ownerDirs, err := os.ReadDir(setting.RepoRootPath) if err != nil { fatalTestError("unable to read the new repo root: %v\n", err) } @@ -100,10 +100,10 @@ func MainTest(m *testing.M, pathToGiteaRoot string) { fatalTestError("unable to read the new repo root: %v\n", err) } for _, repoDir := range repoDirs { - _ = os.MkdirAll(filepath.Join(pathToGiteaRoot, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755) - _ = os.MkdirAll(filepath.Join(pathToGiteaRoot, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755) - _ = os.MkdirAll(filepath.Join(pathToGiteaRoot, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755) - _ = os.MkdirAll(filepath.Join(pathToGiteaRoot, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755) } } From 90078fce5cf86b10375bee38bb2ade848e2bc8ec Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Wed, 15 Dec 2021 20:44:24 +0000 Subject: [PATCH 6/6] and ensure that all of the other places have the objects directories too Signed-off-by: Andrew Thornton --- integrations/integration_test.go | 39 +++++++++++++++++++ integrations/migration-test/migration_test.go | 19 +++++++++ models/migrations/migrations_test.go | 19 +++++++++ 3 files changed, 77 insertions(+) diff --git a/integrations/integration_test.go b/integrations/integration_test.go index 8a008ac621760..3b70605c09de3 100644 --- a/integrations/integration_test.go +++ b/integrations/integration_test.go @@ -251,6 +251,26 @@ func prepareTestEnv(t testing.TB, skip ...int) func() { assert.NoError(t, util.RemoveAll(setting.RepoRootPath)) assert.NoError(t, util.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath)) + ownerDirs, err := os.ReadDir(setting.RepoRootPath) + if err != nil { + assert.NoError(t, err, "unable to read the new repo root: %v\n", err) + } + for _, ownerDir := range ownerDirs { + if !ownerDir.Type().IsDir() { + continue + } + repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name())) + if err != nil { + assert.NoError(t, err, "unable to read the new repo root: %v\n", err) + } + for _, repoDir := range repoDirs { + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755) + } + } + return deferFn } @@ -529,4 +549,23 @@ func resetFixtures(t *testing.T) { assert.NoError(t, models.LoadFixtures()) assert.NoError(t, util.RemoveAll(setting.RepoRootPath)) assert.NoError(t, util.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath)) + ownerDirs, err := os.ReadDir(setting.RepoRootPath) + if err != nil { + assert.NoError(t, err, "unable to read the new repo root: %v\n", err) + } + for _, ownerDir := range ownerDirs { + if !ownerDir.Type().IsDir() { + continue + } + repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name())) + if err != nil { + assert.NoError(t, err, "unable to read the new repo root: %v\n", err) + } + for _, repoDir := range repoDirs { + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755) + } + } } diff --git a/integrations/migration-test/migration_test.go b/integrations/migration-test/migration_test.go index 209ff5a058f43..971d52b1bfae3 100644 --- a/integrations/migration-test/migration_test.go +++ b/integrations/migration-test/migration_test.go @@ -61,6 +61,25 @@ func initMigrationTest(t *testing.T) func() { assert.True(t, len(setting.RepoRootPath) != 0) assert.NoError(t, util.RemoveAll(setting.RepoRootPath)) assert.NoError(t, util.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath)) + ownerDirs, err := os.ReadDir(setting.RepoRootPath) + if err != nil { + assert.NoError(t, err, "unable to read the new repo root: %v\n", err) + } + for _, ownerDir := range ownerDirs { + if !ownerDir.Type().IsDir() { + continue + } + repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name())) + if err != nil { + assert.NoError(t, err, "unable to read the new repo root: %v\n", err) + } + for _, repoDir := range repoDirs { + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755) + } + } git.CheckLFSVersion() setting.InitDBConfig() diff --git a/models/migrations/migrations_test.go b/models/migrations/migrations_test.go index 26066580d8978..faab36697d81b 100644 --- a/models/migrations/migrations_test.go +++ b/models/migrations/migrations_test.go @@ -205,6 +205,25 @@ func prepareTestEnv(t *testing.T, skip int, syncModels ...interface{}) (*xorm.En assert.NoError(t, com.CopyDir(path.Join(filepath.Dir(setting.AppPath), "integrations/gitea-repositories-meta"), setting.RepoRootPath)) + ownerDirs, err := os.ReadDir(setting.RepoRootPath) + if err != nil { + assert.NoError(t, err, "unable to read the new repo root: %v\n", err) + } + for _, ownerDir := range ownerDirs { + if !ownerDir.Type().IsDir() { + continue + } + repoDirs, err := os.ReadDir(filepath.Join(setting.RepoRootPath, ownerDir.Name())) + if err != nil { + assert.NoError(t, err, "unable to read the new repo root: %v\n", err) + } + for _, repoDir := range repoDirs { + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "pack"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "objects", "info"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "heads"), 0755) + _ = os.MkdirAll(filepath.Join(setting.RepoRootPath, ownerDir.Name(), repoDir.Name(), "refs", "tag"), 0755) + } + } if err := deleteDB(); err != nil { t.Errorf("unable to reset database: %v", err)