From 911705e1c6745c2d371aad422028022b63fbbcdf Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 2 Sep 2026 15:30:28 +0200 Subject: [PATCH] [client] Do not panic on a config with no sync message version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apply() wrote the incoming sync message version through the stored pointer, without checking it was there: a config that carries no version yet made it dereference nil. Reachable from the update-settings dry run, which runs inside a request handler — where failing closed is the worst acceptable outcome, and a panic is not one. The field is now reassigned like every other optional one, which also means apply() no longer mutates anything the caller still holds through a pointer, so the dry run's copy has one less field to detach. Reported by cubic-dev-ai on PR #7398. --- client/internal/profilemanager/config.go | 22 +++++++++---------- .../config_would_change_test.go | 16 +++++++++++++- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index 6a7ba82a8..6c8f00c2f 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -682,9 +682,12 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if input.SyncMessageVersion != nil && *input.SyncMessageVersion != *config.SyncMessageVersion { + // Assigning the pointer, not writing through it: a config that carries no + // version yet would otherwise be a nil dereference, and a panic inside a + // request handler is not a way to fail. + if input.SyncMessageVersion != nil && (config.SyncMessageVersion == nil || *input.SyncMessageVersion != *config.SyncMessageVersion) { log.Infof("setting SyncMessageVersion to %v", *input.SyncMessageVersion) - *config.SyncMessageVersion = *input.SyncMessageVersion + config.SyncMessageVersion = input.SyncMessageVersion updated = true } @@ -1032,12 +1035,11 @@ func newDryRunBaseline(configPath string) (*Config, error) { return baseline, nil } -// clone returns a copy of the config that apply can be run against without -// the original observing the writes, or nil for a nil receiver. Only what -// apply mutates in place needs detaching: the slices it replaces or appends -// to, and SyncMessageVersion, which it writes through the pointer. The -// remaining pointer fields are reassigned, not written through, and -// ClientCertKeyPair is only overwritten. +// clone returns a copy of the config that apply can be run against without the +// original observing the writes, or nil for a nil receiver. Only what apply +// mutates in place needs detaching, which is the slices it replaces or appends +// to: every pointer field it touches is reassigned rather than written through, +// and ClientCertKeyPair is only overwritten. func (config *Config) clone() *Config { if config == nil { return nil @@ -1047,10 +1049,6 @@ func (config *Config) clone() *Config { probe.IFaceBlackList = slices.Clone(config.IFaceBlackList) probe.NATExternalIPs = slices.Clone(config.NATExternalIPs) probe.DNSLabels = slices.Clone(config.DNSLabels) - if config.SyncMessageVersion != nil { - version := *config.SyncMessageVersion - probe.SyncMessageVersion = &version - } return &probe } diff --git a/client/internal/profilemanager/config_would_change_test.go b/client/internal/profilemanager/config_would_change_test.go index 561c6c914..b83f57356 100644 --- a/client/internal/profilemanager/config_would_change_test.go +++ b/client/internal/profilemanager/config_would_change_test.go @@ -108,7 +108,7 @@ func TestReadsDoNotWriteTheConfigBack(t *testing.T) { denormalized := []byte(`{"WgIface":"wt0"}`) for name, read := range map[string]func(string) (*Config, error){ - "GetExistingConfig": GetExistingConfig, + "GetExistingConfig": GetExistingConfig, "ReadOrGenerateConfig": ReadOrGenerateConfig, } { t.Run(name, func(t *testing.T) { @@ -261,3 +261,17 @@ func TestUpdateConfigProvisionsAMissingIdentity(t *testing.T) { require.NoError(t, err) require.Equal(t, cfg.PrivateKey, persisted.PrivateKey, "the provisioned identity was not persisted") } + +// A config that carries no sync message version must not make the dry run +// panic: the gate runs inside a request handler, where failing closed is the +// worst acceptable outcome. +func TestWouldChangeWithoutAStoredSyncMessageVersion(t *testing.T) { + cfg := seededConfig(t) + require.Nil(t, cfg.SyncMessageVersion, "the fixture is only useful while the field starts out unset") + + version := 2 + changed, err := cfg.WouldChange(ConfigInput{SyncMessageVersion: &version}) + require.NoError(t, err) + require.True(t, changed) + require.Nil(t, cfg.SyncMessageVersion, "the dry run set the version on the stored config") +}