Merge pull request #707 from pjbgf/libgit2_panic

libgit2: recover from git2go panic
fluxcd · May 6, 2022 · d425923 · d425923
2 parents a952c25 + d86ea25
commit d425923
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 5 deletions.
diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go
@@ -61,7 +61,7 @@ type CheckoutBranch struct {
 }
 
 func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (*git.Commit, error) {
-	repo, err := git2go.Clone(url, path, &git2go.CloneOptions{
+	repo, err := safeClone(url, path, &git2go.CloneOptions{
 		FetchOptions: git2go.FetchOptions{
 			DownloadTags:    git2go.DownloadTagsNone,
 			RemoteCallbacks: RemoteCallbacks(ctx, opts),
@@ -94,7 +94,7 @@ type CheckoutTag struct {
 }
 
 func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (*git.Commit, error) {
-	repo, err := git2go.Clone(url, path, &git2go.CloneOptions{
+	repo, err := safeClone(url, path, &git2go.CloneOptions{
 		FetchOptions: git2go.FetchOptions{
 			DownloadTags:    git2go.DownloadTagsAll,
 			RemoteCallbacks: RemoteCallbacks(ctx, opts),
@@ -118,7 +118,7 @@ type CheckoutCommit struct {
 }
 
 func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, opts *git.AuthOptions) (*git.Commit, error) {
-	repo, err := git2go.Clone(url, path, &git2go.CloneOptions{
+	repo, err := safeClone(url, path, &git2go.CloneOptions{
 		FetchOptions: git2go.FetchOptions{
 			DownloadTags:    git2go.DownloadTagsNone,
 			RemoteCallbacks: RemoteCallbacks(ctx, opts),
@@ -150,7 +150,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, opts *g
 		return nil, fmt.Errorf("semver parse error: %w", err)
 	}
 
-	repo, err := git2go.Clone(url, path, &git2go.CloneOptions{
+	repo, err := safeClone(url, path, &git2go.CloneOptions{
 		FetchOptions: git2go.FetchOptions{
 			DownloadTags:    git2go.DownloadTagsAll,
 			RemoteCallbacks: RemoteCallbacks(ctx, opts),
@@ -239,6 +239,19 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, opts *g
 	return buildCommit(cc, "refs/tags/"+t), nil
 }
 
+// safeClone wraps git2go calls with panic recovering logic, ensuring
+// a predictable execution path for callers.
+func safeClone(url, path string, cloneOpts *git2go.CloneOptions) (repo *git2go.Repository, err error) {
+	defer func() {
+		if r := recover(); r != nil {
+			err = fmt.Errorf("recovered from git2go panic: %v", r)
+		}
+	}()
+
+	repo, err = git2go.Clone(url, path, cloneOpts)
+	return
+}
+
 // checkoutDetachedDwim attempts to perform a detached HEAD checkout by first DWIMing the short name
 // to get a concrete reference, and then calling checkoutDetachedHEAD.
 func checkoutDetachedDwim(repo *git2go.Repository, name string) (*git2go.Commit, error) {

diff --git a/pkg/git/libgit2/checkout_test.go b/pkg/git/libgit2/checkout_test.go
@@ -506,3 +506,42 @@ func TestCheckout_ED25519(t *testing.T) {
 	_, err = branchCheckoutStrat.Checkout(ctx, tmpDir, repoURL, authOpts)
 	g.Expect(err).To(BeNil())
 }
+
+func TestSafeClone(t *testing.T) {
+	g := NewWithT(t)
+
+	// Create a git test server.
+	server, err := gittestserver.NewTempGitServer()
+	g.Expect(err).ToNot(HaveOccurred())
+	defer os.RemoveAll(server.Root())
+	server.Auth("test-user", "test-pswd")
+	server.AutoCreate()
+
+	server.KeyDir(filepath.Join(server.Root(), "keys"))
+	g.Expect(server.ListenSSH()).To(Succeed())
+
+	go func() {
+		server.StartSSH()
+	}()
+	defer server.StopSSH()
+
+	sshURL := server.SSHAddress()
+	repoURL := sshURL + "/test.git"
+
+	u, err := url.Parse(sshURL)
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(u.Host).ToNot(BeEmpty())
+
+	repo, err := safeClone(repoURL, t.TempDir(), &git2go.CloneOptions{
+		FetchOptions: git2go.FetchOptions{
+			RemoteCallbacks: git2go.RemoteCallbacks{
+				CertificateCheckCallback: func(cert *git2go.Certificate, valid bool, hostname string) error {
+					panic("Oops!")
+				},
+			},
+		}})
+
+	g.Expect(repo).To(BeNil())
+	g.Expect(err).To(HaveOccurred())
+	g.Expect(err.Error()).Should(ContainSubstring("recovered from git2go panic"))
+}
diff --git a/pkg/minio/minio_test.go b/pkg/minio/minio_test.go
@@ -39,7 +39,7 @@ import (
 const (
 	objectName string = "test.yaml"
 	objectEtag string = "2020beab5f1711919157756379622d1d"
-	region     string = "us-east-1"
+	region     string = "us-west-2"
 )
 
 var (