From b652df4c00536369507400b98cc2998870c9addc Mon Sep 17 00:00:00 2001 From: Pavel Gabriel Date: Thu, 7 Sep 2023 16:53:49 +0200 Subject: [PATCH] Solve panic issues (#275) * validate spec before creating the message * move field spec validation code into Validate method * update comment * return results of string comparison instead of panic for StringsByHex * ignore panic liner alert for fuzzer * move bitmap code back to Bitmap method --- field/composite.go | 48 ++------------------------------------ field/spec.go | 46 ++++++++++++++++++++++++++++++++++++ message.go | 18 +++++++------- message_spec.go | 20 ++++++++++++++++ sort/strings.go | 15 ++++++------ test/fuzz-reader/reader.go | 2 +- 6 files changed, 86 insertions(+), 63 deletions(-) diff --git a/field/composite.go b/field/composite.go index 40bd6b9e..2edeea52 100644 --- a/field/composite.go +++ b/field/composite.go @@ -10,7 +10,6 @@ import ( "strconv" "github.com/moov-io/iso8583/encoding" - "github.com/moov-io/iso8583/padding" "github.com/moov-io/iso8583/prefix" "github.com/moov-io/iso8583/sort" @@ -120,8 +119,8 @@ func (f *Composite) GetSubfields() map[string]Field { // should only pass None or nil values for ths type. Passing any other value // will result in a panic. func (f *Composite) SetSpec(spec *Spec) { - if err := validateCompositeSpec(spec); err != nil { - panic(err) + if err := spec.Validate(); err != nil { + panic(err) //nolint // as specs moslty static, we panic on spec validation errors } f.spec = spec @@ -609,49 +608,6 @@ func (f *Composite) skipUnknownTLVTags() bool { return f.spec.Tag != nil && f.spec.Tag.SkipUnknownTLVTags && (f.spec.Tag.Enc == encoding.BerTLVTag || f.spec.Tag.PrefUnknownTLV != nil) } -func validateCompositeSpec(spec *Spec) error { - if spec.Enc != nil { - return fmt.Errorf("Composite spec only supports a nil Enc value") - } - if spec.Pad != nil && spec.Pad != padding.None { - return fmt.Errorf("Composite spec only supports nil or None spec padding values") - } - if (spec.Bitmap == nil && spec.Tag == nil) || (spec.Bitmap != nil && spec.Tag != nil) { - return fmt.Errorf("Composite spec only supports a definition of Bitmap or Tag, can't stand both or neither") - } - - // If bitmap is defined, validates subfields keys. - // spec.Tag is not validated. - if spec.Bitmap != nil { - if !spec.Bitmap.spec.DisableAutoExpand { - return fmt.Errorf("Composite spec only supports a bitmap with 'DisableAutoExpand = true'") - } - - for key := range spec.Subfields { - parsedKey, err := strconv.Atoi(key) - if err != nil { - return fmt.Errorf("error parsing key from bitmapped subfield definition: %w", err) - } - - if parsedKey <= 0 { - return fmt.Errorf("Composite spec only supports integers greater than 0 as keys for bitmapped subfields definition") - } - } - - return nil - } - - // Validate spec.Tag. - if spec.Tag.Sort == nil { - return fmt.Errorf("Composite spec requires a Tag.Sort function to define a Tag") - } - if spec.Tag.Enc == nil && spec.Tag.Length > 0 { - return fmt.Errorf("Composite spec requires a Tag.Enc to be defined if Tag.Length > 0") - } - - return nil -} - func orderedKeys(kvs map[string]Field, sorter sort.StringSlice) []string { keys := make([]string, 0, len(kvs)) for k := range kvs { diff --git a/field/spec.go b/field/spec.go index d726abe4..6fd14d5c 100644 --- a/field/spec.go +++ b/field/spec.go @@ -1,7 +1,9 @@ package field import ( + "fmt" "reflect" + "strconv" "github.com/moov-io/iso8583/encoding" "github.com/moov-io/iso8583/padding" @@ -89,6 +91,50 @@ func NewSpec(length int, desc string, enc encoding.Encoder, pref prefix.Prefixer } } +// Validate validates the spec. +func (s *Spec) Validate() error { + if s.Enc != nil { + return fmt.Errorf("Composite spec only supports a nil Enc value") + } + if s.Pad != nil && s.Pad != padding.None { + return fmt.Errorf("Composite spec only supports nil or None spec padding values") + } + if (s.Bitmap == nil && s.Tag == nil) || (s.Bitmap != nil && s.Tag != nil) { + return fmt.Errorf("Composite spec only supports a definition of Bitmap or Tag, can't stand both or neither") + } + + // If bitmap is defined, validates subfields keys. + // spec.Tag is not validated. + if s.Bitmap != nil { + if !s.Bitmap.spec.DisableAutoExpand { + return fmt.Errorf("Composite spec only supports a bitmap with 'DisableAutoExpand = true'") + } + + for key := range s.Subfields { + parsedKey, err := strconv.Atoi(key) + if err != nil { + return fmt.Errorf("error parsing key from bitmapped subfield definition: %w", err) + } + + if parsedKey <= 0 { + return fmt.Errorf("Composite spec only supports integers greater than 0 as keys for bitmapped subfields definition") + } + } + + return nil + } + + // Validate spec.Tag. + if s.Tag.Sort == nil { + return fmt.Errorf("Composite spec requires a Tag.Sort function to define a Tag") + } + if s.Tag.Enc == nil && s.Tag.Length > 0 { + return fmt.Errorf("Composite spec requires a Tag.Enc to be defined if Tag.Length > 0") + } + + return nil +} + // CreateSubfield creates a new instance of a field based on the input // provided. func CreateSubfield(specField Field) Field { diff --git a/message.go b/message.go index 1ad2f5a2..51c7a474 100644 --- a/message.go +++ b/message.go @@ -33,6 +33,11 @@ type Message struct { } func NewMessage(spec *MessageSpec) *Message { + // Validate the spec + if err := spec.Validate(); err != nil { + panic(err) //nolint // as specs moslty static, we panic on spec validation errors + } + fields := spec.CreateMessageFields() return &Message{ @@ -52,15 +57,10 @@ func (m *Message) Bitmap() *field.Bitmap { return m.bitmap } - bitmap, ok := m.fields[bitmapIdx] - if !ok { - return nil - } - - if m.bitmap, ok = bitmap.(*field.Bitmap); !ok { - panic(fmt.Sprintf("bitmap field is %T not of type *field.Bitmap", m.fields[bitmapIdx])) - } - + // We validate the presence and type of the bitmap field in + // spec.Validate() when we create the message so we can safely assume + // it exists and is of the correct type + m.bitmap, _ = m.fields[bitmapIdx].(*field.Bitmap) m.bitmap.Reset() m.fieldsMap[bitmapIdx] = struct{}{} diff --git a/message_spec.go b/message_spec.go index 45c00fad..8e1159f2 100644 --- a/message_spec.go +++ b/message_spec.go @@ -1,6 +1,7 @@ package iso8583 import ( + "fmt" "reflect" "github.com/moov-io/iso8583/field" @@ -11,6 +12,25 @@ type MessageSpec struct { Fields map[int]field.Field } +// Validate checks if the MessageSpec is valid. +func (s *MessageSpec) Validate() error { + // we require MTI and Bitmap fields + if _, ok := s.Fields[mtiIdx]; !ok { + return fmt.Errorf("MTI field (%d) is required", mtiIdx) + } + + if _, ok := s.Fields[bitmapIdx]; !ok { + return fmt.Errorf("Bitmap field (%d) is required", bitmapIdx) + } + + // check type of the bitmap field + if _, ok := s.Fields[bitmapIdx].(*field.Bitmap); !ok { + return fmt.Errorf("Bitmap field (%d) must be of type *field.Bitmap", bitmapIdx) + } + + return nil +} + // Creates a map with new instances of Fields (Field interface) // based on the field type in MessageSpec. func (s *MessageSpec) CreateMessageFields() map[int]field.Field { diff --git a/sort/strings.go b/sort/strings.go index 7efa44b2..763b948e 100644 --- a/sort/strings.go +++ b/sort/strings.go @@ -1,7 +1,6 @@ package sort import ( - "fmt" "math/big" "sort" "strconv" @@ -33,19 +32,21 @@ func StringsByInt(x []string) { }) } -// StringsByHex sorts a slice of strings according to their big-endian Hex value. -// This function panics in the event that an element in the slice cannot be -// converted to a Hex slice. Each string representation of a hex value must be -// of even length. +// StringsByHex sorts a slice of strings according to their big-endian Hex +// value. This function compares values as a regular strings in the event that +// an element in the slice cannot be converted to a Hex slice. Each string +// representation of a hex value must be of even length. func StringsByHex(x []string) { sort.Slice(x, func(i, j int) bool { valI, err := encoding.ASCIIHexToBytes.Encode([]byte(x[i])) if err != nil { - panic(fmt.Sprintf("failed to encode ascii hex %s to bytes : %v", x[i], err)) + // we will ignore the error and sort the strings as normal + return x[i] < x[j] } valJ, err := encoding.ASCIIHexToBytes.Encode([]byte(x[j])) if err != nil { - panic(fmt.Sprintf("failed to sort strings by hex: %v", err)) + // we will ignore the error and sort the strings as normal + return x[i] < x[j] } return new(big.Int).SetBytes(valI).Int64() < new(big.Int).SetBytes(valJ).Int64() }) diff --git a/test/fuzz-reader/reader.go b/test/fuzz-reader/reader.go index 8bb4aadc..c66c682c 100644 --- a/test/fuzz-reader/reader.go +++ b/test/fuzz-reader/reader.go @@ -40,7 +40,7 @@ func Fuzz(data []byte) int { _, err = message.Pack() if err != nil { - panic(fmt.Errorf("failed to pack unpacked message: %w", err)) + panic(fmt.Errorf("failed to pack unpacked message: %w", err)) //nolint } return 1