From b5a338f43abd59aa8bc473a1e7065f847948808a Mon Sep 17 00:00:00 2001 From: Ronny Baturov Date: Thu, 5 Dec 2024 11:28:05 +0200 Subject: [PATCH] OCPBUGS-45346: performanceprofile cpuset input validation (#1231) * performanceprofile cpuset input validation This fix ensures that the input for any CPU set field is valid. If an invalid string that does not represent a valid CPU set is provided, the admission webhook will return an appropriate error, such as: spec.cpu: Internal error: strconv.Atoi: parsing "garbage": invalid syntax This replaces the previous behavior where the server would return an error like: Error from server: error when creating "pp.yaml": admission webhook "vwb.performance.openshift.io" denied the request: panic: runtime error: invalid memory address or nil pointer dereference [recovered]. Signed-off-by: Ronny Baturov * added fuzz tests for ValidateCPUs function Fuzz test for ValidateCPUs to verify it handles both valid and invalid inputs without panicking. This ensures that all potential errors are exercised and no panic occurs during execution. Signed-off-by: Ronny Baturov --------- Signed-off-by: Ronny Baturov --- Makefile | 5 ++- .../v2/performanceprofile_validation.go | 2 + .../v2/performanceprofile_validation_test.go | 41 +++++++++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 0435afb2c..6d46c9b23 100644 --- a/Makefile +++ b/Makefile @@ -141,9 +141,12 @@ endif vet: $(BINDATA) $(GO) vet ./... -test-unit: $(BINDATA) +test-unit: $(BINDATA) test-fuzz $(GO) test ./cmd/... ./pkg/... -coverprofile cover.out +test-fuzz: + $(GO) test ./pkg/apis/performanceprofile/v2 -fuzz=FuzzValidateCPUs -fuzztime=10s + clean: $(GO) clean $(PACKAGE_MAIN) rm -rf $(BINDATA) $(OUT_DIR) tmp diff --git a/pkg/apis/performanceprofile/v2/performanceprofile_validation.go b/pkg/apis/performanceprofile/v2/performanceprofile_validation.go index f353b3320..c71a75ca1 100644 --- a/pkg/apis/performanceprofile/v2/performanceprofile_validation.go +++ b/pkg/apis/performanceprofile/v2/performanceprofile_validation.go @@ -179,6 +179,8 @@ func (r *PerformanceProfile) validateCPUs() field.ErrorList { cpuLists, err := components.NewCPULists(string(*cpus.Reserved), string(*cpus.Isolated), offlined, shared) if err != nil { allErrs = append(allErrs, field.InternalError(field.NewPath("spec.cpu"), err)) + // If err != nil then the cpuList is nil and we can't continue with the function logic + return allErrs } if cpuLists.GetReserved().IsEmpty() { diff --git a/pkg/apis/performanceprofile/v2/performanceprofile_validation_test.go b/pkg/apis/performanceprofile/v2/performanceprofile_validation_test.go index 75fc74a0a..a2336783b 100644 --- a/pkg/apis/performanceprofile/v2/performanceprofile_validation_test.go +++ b/pkg/apis/performanceprofile/v2/performanceprofile_validation_test.go @@ -2,6 +2,7 @@ package v2 import ( "fmt" + "testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -153,6 +154,33 @@ func NewPerformanceProfile(name string) *PerformanceProfile { } } +// Fuzz test for ValidateCPUs to ensure it handles invalid inputs and does not panic. +func FuzzValidateCPUs(f *testing.F) { + seeds := []string{"garbage", "a,b,c", "0-1"} + for _, seed := range seeds { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, input string) { + cpuFields := map[string]func(*PerformanceProfile, CPUSet){ + "reserved": func(p *PerformanceProfile, input CPUSet) { p.Spec.CPU.Reserved = &input }, + "isolated": func(p *PerformanceProfile, input CPUSet) { p.Spec.CPU.Isolated = &input }, + "shared": func(p *PerformanceProfile, input CPUSet) { p.Spec.CPU.Shared = &input }, + "offline": func(p *PerformanceProfile, input CPUSet) { p.Spec.CPU.Offlined = &input }, + } + + for fieldName, setField := range cpuFields { + t.Run(fieldName, func(t *testing.T) { + cpuSet := CPUSet(input) + profile := NewPerformanceProfile("test") + + setField(profile, cpuSet) + // We don't care for the errors we got, only care about panics, which will cause a failure if they occur. + _ = profile.validateCPUs() + }) + } + }) +} + var _ = Describe("PerformanceProfile", func() { var profile *PerformanceProfile @@ -265,6 +293,19 @@ var _ = Describe("PerformanceProfile", func() { Expect(errors).NotTo(BeEmpty(), "should have validation error when isolated and shared CPUs have overlap") Expect(errors[0].Error()).To(Or(ContainSubstring("isolated and shared cpus overlap"), ContainSubstring("shared and isolated cpus overlap"))) }) + DescribeTable("should reject invalid input that does not represent CPU sets", + func(fieldSetter func(*PerformanceProfile, CPUSet), cpusField string) { + garbageInput := CPUSet("garbage") + fieldSetter(profile, garbageInput) + errors := profile.validateCPUs() + Expect(errors).NotTo(BeEmpty(), "should have error when "+cpusField+" is filled with garbage input") + Expect(errors[0].Error()).To(Or(ContainSubstring("Internal error: strconv.Atoi: parsing"))) + }, + Entry("reserved CPUs", func(p *PerformanceProfile, input CPUSet) { p.Spec.CPU.Reserved = &input }, "reserved CPUs"), + Entry("isolated CPUs", func(p *PerformanceProfile, input CPUSet) { p.Spec.CPU.Isolated = &input }, "isolated CPUs"), + Entry("shared CPUs", func(p *PerformanceProfile, input CPUSet) { p.Spec.CPU.Shared = &input }, "shared CPUs"), + Entry("offline CPUs", func(p *PerformanceProfile, input CPUSet) { p.Spec.CPU.Offlined = &input }, "offline CPUs"), + ) }) Describe("CPU Frequency validation", func() {