fix: build tag parsing.

[matthewhartstonge]

Aims to fix #1412

Couple of thoughts:

• I removed the mutex as I build the required config on load due to requiring transforming. This could be optimised by making load() take in both buildTags and a *package.Config so it's only processed and in memory once.
• The previous build tag test never compiled, which leads to a panic, which is then hidden due to the graceful recover code. Wondering if it's best to remove the test? I changed the test sample name from SampleCodeBuildTag => SampleCodeCompilationFail and added should not panic if a file can not compile as a test case

[ccojocar]

Please could you fix the lint warnings?

[ccojocar]

Why was required to remove this mutex? How is related to this change?

[matthewhartstonge]

This mutex was only used when mutating the config (refer: https://github.com/securego/gosec/pull/1053/files). But the mutation of the config causes potential issues for the next worker.

Given the config was passed in via a pointer to func (gosec *Analyzer) load(pkgPath string, conf *packages.Config), the previous code lines mutate the top level config, potentially removing the build flags for the next worker:

	gosec.mu.Lock()
	conf.BuildFlags = nil
	defer gosec.mu.Unlock()

[ccojocar]

Can you please run the make build-race and post the result? Thanks

[ccojocar]

Ins't this the only fix required to get properly the build flags propagated? Why's the reason for the extended refactoring?

[matthewhartstonge]

Couple of reasons:

• The default builder wants build tags in the form []string{"tag1", "tag2"}
• packages.Load wants cli build flags in the form of []string{"-tags=tag1,tag2", "-ldflags=\"-s -w\""}
• Modifying the top level config here by nil'ing out the build flags here could remove build tags for the next worker.

The change was made to stop top level config mutation (passed in as a pointer). By defining the config locally the mutex isn't required.

In the PR description I mentioned a possible refactor: moving the config creation back and making load() take in both buildTags []string and the fixed conf *packages.Config which could reduce code churn.

However, passing a shared conf *packages.Config again risks future contributors accidentally mutating it, so preferred creating the config where it's required.

Other option is to deep-copy the config before calling gosec.load().

Happy to refine things as required! 😄

[matthewhartstonge]

To not break the API for existing tests, I decided to use variadic options to inject expected build tags under test.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 51.61290% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.66%. Comparing base (1216c9b) to head (5137d37).
⚠️ Report is 127 commits behind head on master.

Files with missing lines	Patch %	Lines
testutils/pkg.go	0.00%	15 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1413      +/-   ##
==========================================
- Coverage   68.49%   64.66%   -3.84%     
==========================================
  Files          75       76       +1     
  Lines        4384     4590     +206     
==========================================
- Hits         3003     2968      -35     
- Misses       1233     1487     +254     
+ Partials      148      135      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.