From 9d701c704416a1d8648dd2818a8a15c4da99b424 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 21 Mar 2018 14:27:14 -0400 Subject: [PATCH] Fix various segfaults when running `go test` manually (#8448) * failing to find i18n shouldn't segfault The server was trying to handle the fact that it couldn't find the i18n directory, by emitting a translated log message... * fix utils.FindDir The attempts to find the directory in the parent or grandparent directory don't work if the current working directory was inside `enterprise`, with `enterprise` itself being a symlink as per the usual developer setup. Recurse to the root of the filesystem, cleaning the path along the way to work around this limitation (and allow tests to be run from an arbitrarily deep nesting level.) Fix corresponding usages to employ filepath.Join. * failing to find html templates shouldn't segfault * fail fast if the test user cannot be created * rework utils.FindDir to retain backwards compatibility --- api/file_test.go | 3 ++- api/user_test.go | 3 ++- api4/apitestlib.go | 9 +++++++-- api4/oauth.go | 3 ++- api4/plugin_test.go | 3 ++- app/app.go | 6 +++++- app/auto_posts.go | 3 ++- app/import_test.go | 7 ++++--- app/saml.go | 3 +-- app/user.go | 3 ++- utils/config.go | 30 ++++++++++++------------------ utils/html.go | 10 ++++++++-- utils/i18n.go | 12 +++++++----- utils/license.go | 3 ++- web/web.go | 3 ++- 15 files changed, 60 insertions(+), 41 deletions(-) diff --git a/api/file_test.go b/api/file_test.go index 8e5fc6f679..7a04674cd6 100644 --- a/api/file_test.go +++ b/api/file_test.go @@ -10,6 +10,7 @@ import ( "io/ioutil" "net/http" "os" + "path/filepath" "strings" "testing" "time" @@ -870,7 +871,7 @@ func TestGetInfoForFilename(t *testing.T) { func readTestFile(name string) ([]byte, error) { path, _ := utils.FindDir("tests") - file, err := os.Open(path + "/" + name) + file, err := os.Open(filepath.Join(path, name)) if err != nil { return nil, err } diff --git a/api/user_test.go b/api/user_test.go index f65d7c45b7..5183793055 100644 --- a/api/user_test.go +++ b/api/user_test.go @@ -12,6 +12,7 @@ import ( "mime/multipart" "net/http" "os" + "path/filepath" "strings" "testing" "time" @@ -778,7 +779,7 @@ func TestUserUploadProfileImage(t *testing.T) { } path, _ := utils.FindDir("tests") - file, err := os.Open(path + "/test.png") + file, err := os.Open(filepath.Join(path, "test.png")) if err != nil { t.Fatal(err) } diff --git a/api4/apitestlib.go b/api4/apitestlib.go index 6edd378128..386afdadd5 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -11,6 +11,7 @@ import ( "net" "net/http" "os" + "path/filepath" "reflect" "strconv" "strings" @@ -302,7 +303,11 @@ func (me *TestHelper) CreateUserWithClient(client *model.Client4) *model.User { } utils.DisableDebugLogForTest() - ruser, _ := client.CreateUser(user) + ruser, response := client.CreateUser(user) + if response.Error != nil { + panic(response.Error) + } + ruser.Password = "Password1" store.Must(me.App.Srv.Store.User().VerifyEmail(ruser.Id)) utils.EnableDebugLogForTest() @@ -675,7 +680,7 @@ func CheckInternalErrorStatus(t *testing.T, resp *model.Response) { func readTestFile(name string) ([]byte, error) { path, _ := utils.FindDir("tests") - file, err := os.Open(path + "/" + name) + file, err := os.Open(filepath.Join(path, name)) if err != nil { return nil, err } diff --git a/api4/oauth.go b/api4/oauth.go index d0f43256a8..a173159b61 100644 --- a/api4/oauth.go +++ b/api4/oauth.go @@ -6,6 +6,7 @@ package api4 import ( "net/http" "net/url" + "path/filepath" "strings" l4g "github.com/alecthomas/log4go" @@ -375,7 +376,7 @@ func authorizeOAuthPage(c *Context, w http.ResponseWriter, r *http.Request) { w.Header().Set("Cache-Control", "no-cache, max-age=31556926, public") staticDir, _ := utils.FindDir(model.CLIENT_DIR) - http.ServeFile(w, r, staticDir+"root.html") + http.ServeFile(w, r, filepath.Join(staticDir, "root.html")) } func getAccessToken(c *Context, w http.ResponseWriter, r *http.Request) { diff --git a/api4/plugin_test.go b/api4/plugin_test.go index e385b5c8cc..045ae9212e 100644 --- a/api4/plugin_test.go +++ b/api4/plugin_test.go @@ -8,6 +8,7 @@ import ( "encoding/json" "io/ioutil" "os" + "path/filepath" "testing" "github.com/mattermost/mattermost-server/model" @@ -53,7 +54,7 @@ func TestPlugin(t *testing.T) { }() path, _ := utils.FindDir("tests") - file, err := os.Open(path + "/testplugin.tar.gz") + file, err := os.Open(filepath.Join(path, "testplugin.tar.gz")) if err != nil { t.Fatal(err) } diff --git a/app/app.go b/app/app.go index f5e5dd21e2..48e95fb998 100644 --- a/app/app.go +++ b/app/app.go @@ -439,7 +439,11 @@ func (a *App) WaitForGoroutines() { } func (a *App) HTMLTemplates() *template.Template { - return a.htmlTemplateWatcher.Templates() + if a.htmlTemplateWatcher != nil { + return a.htmlTemplateWatcher.Templates() + } + + return nil } func (a *App) HTTPClient(trustURLs bool) *http.Client { diff --git a/app/auto_posts.go b/app/auto_posts.go index 379c74ab72..a2adb9d6c9 100644 --- a/app/auto_posts.go +++ b/app/auto_posts.go @@ -7,6 +7,7 @@ import ( "bytes" "io" "os" + "path/filepath" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/utils" @@ -43,7 +44,7 @@ func (cfg *AutoPostCreator) UploadTestFile() ([]string, bool) { filename := cfg.ImageFilenames[utils.RandIntFromRange(utils.Range{Begin: 0, End: len(cfg.ImageFilenames) - 1})] path, _ := utils.FindDir("web/static/images") - file, err := os.Open(path + "/" + filename) + file, err := os.Open(filepath.Join(path, filename)) if err != nil { return nil, false } diff --git a/app/import_test.go b/app/import_test.go index 6a284f63d7..f294c87315 100644 --- a/app/import_test.go +++ b/app/import_test.go @@ -4,6 +4,7 @@ package app import ( + "path/filepath" "runtime/debug" "strings" "testing" @@ -370,7 +371,7 @@ func TestImportValidateUserImportData(t *testing.T) { // Test a valid User with all fields populated. testsDir, _ := utils.FindDir("tests") data = UserImportData{ - ProfileImage: ptrStr(testsDir + "test.png"), + ProfileImage: ptrStr(filepath.Join(testsDir, "test.png")), Username: ptrStr("bob"), Email: ptrStr("bob@example.com"), AuthService: ptrStr("ldap"), @@ -1487,7 +1488,7 @@ func TestImportImportUser(t *testing.T) { username := model.NewId() testsDir, _ := utils.FindDir("tests") data = UserImportData{ - ProfileImage: ptrStr(testsDir + "test.png"), + ProfileImage: ptrStr(filepath.Join(testsDir, "test.png")), Username: &username, Email: ptrStr(model.NewId() + "@example.com"), Nickname: ptrStr(model.NewId()), @@ -1543,7 +1544,7 @@ func TestImportImportUser(t *testing.T) { // Alter all the fields of that user. data.Email = ptrStr(model.NewId() + "@example.com") - data.ProfileImage = ptrStr(testsDir + "testgif.gif") + data.ProfileImage = ptrStr(filepath.Join(testsDir, "testgif.gif")) data.AuthService = ptrStr("ldap") data.AuthData = &username data.Nickname = ptrStr(model.NewId()) diff --git a/app/saml.go b/app/saml.go index bd9f647a14..29d5f87726 100644 --- a/app/saml.go +++ b/app/saml.go @@ -8,7 +8,6 @@ import ( "mime/multipart" "net/http" "os" - "path/filepath" "github.com/mattermost/mattermost-server/model" @@ -42,7 +41,7 @@ func WriteSamlFile(fileData *multipart.FileHeader) *model.AppError { defer file.Close() configDir, _ := utils.FindDir("config") - out, err := os.Create(configDir + filename) + out, err := os.Create(filepath.Join(configDir, filename)) if err != nil { return model.NewAppError("AddSamlCertificate", "api.admin.add_certificate.saving.app_error", nil, err.Error(), http.StatusInternalServerError) } diff --git a/app/user.go b/app/user.go index 7a6dc0b497..8f393038a9 100644 --- a/app/user.go +++ b/app/user.go @@ -18,6 +18,7 @@ import ( "io/ioutil" "mime/multipart" "net/http" + "path/filepath" "strconv" "strings" @@ -717,7 +718,7 @@ func CreateProfileImage(username string, userId string, initialFont string) ([]b initial := string(strings.ToUpper(username)[0]) fontDir, _ := utils.FindDir("fonts") - fontBytes, err := ioutil.ReadFile(fontDir + initialFont) + fontBytes, err := ioutil.ReadFile(filepath.Join(fontDir, initialFont)) if err != nil { return nil, model.NewAppError("CreateProfileImage", "api.user.create_profile_image.default_font.app_error", nil, err.Error(), http.StatusInternalServerError) } diff --git a/utils/config.go b/utils/config.go index 93a8707434..5e98499304 100644 --- a/utils/config.go +++ b/utils/config.go @@ -9,7 +9,6 @@ import ( "io" "io/ioutil" "os" - "path" "path/filepath" "strconv" "strings" @@ -51,21 +50,17 @@ func FindConfigFile(fileName string) (path string) { return "" } +// FindDir looks for the given directory in nearby ancestors, falling back to `./` if not found. func FindDir(dir string) (string, bool) { - fileName := "." - found := false - if _, err := os.Stat("./" + dir + "/"); err == nil { - fileName, _ = filepath.Abs("./" + dir + "/") - found = true - } else if _, err := os.Stat("../" + dir + "/"); err == nil { - fileName, _ = filepath.Abs("../" + dir + "/") - found = true - } else if _, err := os.Stat("../../" + dir + "/"); err == nil { - fileName, _ = filepath.Abs("../../" + dir + "/") - found = true + for _, parent := range []string{".", "..", "../.."} { + foundDir, err := filepath.Abs(filepath.Join(parent, dir)) + if err != nil { + continue + } else if _, err := os.Stat(foundDir); err == nil { + return foundDir, true + } } - - return fileName + "/", found + return "./", false } func DisableDebugLogForTest() { @@ -136,11 +131,10 @@ func ConfigureLog(s *model.LogSettings) { func GetLogFileLocation(fileLocation string) string { if fileLocation == "" { - logDir, _ := FindDir("logs") - return logDir + LOG_FILENAME - } else { - return path.Join(fileLocation, LOG_FILENAME) + fileLocation, _ = FindDir("logs") } + + return filepath.Join(fileLocation, LOG_FILENAME) } func SaveConfig(fileName string, config *model.Config) *model.AppError { diff --git a/utils/html.go b/utils/html.go index 6bbe55c6db..f9a7abe5b5 100644 --- a/utils/html.go +++ b/utils/html.go @@ -5,8 +5,10 @@ package utils import ( "bytes" + "errors" "html/template" "io" + "path/filepath" "reflect" "sync/atomic" @@ -39,7 +41,7 @@ func NewHTMLTemplateWatcher(directory string) (*HTMLTemplateWatcher, error) { return nil, err } - if htmlTemplates, err := template.ParseGlob(templatesDir + "*.html"); err != nil { + if htmlTemplates, err := template.ParseGlob(filepath.Join(templatesDir, "*.html")); err != nil { return nil, err } else { ret.templates.Store(htmlTemplates) @@ -56,7 +58,7 @@ func NewHTMLTemplateWatcher(directory string) (*HTMLTemplateWatcher, error) { case event := <-watcher.Events: if event.Op&fsnotify.Write == fsnotify.Write { l4g.Info("Re-parsing templates because of modified file %v", event.Name) - if htmlTemplates, err := template.ParseGlob(templatesDir + "*.html"); err != nil { + if htmlTemplates, err := template.ParseGlob(filepath.Join(templatesDir, "*.html")); err != nil { l4g.Error("Failed to parse templates %v", err) } else { ret.templates.Store(htmlTemplates) @@ -103,6 +105,10 @@ func (t *HTMLTemplate) Render() string { } func (t *HTMLTemplate) RenderToWriter(w io.Writer) error { + if t.Templates == nil { + return errors.New("no html templates") + } + if err := t.Templates.ExecuteTemplate(w, t.TemplateName, t); err != nil { l4g.Error(T("api.api.render.error"), t.TemplateName, err) return err diff --git a/utils/i18n.go b/utils/i18n.go index 8ed82d19ff..7b8d1fef0e 100644 --- a/utils/i18n.go +++ b/utils/i18n.go @@ -23,13 +23,15 @@ var settings model.LocalizationSettings // this functions loads translations from filesystem // and assign english while loading server config func TranslationsPreInit() error { + // Set T even if we fail to load the translations. Lots of shutdown handling code will + // segfault trying to handle the error, and the untranslated IDs are strictly better. + T = TfuncWithFallback("en") + TDefault = TfuncWithFallback("en") + if err := InitTranslationsWithDir("i18n"); err != nil { return err } - T = TfuncWithFallback("en") - TDefault = TfuncWithFallback("en") - return nil } @@ -51,9 +53,9 @@ func InitTranslationsWithDir(dir string) error { for _, f := range files { if filepath.Ext(f.Name()) == ".json" { filename := f.Name() - locales[strings.Split(filename, ".")[0]] = i18nDirectory + filename + locales[strings.Split(filename, ".")[0]] = filepath.Join(i18nDirectory, filename) - if err := i18n.LoadTranslationFile(i18nDirectory + filename); err != nil { + if err := i18n.LoadTranslationFile(filepath.Join(i18nDirectory, filename)); err != nil { return err } } diff --git a/utils/license.go b/utils/license.go index 2853a58d00..cf874b62bf 100644 --- a/utils/license.go +++ b/utils/license.go @@ -12,6 +12,7 @@ import ( "encoding/pem" "io/ioutil" "os" + "path/filepath" "strconv" "strings" @@ -114,7 +115,7 @@ func GetLicenseFileFromDisk(fileName string) []byte { func GetLicenseFileLocation(fileLocation string) string { if fileLocation == "" { configDir, _ := FindDir("config") - return configDir + "mattermost.mattermost-license" + return filepath.Join(configDir, "mattermost.mattermost-license") } else { return fileLocation } diff --git a/web/web.go b/web/web.go index 22fe439233..c9d3183979 100644 --- a/web/web.go +++ b/web/web.go @@ -5,6 +5,7 @@ package web import ( "net/http" + "path/filepath" "strings" "github.com/NYTimes/gziphandler" @@ -102,5 +103,5 @@ func root(c *api.Context, w http.ResponseWriter, r *http.Request) { w.Header().Set("Cache-Control", "no-cache, max-age=31556926, public") staticDir, _ := utils.FindDir(model.CLIENT_DIR) - http.ServeFile(w, r, staticDir+"root.html") + http.ServeFile(w, r, filepath.Join(staticDir, "root.html")) }