Skip to content

Commit

Permalink
store: Fix SourceConfig.RepoConfig() function call
Browse files Browse the repository at this point in the history
The SourceConfig pointer may be a loop variable that gets reused. This
results in unexpected behavior when the value pointed to is overwritten
by the loop calling this function.

Includes a test to make sure this is fixed.

So, DO NOT point to unsafe variables. Make a new pointer using
common.ToPtr where it is passed by value and returns a pointer to that
new value.

NOTE: This is NOT caught by golangci-lint. There may be other places
where this happens, but I have gone through the potential looking code
in osbuild-composer and images and not found any (other than a couple
places already noted with G601 tags as not a problem).
  • Loading branch information
bcl authored and ondrejbudai committed Nov 1, 2023
1 parent a65f17e commit b786178
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 2 deletions.
4 changes: 2 additions & 2 deletions internal/store/store.go
Expand Up @@ -624,9 +624,9 @@ func (s *SourceConfig) RepoConfig(name string) rpmmd.RepoConfig {

repo.Name = name
repo.IgnoreSSL = common.ToPtr(!s.CheckSSL)
repo.CheckGPG = &s.CheckGPG
repo.CheckGPG = common.ToPtr(s.CheckGPG)
repo.RHSM = s.RHSM
repo.CheckRepoGPG = &s.CheckRepoGPG
repo.CheckRepoGPG = common.ToPtr(s.CheckRepoGPG)
repo.GPGKeys = s.GPGKeys

var urls []string
Expand Down
40 changes: 40 additions & 0 deletions internal/store/store_test.go
Expand Up @@ -489,6 +489,46 @@ func (suite *storeTest) TestRepoConfigMirrorlist() {
suite.Equal(expectedRepo, actualRepo)
}

// Test multiple SourceConfigs with different CheckGPG and CheckRepoGPG settings
func (suite *storeTest) TestSourceConfigGPGKeysTrueFalse() {
// We only care about the GPG bools
sources := map[string]SourceConfig{
"source-with-true": {Name: "source-with-true", CheckGPG: true, CheckRepoGPG: true},
"source-with-false": {Name: "source-with-false", CheckGPG: false, CheckRepoGPG: false},
}

// source is reused inside the loop, which can result in unexpected changes in go < 1.22
// https://go.dev/blog/loopvar-preview
var repos []rpmmd.RepoConfig
for id, source := range sources {
repos = append(repos, source.RepoConfig(id))
}

// First repo should be true, second should be false
suite.True(*repos[0].CheckGPG)
suite.True(*repos[0].CheckRepoGPG)
suite.False(*repos[1].CheckGPG)
suite.False(*repos[1].CheckRepoGPG)

// We only care about the GPG bools
// Test with false then true
sources = map[string]SourceConfig{
"source-with-false": {Name: "source-with-false", CheckGPG: false, CheckRepoGPG: false},
"source-with-true": {Name: "source-with-true", CheckGPG: true, CheckRepoGPG: true},
}

repos = []rpmmd.RepoConfig{}
for id, source := range sources {
repos = append(repos, source.RepoConfig(id))
}

// First repo should be false, second should be true
suite.False(*repos[0].CheckGPG)
suite.False(*repos[0].CheckRepoGPG)
suite.True(*repos[1].CheckGPG)
suite.True(*repos[1].CheckRepoGPG)
}

func TestStore(t *testing.T) {
suite.Run(t, new(storeTest))
}

0 comments on commit b786178

Please sign in to comment.