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

@@ -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 {

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 {

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{}{}
 

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 {

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()
 	})

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