From 4c3a671b48c3aa3e9b8cf3e33d1890ce9cae6b6d Mon Sep 17 00:00:00 2001 From: Snider Date: Fri, 20 Mar 2026 17:25:12 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20Codex=20review=20=E2=80=94=20medium/low?= =?UTF-8?q?=20severity=20issues=20resolved?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Medium: - Config zero-value safe (nil ConfigOptions guards on all mutators) - ServiceShutdown collects and returns first OnStop error - Default logger uses atomic.Pointer (race fix) - Command registration rejects duplicates (like Service) Low: - Array.AsSlice returns copy, not backing slice - fs.validatePath constructs error on sandbox escape (was nil) Co-Authored-By: Virgil --- pkg/core/array.go | 9 +++++++-- pkg/core/command.go | 4 ++++ pkg/core/config.go | 20 +++++++++++++++++--- pkg/core/contract.go | 2 +- pkg/core/error.go | 2 +- pkg/core/fs.go | 3 +++ pkg/core/log.go | 26 ++++++++++++++++---------- pkg/core/runtime.go | 11 ++++++++++- 8 files changed, 59 insertions(+), 18 deletions(-) diff --git a/pkg/core/array.go b/pkg/core/array.go index ba2de77..ff085bb 100644 --- a/pkg/core/array.go +++ b/pkg/core/array.go @@ -90,7 +90,12 @@ func (s *Array[T]) Clear() { s.items = nil } -// AsSlice returns the underlying slice. +// AsSlice returns a copy of the underlying slice. func (s *Array[T]) AsSlice() []T { - return s.items + if s.items == nil { + return nil + } + out := make([]T, len(s.items)) + copy(out, s.items) + return out } diff --git a/pkg/core/command.go b/pkg/core/command.go index 8619128..8253b15 100644 --- a/pkg/core/command.go +++ b/pkg/core/command.go @@ -144,6 +144,10 @@ func (c *Core) Command(path string, command ...Command) Result { c.commands.mu.Lock() defer c.commands.mu.Unlock() + if _, exists := c.commands.commands[path]; exists { + return Result{E("core.Command", Concat("command \"", path, "\" already registered"), nil), false} + } + cmd := &command[0] cmd.Name = pathName(path) cmd.Path = path diff --git a/pkg/core/config.go b/pkg/core/config.go index ffb19b4..41415b7 100644 --- a/pkg/core/config.go +++ b/pkg/core/config.go @@ -51,6 +51,9 @@ type Config struct { // Set stores a configuration value by key. func (e *Config) Set(key string, val any) { e.mu.Lock() + if e.ConfigOptions == nil { + e.ConfigOptions = &ConfigOptions{} + } e.ConfigOptions.init() e.Settings[key] = val e.mu.Unlock() @@ -89,6 +92,9 @@ func ConfigGet[T any](e *Config, key string) T { func (e *Config) Enable(feature string) { e.mu.Lock() + if e.ConfigOptions == nil { + e.ConfigOptions = &ConfigOptions{} + } e.ConfigOptions.init() e.Features[feature] = true e.mu.Unlock() @@ -96,6 +102,9 @@ func (e *Config) Enable(feature string) { func (e *Config) Disable(feature string) { e.mu.Lock() + if e.ConfigOptions == nil { + e.ConfigOptions = &ConfigOptions{} + } e.ConfigOptions.init() e.Features[feature] = false e.mu.Unlock() @@ -103,14 +112,19 @@ func (e *Config) Disable(feature string) { func (e *Config) Enabled(feature string) bool { e.mu.RLock() - v := e.Features[feature] - e.mu.RUnlock() - return v + defer e.mu.RUnlock() + if e.ConfigOptions == nil || e.Features == nil { + return false + } + return e.Features[feature] } func (e *Config) EnabledFeatures() []string { e.mu.RLock() defer e.mu.RUnlock() + if e.ConfigOptions == nil || e.Features == nil { + return nil + } var result []string for k, v := range e.Features { if v { diff --git a/pkg/core/contract.go b/pkg/core/contract.go index a033b58..dad7f02 100644 --- a/pkg/core/contract.go +++ b/pkg/core/contract.go @@ -79,7 +79,7 @@ func New(opts ...Options) *Core { fs: &Fs{root: "/"}, config: &Config{ConfigOptions: &ConfigOptions{}}, error: &ErrorPanic{}, - log: &ErrorLog{log: defaultLog}, + log: &ErrorLog{log: Default()}, lock: &Lock{}, ipc: &Ipc{}, i18n: &I18n{}, diff --git a/pkg/core/error.go b/pkg/core/error.go index cfabe06..ed6853d 100644 --- a/pkg/core/error.go +++ b/pkg/core/error.go @@ -244,7 +244,7 @@ func (el *ErrorLog) logger() *Log { if el.log != nil { return el.log } - return defaultLog + return Default() } // Error logs at Error level and returns a Result with the wrapped error. diff --git a/pkg/core/fs.go b/pkg/core/fs.go index 210deb0..e347ae9 100644 --- a/pkg/core/fs.go +++ b/pkg/core/fs.go @@ -79,6 +79,9 @@ func (m *Fs) validatePath(p string) Result { } Print(os.Stderr, "[%s] SECURITY sandbox escape detected root=%s path=%s attempted=%s user=%s", time.Now().Format(time.RFC3339), m.root, p, realNext, username) + if err == nil { + err = E("fs.validatePath", Concat("sandbox escape: ", p, " resolves outside ", m.root), nil) + } return Result{err, false} } current = realNext diff --git a/pkg/core/log.go b/pkg/core/log.go index e4c6a72..65f8c5f 100644 --- a/pkg/core/log.go +++ b/pkg/core/log.go @@ -11,6 +11,7 @@ import ( "os/user" "slices" "sync" + "sync/atomic" "time" ) @@ -296,51 +297,56 @@ func Username() string { // --- Default logger --- -var defaultLog = NewLog(LogOptions{Level: LevelInfo}) +var defaultLogPtr atomic.Pointer[Log] + +func init() { + l := NewLog(LogOptions{Level: LevelInfo}) + defaultLogPtr.Store(l) +} // Default returns the default logger. func Default() *Log { - return defaultLog + return defaultLogPtr.Load() } // SetDefault sets the default logger. func SetDefault(l *Log) { - defaultLog = l + defaultLogPtr.Store(l) } // SetLevel sets the default logger's level. func SetLevel(level Level) { - defaultLog.SetLevel(level) + Default().SetLevel(level) } // SetRedactKeys sets the default logger's redaction keys. func SetRedactKeys(keys ...string) { - defaultLog.SetRedactKeys(keys...) + Default().SetRedactKeys(keys...) } // Debug logs to the default logger. func Debug(msg string, keyvals ...any) { - defaultLog.Debug(msg, keyvals...) + Default().Debug(msg, keyvals...) } // Info logs to the default logger. func Info(msg string, keyvals ...any) { - defaultLog.Info(msg, keyvals...) + Default().Info(msg, keyvals...) } // Warn logs to the default logger. func Warn(msg string, keyvals ...any) { - defaultLog.Warn(msg, keyvals...) + Default().Warn(msg, keyvals...) } // Error logs to the default logger. func Error(msg string, keyvals ...any) { - defaultLog.Error(msg, keyvals...) + Default().Error(msg, keyvals...) } // Security logs to the default logger. func Security(msg string, keyvals ...any) { - defaultLog.Security(msg, keyvals...) + Default().Security(msg, keyvals...) } // --- LogErr: Error-Aware Logger --- diff --git a/pkg/core/runtime.go b/pkg/core/runtime.go index 76ae3d9..9e47ead 100644 --- a/pkg/core/runtime.go +++ b/pkg/core/runtime.go @@ -53,13 +53,19 @@ func (c *Core) ServiceStartup(ctx context.Context, options any) Result { func (c *Core) ServiceShutdown(ctx context.Context) Result { c.shutdown.Store(true) c.ACTION(ActionServiceShutdown{}) + var firstErr error stoppables := c.Stoppables() if stoppables.OK { for _, s := range stoppables.Value.([]*Service) { if err := ctx.Err(); err != nil { return Result{err, false} } - s.OnStop() + r := s.OnStop() + if !r.OK && firstErr == nil { + if e, ok := r.Value.(error); ok { + firstErr = e + } + } } } done := make(chan struct{}) @@ -72,6 +78,9 @@ func (c *Core) ServiceShutdown(ctx context.Context) Result { case <-ctx.Done(): return Result{ctx.Err(), false} } + if firstErr != nil { + return Result{firstErr, false} + } return Result{OK: true} }