From 74c57245547d127cfc7bd4ea7fadb5de5b1066de Mon Sep 17 00:00:00 2001 From: Anas Khan Date: Wed, 15 Jul 2026 15:45:35 +0530 Subject: [PATCH] Fix: nil pointer panic in getMatchingDefinitionRevision on malformed revision version (#7207) * Fix: nil pointer panic in getMatchingDefinitionRevision on malformed revision version getMatchingDefinitionRevision dereferenced the semver result (v.String()) before checking semver.NewVersion's parse error. A DefinitionRevision whose name carries the expected prefix but an unparseable version segment (e.g. configmap-component-v1.bad) made semver.NewVersion return (nil, err), so v.String() panicked instead of returning the error. Move the error check ahead of the map write so a bad version returns the error as intended. Valid input is unaffected. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com> * test: cover empty version segment in getMatchingDefinitionRevision Add a regression case for a DefinitionRevision whose version segment is empty (name ending in the definition name plus a trailing hyphen, e.g. configmap-component-). Such a name is filtered out by the revisionPrefix HasPrefix check before it reaches semver.NewVersion, and semver.NewVersion would return an error for an empty string anyway, so the same err != nil guard that fixes the malformed-version panic already rejects it cleanly. This locks in that behaviour and addresses the empty-segment edge case raised in review. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com> --------- Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com> --- pkg/oam/util/helper.go | 2 +- pkg/oam/util/helper_test.go | 58 +++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/pkg/oam/util/helper.go b/pkg/oam/util/helper.go index fa9d4a79a..9d9dfe560 100644 --- a/pkg/oam/util/helper.go +++ b/pkg/oam/util/helper.go @@ -354,10 +354,10 @@ func getMatchingDefinitionRevision(exactRevisionName, definitionName string, rev if strings.HasPrefix(revision.Name, revisionPrefix) { version := strings.Split(revision.Name, definitionName+"-")[1] v, err := semver.NewVersion(version) - orignalVersions[v.String()] = version if err != nil { return "", err } + orignalVersions[v.String()] = version definitionVersions = append(definitionVersions, v) } } diff --git a/pkg/oam/util/helper_test.go b/pkg/oam/util/helper_test.go index 2c5ae83f8..86f7008b3 100644 --- a/pkg/oam/util/helper_test.go +++ b/pkg/oam/util/helper_test.go @@ -992,6 +992,16 @@ func TestGetLatestDefinitionRevisionName(t *testing.T) { defRevisionList.DeepCopyInto(list.(*v1beta1.DefinitionRevisionList)) return nil }} + malformedListCli := test.MockClient{MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + defRevisionList := getMalformedComponentDefRevisionList() + defRevisionList.DeepCopyInto(list.(*v1beta1.DefinitionRevisionList)) + return nil + }} + emptyVersionListCli := test.MockClient{MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + defRevisionList := getEmptyVersionComponentDefRevisionList() + defRevisionList.DeepCopyInto(list.(*v1beta1.DefinitionRevisionList)) + return nil + }} testcases := []struct { name string @@ -1053,6 +1063,24 @@ func TestGetLatestDefinitionRevisionName(t *testing.T) { client: &traitListCli, err: fmt.Errorf("error finding definition revision for Name: scaler-trait, Type: Trait"), }, + { + name: "Malformed Component version segment returns error not panic", + inputRevisionName: "configmap-component-v1", + definitionName: "configmap-component", + definitionType: "Component", + expectedDefRevisionName: "", + client: &malformedListCli, + err: fmt.Errorf("error finding definition revision for Name: configmap-component, Type: Component"), + }, + { + name: "Empty Component version segment returns error not panic", + inputRevisionName: "configmap-component-v1", + definitionName: "configmap-component", + definitionType: "Component", + expectedDefRevisionName: "", + client: &emptyVersionListCli, + err: fmt.Errorf("error finding definition revision for Name: configmap-component, Type: Component"), + }, } ctx := context.Background() for _, tc := range testcases { @@ -1221,6 +1249,36 @@ func getComponentDefRevisionList() v1beta1.DefinitionRevisionList { return compRevisionList } +func getMalformedComponentDefRevisionList() v1beta1.DefinitionRevisionList { + compDefRevision := componentDefinitionRevision.DeepCopy() + compDefRevision.Name = "configmap-component-v1.bad" + + return v1beta1.DefinitionRevisionList{ + TypeMeta: metav1.TypeMeta{ + Kind: "DefinitionRevision", + APIVersion: "core.oam.dev/v1beta1", + }, + Items: []v1beta1.DefinitionRevision{ + *compDefRevision, + }, + } +} + +func getEmptyVersionComponentDefRevisionList() v1beta1.DefinitionRevisionList { + compDefRevision := componentDefinitionRevision.DeepCopy() + compDefRevision.Name = "configmap-component-" + + return v1beta1.DefinitionRevisionList{ + TypeMeta: metav1.TypeMeta{ + Kind: "DefinitionRevision", + APIVersion: "core.oam.dev/v1beta1", + }, + Items: []v1beta1.DefinitionRevision{ + *compDefRevision, + }, + } +} + func getTraitDefRevisionList() v1beta1.DefinitionRevisionList { traitDefRevision1 := traitDefinitionRevision.DeepCopy() traitDefRevision1.Spec.TraitDefinition.Spec.Version = "1.2.0"