From 6b22b0ee42448ffb5692da46b3ab392ccc1def75 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 9 Nov 2021 14:29:11 +0100 Subject: [PATCH 1/8] Fix nil checking on typed interface - Partially resoles #17596 - Resolves SA4023 errors. - Ensure correctly that typed interface are nil. --- modules/indexer/code/indexer.go | 5 +++-- modules/util/util.go | 12 ++++++++++++ modules/util/util_test.go | 27 +++++++++++++++++++++++++++ routers/web/base.go | 3 ++- 4 files changed, 44 insertions(+), 3 deletions(-) diff --git a/modules/indexer/code/indexer.go b/modules/indexer/code/indexer.go index 981167a8254a5..0152212aff445 100644 --- a/modules/indexer/code/indexer.go +++ b/modules/indexer/code/indexer.go @@ -18,6 +18,7 @@ import ( "code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" + giteautil "code.gitea.io/gitea/modules/util" ) // SearchResult result of performing a search in a repo @@ -185,7 +186,7 @@ func Init() { rIndexer, populate, err = NewBleveIndexer(setting.Indexer.RepoPath) if err != nil { - if rIndexer != nil { + if !giteautil.IsInterfaceNil(rIndexer) { rIndexer.Close() } cancel() @@ -205,7 +206,7 @@ func Init() { rIndexer, populate, err = NewElasticSearchIndexer(setting.Indexer.RepoConnStr, setting.Indexer.RepoIndexerName) if err != nil { - if rIndexer != nil { + if !giteautil.IsInterfaceNil(rIndexer) { rIndexer.Close() } cancel() diff --git a/modules/util/util.go b/modules/util/util.go index cbc6eb4f8a01c..68a2e97423279 100644 --- a/modules/util/util.go +++ b/modules/util/util.go @@ -9,6 +9,7 @@ import ( "crypto/rand" "errors" "math/big" + "reflect" "strconv" "strings" ) @@ -161,3 +162,14 @@ func RandomString(length int64) (string, error) { } return string(bytes), nil } + +// IsInterfaceNil checks if a variable with a typed interface is nil. +// Because interface{} == nil is always false. +func IsInterfaceNil(i interface{}) bool { + switch reflect.TypeOf(i).Kind() { + case reflect.Ptr, reflect.Map, reflect.Array, reflect.Chan, reflect.Slice: + return reflect.ValueOf(i).IsNil() + default: + return false + } +} diff --git a/modules/util/util_test.go b/modules/util/util_test.go index 39cf07c85543e..e0d8439c876e8 100644 --- a/modules/util/util_test.go +++ b/modules/util/util_test.go @@ -169,3 +169,30 @@ func Test_OptionalBool(t *testing.T) { assert.Equal(t, OptionalBoolTrue, OptionalBoolParse("t")) assert.Equal(t, OptionalBoolTrue, OptionalBoolParse("True")) } + +type SampleInterface interface { + Hello() +} + +type BasicInterface int + +func (BasicInterface) Hello() { +} + +func returnFilledBasicInterface() BasicInterface { + return BasicInterface(123123) +} + +func returnNil() *BasicInterface { + return nil +} + +func TestTypedInterfaceNil(t *testing.T) { + var info SampleInterface + + info = returnNil() + assert.Equal(t, IsInterfaceNil(info), true) + + info = returnFilledBasicInterface() + assert.Equal(t, IsInterfaceNil(info), false) +} diff --git a/routers/web/base.go b/routers/web/base.go index 16d3192da21dc..b26104bc9a73e 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -20,6 +20,7 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/templates" + giteautil "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web/middleware" "code.gitea.io/gitea/services/auth" @@ -130,7 +131,7 @@ func Recovery() func(next http.Handler) http.Handler { log.Error("%v", combinedErr) sessionStore := session.GetSession(req) - if sessionStore == nil { + if giteautil.IsInterfaceNil(sessionStore) { if setting.IsProd { http.Error(w, http.StatusText(500), 500) } else { From e1cecf809742125bb322e2b1b85c879949e5cbc6 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 10 Nov 2021 15:27:04 +0100 Subject: [PATCH 2/8] Remove unnecessary code `NewBleveIndexer` will never return nil, even on errors. --- modules/indexer/code/indexer.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/modules/indexer/code/indexer.go b/modules/indexer/code/indexer.go index 0152212aff445..6617971f23a9a 100644 --- a/modules/indexer/code/indexer.go +++ b/modules/indexer/code/indexer.go @@ -186,9 +186,6 @@ func Init() { rIndexer, populate, err = NewBleveIndexer(setting.Indexer.RepoPath) if err != nil { - if !giteautil.IsInterfaceNil(rIndexer) { - rIndexer.Close() - } cancel() indexer.Close() close(waitChannel) From 120988b2055b4ec0d86dd8394107edc79ee74028 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 10 Nov 2021 16:28:24 +0100 Subject: [PATCH 3/8] Patch `NewBleveIndexer` --- modules/indexer/code/bleve.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/indexer/code/bleve.go b/modules/indexer/code/bleve.go index 8e5df34e60be7..66b2602d3bf2e 100644 --- a/modules/indexer/code/bleve.go +++ b/modules/indexer/code/bleve.go @@ -173,6 +173,10 @@ func NewBleveIndexer(indexDir string) (*BleveIndexer, bool, error) { indexDir: indexDir, } created, err := indexer.init() + if err != nil { + indexer.Close() + return nil, false, err + } return indexer, created, err } From 0dac63725ccf686582b2f4074e0b98e047ada352 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 10 Nov 2021 16:55:59 +0100 Subject: [PATCH 4/8] Fix low-level functions --- modules/indexer/code/indexer.go | 4 ---- modules/util/util.go | 12 ------------ modules/util/util_test.go | 10 ---------- routers/web/base.go | 5 ++--- vendor/gitea.com/go-chi/session/session.go | 10 ++++++++-- 5 files changed, 10 insertions(+), 31 deletions(-) diff --git a/modules/indexer/code/indexer.go b/modules/indexer/code/indexer.go index 6617971f23a9a..c56b1b2bb0c5d 100644 --- a/modules/indexer/code/indexer.go +++ b/modules/indexer/code/indexer.go @@ -18,7 +18,6 @@ import ( "code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" - giteautil "code.gitea.io/gitea/modules/util" ) // SearchResult result of performing a search in a repo @@ -203,9 +202,6 @@ func Init() { rIndexer, populate, err = NewElasticSearchIndexer(setting.Indexer.RepoConnStr, setting.Indexer.RepoIndexerName) if err != nil { - if !giteautil.IsInterfaceNil(rIndexer) { - rIndexer.Close() - } cancel() indexer.Close() close(waitChannel) diff --git a/modules/util/util.go b/modules/util/util.go index 68a2e97423279..cbc6eb4f8a01c 100644 --- a/modules/util/util.go +++ b/modules/util/util.go @@ -9,7 +9,6 @@ import ( "crypto/rand" "errors" "math/big" - "reflect" "strconv" "strings" ) @@ -162,14 +161,3 @@ func RandomString(length int64) (string, error) { } return string(bytes), nil } - -// IsInterfaceNil checks if a variable with a typed interface is nil. -// Because interface{} == nil is always false. -func IsInterfaceNil(i interface{}) bool { - switch reflect.TypeOf(i).Kind() { - case reflect.Ptr, reflect.Map, reflect.Array, reflect.Chan, reflect.Slice: - return reflect.ValueOf(i).IsNil() - default: - return false - } -} diff --git a/modules/util/util_test.go b/modules/util/util_test.go index e0d8439c876e8..ddad8221afc1c 100644 --- a/modules/util/util_test.go +++ b/modules/util/util_test.go @@ -186,13 +186,3 @@ func returnFilledBasicInterface() BasicInterface { func returnNil() *BasicInterface { return nil } - -func TestTypedInterfaceNil(t *testing.T) { - var info SampleInterface - - info = returnNil() - assert.Equal(t, IsInterfaceNil(info), true) - - info = returnFilledBasicInterface() - assert.Equal(t, IsInterfaceNil(info), false) -} diff --git a/routers/web/base.go b/routers/web/base.go index b26104bc9a73e..ba82b2fbae524 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -20,7 +20,6 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/templates" - giteautil "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web/middleware" "code.gitea.io/gitea/services/auth" @@ -130,8 +129,8 @@ func Recovery() func(next http.Handler) http.Handler { combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, string(log.Stack(2))) log.Error("%v", combinedErr) - sessionStore := session.GetSession(req) - if giteautil.IsInterfaceNil(sessionStore) { + sessionStore, ok := session.GetSessionWithCheck(req) + if !ok { if setting.IsProd { http.Error(w, http.StatusText(500), 500) } else { diff --git a/vendor/gitea.com/go-chi/session/session.go b/vendor/gitea.com/go-chi/session/session.go index 5c2afedc934b5..546e0a034c0b5 100644 --- a/vendor/gitea.com/go-chi/session/session.go +++ b/vendor/gitea.com/go-chi/session/session.go @@ -267,10 +267,16 @@ func Sessioner(options ...Options) func(next http.Handler) http.Handler { } } +// GetSessionWithCheck returns session store and a bool if Session is nil +func GetSessionWithCheck(req *http.Request) (Store, bool) { + sessCtx := req.Context().Value("Session") + sess, ok := sessCtx.(*store) + return sess, ok +} + // GetSession returns session store func GetSession(req *http.Request) Store { - sessCtx := req.Context().Value("Session") - sess, _ := sessCtx.(*store) + sess, _ := GetSessionWithCheck(req) return sess } From 3297fceb05cb2ff2db29e304f3cc30f698af1035 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 10 Nov 2021 22:57:32 +0100 Subject: [PATCH 5/8] Remove deadcode --- modules/util/util_test.go | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/modules/util/util_test.go b/modules/util/util_test.go index ddad8221afc1c..39cf07c85543e 100644 --- a/modules/util/util_test.go +++ b/modules/util/util_test.go @@ -169,20 +169,3 @@ func Test_OptionalBool(t *testing.T) { assert.Equal(t, OptionalBoolTrue, OptionalBoolParse("t")) assert.Equal(t, OptionalBoolTrue, OptionalBoolParse("True")) } - -type SampleInterface interface { - Hello() -} - -type BasicInterface int - -func (BasicInterface) Hello() { -} - -func returnFilledBasicInterface() BasicInterface { - return BasicInterface(123123) -} - -func returnNil() *BasicInterface { - return nil -} From 05317d61f9c13db6ea42e3b53f269dddb6a6eb67 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 11 Nov 2021 16:49:59 +0100 Subject: [PATCH 6/8] Fix GetSession --- routers/web/base.go | 10 +--------- vendor/gitea.com/go-chi/session/session.go | 10 ++-------- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/routers/web/base.go b/routers/web/base.go index ba82b2fbae524..98713bc8818f6 100644 --- a/routers/web/base.go +++ b/routers/web/base.go @@ -129,15 +129,7 @@ func Recovery() func(next http.Handler) http.Handler { combinedErr := fmt.Sprintf("PANIC: %v\n%s", err, string(log.Stack(2))) log.Error("%v", combinedErr) - sessionStore, ok := session.GetSessionWithCheck(req) - if !ok { - if setting.IsProd { - http.Error(w, http.StatusText(500), 500) - } else { - http.Error(w, combinedErr, 500) - } - return - } + sessionStore := session.GetSession(req) var lc = middleware.Locale(w, req) var store = dataStore{ diff --git a/vendor/gitea.com/go-chi/session/session.go b/vendor/gitea.com/go-chi/session/session.go index 546e0a034c0b5..5c2afedc934b5 100644 --- a/vendor/gitea.com/go-chi/session/session.go +++ b/vendor/gitea.com/go-chi/session/session.go @@ -267,16 +267,10 @@ func Sessioner(options ...Options) func(next http.Handler) http.Handler { } } -// GetSessionWithCheck returns session store and a bool if Session is nil -func GetSessionWithCheck(req *http.Request) (Store, bool) { - sessCtx := req.Context().Value("Session") - sess, ok := sessCtx.(*store) - return sess, ok -} - // GetSession returns session store func GetSession(req *http.Request) Store { - sess, _ := GetSessionWithCheck(req) + sessCtx := req.Context().Value("Session") + sess, _ := sessCtx.(*store) return sess } From 1fb1ccd317ed37ec5220bd7ec8de6cb20a2beb07 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 15 Nov 2021 12:59:39 +0100 Subject: [PATCH 7/8] Close Elastic search when err isn't nil --- modules/indexer/code/elastic_search.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/indexer/code/elastic_search.go b/modules/indexer/code/elastic_search.go index 49633c31916df..24c9e98a91a9b 100644 --- a/modules/indexer/code/elastic_search.go +++ b/modules/indexer/code/elastic_search.go @@ -82,6 +82,9 @@ func NewElasticSearchIndexer(url, indexerName string) (*ElasticSearchIndexer, bo indexerAliasName: indexerName, } exists, err := indexer.init() + if err != nil { + indexer.Close() + } return indexer, !exists, err } From 9d4c95377cb7390aaed6f990f9ffe708ab3430a7 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 15 Nov 2021 20:04:10 +0800 Subject: [PATCH 8/8] Update elastic_search.go --- modules/indexer/code/elastic_search.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/indexer/code/elastic_search.go b/modules/indexer/code/elastic_search.go index 24c9e98a91a9b..f76f316f6494f 100644 --- a/modules/indexer/code/elastic_search.go +++ b/modules/indexer/code/elastic_search.go @@ -84,8 +84,8 @@ func NewElasticSearchIndexer(url, indexerName string) (*ElasticSearchIndexer, bo exists, err := indexer.init() if err != nil { indexer.Close() + return nil, false, err } - return indexer, !exists, err }