Skip to content

Commit ffc5f65

Browse files
committed
fix: address linter findings and harden runtime correctness
- processor: drop leaked cancel in WithContext (gosec G118); route cancellation through stopCh/parentDone and close readers in Stop() - executor: convert isStarted/isFinished to atomic.Bool; treat exec.ErrWaitDelay and context.Canceled-with-ProcessState as non-failures - executor: validate raw command path before filepath.Clean to catch /../etc/passwd-style traversal that Clean silently normalizes away - main: install signal handlers before exec.Start() to avoid orphaning the child; bound shutdown with killTimeout; derive signal exit codes from signalExitCodeBase+sig and surface non-exit errors as 1 - filter: pass lines with no detected level so plain output is no longer silently dropped by include/exclude level rules - formatter: getColorCode returns (string, error) and rejects unknown color names; templateReferencesLine recognizes {{- .Line trim syntax - config: skip filter validation when disabled; dedupe include/exclude pattern checks via validateFilterPatterns - tests: add goleak TestMain to every package; pin Go via go-version-file in linter workflow; add go.uber.org/goleak v1.3.0 Closes #87
1 parent d9ccfb9 commit ffc5f65

22 files changed

Lines changed: 358 additions & 136 deletions

.github/workflows/linter.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ jobs:
1313
- uses: actions/checkout@v6
1414
- uses: actions/setup-go@v6
1515
with:
16-
go-version: stable
16+
go-version-file: 'go.mod'
1717
- name: Install task
1818
uses: jaxxstorm/action-install-gh-release@v2.1.0
1919
with:

cmd/logwrap/integration_test.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"github.com/sgaunet/logwrap/pkg/processor"
2121
"github.com/stretchr/testify/assert"
2222
"github.com/stretchr/testify/require"
23+
"go.uber.org/goleak"
2324
)
2425

2526
// testBinaryPath holds the path to the compiled logwrap binary for subprocess tests.
@@ -49,9 +50,9 @@ func TestMain(m *testing.M) {
4950
os.Exit(1)
5051
}
5152

52-
exitCode := m.Run()
53-
os.RemoveAll(tmpDir)
54-
os.Exit(exitCode)
53+
goleak.VerifyTestMain(m, goleak.Cleanup(func(exitCode int) {
54+
os.RemoveAll(tmpDir)
55+
}))
5556
}
5657

5758
// runPipeline constructs the full logwrap pipeline with a thread-safe captured

cmd/logwrap/main.go

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,12 @@ var (
2626
)
2727

2828
const (
29-
exitCodeSIGINT = 130 // 128 + 2 (SIGINT)
30-
exitCodeSIGTERM = 143 // 128 + 15 (SIGTERM)
29+
signalExitCodeBase = 128 // UNIX convention: 128 + signal number
30+
exitCodeSIGINT = signalExitCodeBase + 2 // SIGINT
31+
exitCodeSIGTERM = signalExitCodeBase + 15 // SIGTERM
3132
gracefulShutdownTimeout = 5 * time.Second
3233
processorWaitTimeout = 3 * time.Second
34+
killTimeout = 2 * time.Second
3335
usage = `LogWrap - Command execution wrapper with configurable log prefixes
3436
3537
Usage:
@@ -292,6 +294,16 @@ func run(cfg *config.Config, command []string) int {
292294
procOpts = append(procOpts, processor.WithFilter(f))
293295
}
294296

297+
// Set up signal handling before starting the child process to avoid
298+
// a race where a signal arrives after Start() but before Notify(),
299+
// which would use Go's default handler (os.Exit) and orphan the child.
300+
sigChan := make(chan os.Signal, 1)
301+
signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)
302+
303+
ctx, ctxCancel := context.WithCancel(context.Background())
304+
defer ctxCancel()
305+
306+
procOpts = append(procOpts, processor.WithContext(ctx))
295307
proc := processor.New(form, os.Stdout, procOpts...)
296308

297309
if err := exec.Start(); err != nil {
@@ -301,12 +313,7 @@ func run(cfg *config.Config, command []string) int {
301313

302314
stdout, stderr := exec.GetStreams()
303315

304-
// Set up signal handling
305-
sigChan := make(chan os.Signal, 1)
306-
signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)
307-
308316
// Start stream processing in background
309-
ctx := context.Background()
310317
processingDone := make(chan error, 1)
311318
go func() {
312319
processingDone <- proc.ProcessStreams(ctx, stdout, stderr)
@@ -372,7 +379,16 @@ func handleSignalShutdown(exec *executor.Executor, proc *processor.Processor, si
372379
fmt.Fprintf(os.Stderr, "Warning: failed to kill process: %v\n", err)
373380
}
374381
proc.Stop()
375-
return <-cmdDone // Wait for process to actually die
382+
// Wait for process to die, with a hard timeout to avoid hanging
383+
// indefinitely if the process is in an unkillable state (e.g., D state on Linux).
384+
killTimer := time.NewTimer(killTimeout)
385+
defer killTimer.Stop()
386+
select {
387+
case cmdErr := <-cmdDone:
388+
return cmdErr
389+
case <-killTimer.C:
390+
return nil
391+
}
376392
}
377393
}
378394

@@ -391,17 +407,28 @@ func waitForProcessing(proc *processor.Processor, processingDone chan error) {
391407
}
392408
}
393409

394-
func determineExitCode(exec *executor.Executor, receivedSignal os.Signal, _ error) int {
410+
func determineExitCode(exec *executor.Executor, receivedSignal os.Signal, cmdErr error) int {
395411
// If we received a signal, use signal-based exit code
396412
if receivedSignal != nil {
397413
switch receivedSignal {
398414
case syscall.SIGINT:
399415
return exitCodeSIGINT
400416
case syscall.SIGTERM:
401417
return exitCodeSIGTERM
418+
default:
419+
if sig, ok := receivedSignal.(syscall.Signal); ok {
420+
return signalExitCodeBase + int(sig)
421+
}
422+
423+
return 1
402424
}
403425
}
404426

405-
// Otherwise use command's exit code
427+
// If the command failed with a non-exit error (e.g., I/O error, context error),
428+
// the executor's exit code stays at 0. Use 1 to avoid masking the failure.
429+
if cmdErr != nil && exec.GetExitCode() == 0 {
430+
return 1
431+
}
432+
406433
return exec.GetExitCode()
407434
}

go.mod

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,12 @@ go 1.25.0
44

55
require gopkg.in/yaml.v3 v3.0.1
66

7-
require github.com/itchyny/timefmt-go v0.1.8
7+
require (
8+
github.com/itchyny/timefmt-go v0.1.8
9+
go.uber.org/goleak v1.3.0
10+
)
11+
12+
require github.com/kr/text v0.2.0 // indirect
813

914
require (
1015
github.com/davecgh/go-spew v1.1.1 // indirect

go.sum

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,20 @@
1+
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
12
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
23
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
34
github.com/itchyny/timefmt-go v0.1.8 h1:1YEo1JvfXeAHKdjelbYr/uCuhkybaHCeTkH8Bo791OI=
45
github.com/itchyny/timefmt-go v0.1.8/go.mod h1:5E46Q+zj7vbTgWY8o5YkMeYb4I6GeWLFnetPy5oBrAI=
6+
github.com/kr/pretty v0.1.0 h1:L/CwN0zerZDmRFUapSPitk6f+Q3+0za1rQkzVuMiMFI=
7+
github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
8+
github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
9+
github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
510
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
611
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
712
github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U=
813
github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U=
9-
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
14+
go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
15+
go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
1016
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
17+
gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127 h1:qIbj1fsPNlZgppZ+VLlY7N33q108Sa+fhmuc+sWQYwY=
18+
gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
1119
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
1220
gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

pkg/apperrors/testmain_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
package apperrors
2+
3+
import (
4+
"testing"
5+
6+
"go.uber.org/goleak"
7+
)
8+
9+
func TestMain(m *testing.M) {
10+
goleak.VerifyTestMain(m)
11+
}

pkg/config/config.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ import (
6262
"io"
6363
"os"
6464
"path/filepath"
65+
"slices"
6566
"strings"
6667

6768
"github.com/sgaunet/logwrap/pkg/apperrors"
@@ -321,7 +322,7 @@ func parseCLIFlags(args []string) (*CLIFlags, error) {
321322
}
322323

323324
func applyCLIOverrides(config *Config, flags *CLIFlags) {
324-
if flags.Template != nil && *flags.Template != "" {
325+
if flags.setFlags["template"] {
325326
config.Prefix.Template = *flags.Template
326327
}
327328
if flags.setFlags["utc"] {
@@ -330,7 +331,7 @@ func applyCLIOverrides(config *Config, flags *CLIFlags) {
330331
if flags.setFlags["colors"] {
331332
config.Prefix.Colors.Enabled = *flags.ColorsEnabled
332333
}
333-
if flags.OutputFormat != nil && *flags.OutputFormat != "" {
334+
if flags.setFlags["format"] {
334335
config.Output.Format = *flags.OutputFormat
335336
}
336337
}
@@ -369,9 +370,10 @@ func FindConfigFile() string {
369370
// - Path traversal: rejects paths containing ".." after filepath.Clean
370371
// - File type: only .yaml and .yml extensions are accepted (case-insensitive)
371372
func validateConfigPath(configFile string) error {
372-
// Prevent path traversal attacks
373+
// Prevent path traversal attacks by checking for ".." as a path component,
374+
// not a substring — filenames like "backup..yaml" are valid.
373375
cleaned := filepath.Clean(configFile)
374-
if strings.Contains(cleaned, "..") {
376+
if slices.Contains(strings.Split(cleaned, string(filepath.Separator)), "..") {
375377
return apperrors.ErrPathTraversal
376378
}
377379

pkg/config/config_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,7 @@ func TestApplyCLIOverrides(t *testing.T) {
490490
TimestampUTC: &utc,
491491
ColorsEnabled: &colors,
492492
OutputFormat: &format,
493-
setFlags: map[string]bool{"utc": true, "colors": true},
493+
setFlags: map[string]bool{"utc": true, "colors": true, "template": true, "format": true},
494494
}
495495

496496
// Apply overrides

pkg/config/testmain_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
package config
2+
3+
import (
4+
"testing"
5+
6+
"go.uber.org/goleak"
7+
)
8+
9+
func TestMain(m *testing.M) {
10+
goleak.VerifyTestMain(m)
11+
}

pkg/config/validation.go

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,10 @@ func isValidLogLevel(level string, validLevels []string) bool {
374374
// that assigns levels to lines. Without detection, all lines have
375375
// an empty detected level and level filters silently drop everything.
376376
func (c *Config) validateFilter() error {
377+
if !c.Filter.Enabled {
378+
return nil
379+
}
380+
377381
validLevels := []string{"TRACE", "DEBUG", "INFO", "WARN", "ERROR", "FATAL"}
378382

379383
if !c.LogLevel.Detection.Enabled {
@@ -387,19 +391,19 @@ func (c *Config) validateFilter() error {
387391
if err := validateFilterLevelNames(c.Filter.ExcludeLevels, "exclude_levels", validLevels); err != nil {
388392
return err
389393
}
390-
if slices.Contains(c.Filter.ExcludePatterns, "") {
391-
return fmt.Errorf("%w in exclude_patterns", apperrors.ErrEmptyFilterPattern)
392-
}
393-
if slices.Contains(c.Filter.IncludePatterns, "") {
394-
return fmt.Errorf("%w in include_patterns", apperrors.ErrEmptyFilterPattern)
395-
}
396-
if err := validateRegexPatterns(c.Filter.ExcludePatterns, "exclude_patterns"); err != nil {
394+
if err := validateFilterPatterns(c.Filter.ExcludePatterns, "exclude_patterns"); err != nil {
397395
return err
398396
}
399-
if err := validateRegexPatterns(c.Filter.IncludePatterns, "include_patterns"); err != nil {
400-
return err
397+
return validateFilterPatterns(c.Filter.IncludePatterns, "include_patterns")
398+
}
399+
400+
// validateFilterPatterns checks that a pattern list contains no empty strings
401+
// and that all entries are valid regular expressions.
402+
func validateFilterPatterns(patterns []string, field string) error {
403+
if slices.Contains(patterns, "") {
404+
return fmt.Errorf("%w in %s", apperrors.ErrEmptyFilterPattern, field)
401405
}
402-
return nil
406+
return validateRegexPatterns(patterns, field)
403407
}
404408

405409
// validateFilterLevelNames checks that all level names in the list are valid

0 commit comments

Comments
 (0)