encoding/base32: Add RFC-compliant error handling and improve reliability

[zolk3ri]

The current implementation of base32.decode() uses assertions for (incomplete) validation, which causes programs to crash on invalid input. This PR replaces assertions with proper error handling according to RFC 4648, allowing users of the package to handle invalid input gracefully.

Motivation: While implementing a pure Odin implementation of TOTP (Time-based One-Time Password), I discovered that base32.decode() would segfault on invalid input due to assertions. As a core library package, it should provide proper error handling to allow users to gracefully handle decode errors, which these changes enable.

Problem Description

• Programs crash with segfaults on invalid base32 input due to assertions
• Explicit disabling of bounds checking (#no_bounds_check) prevents proper error handling
• Buffer allocation issues can cause out-of-bounds access
• Non-compliance with RFC 4648 padding rules

The original implementation prioritized performance by disabling bounds checking, but this tradeoff is not appropriate for a core library package. The small or potentially negligible performance overhead from bounds checking is worth the improved safety and usability.

Implementation Details

The error enum:

Error :: enum {
  None,
  Invalid_Character, // Input contains characters outside the specified alphabet
  Invalid_Length,    // Input length is not valid for base32 (must be a multiple of 8 with proper padding)
  Malformed_Input,   // Input has improper structure (wrong padding position or incomplete groups)
}

Before (crashes on invalid input):

data := decode(input)
if data == nil { // Only checks for empty input
  return nil
}
defer delete(data)

After (error handling available):

data, err := decode(input)
if err != .None {
  #partial switch err {
    case .Invalid_Character:
    // Handle error where input contains characters not in base32 alphabet
    case .Malformed_Input:
    // Handle error where invalid padding structure or position
    case .Invalid_Length:
    // Handle error where input length is not valid for base32
  }
}
defer delete(data)

Key implementation changes:

• Function now returns ([]byte, Error) instead of just []byte
• Removed #no_bounds_check directive for safety
• Proper buffer size calculation
• Added bounds validation throughout
• Proper padding validation
• RFC 4648 compliant error handling
• Support for custom alphabets with custom validation

Design Decisions

1. Bounds Checking:

   • Original code used #no_bounds_check for performance
   • Removed this directive despite small (if any) performance impact
   • Proper error handling requires bounds validation
   • Safety and usability outweigh minor performance cost
   • Follows Odin's philosophy of being explicit and safe by default

2. Error Handling:

   • Simple enum with distinct error cases
   • Clear separation between character, length, and structure errors
   • RFC 4648 compliant validation
   • Empty input returns (nil, .None) by convention

3. Testing:

   • RFC 4648 test vectors
   • Tests all error conditions
   • Verifies correct handling of edge cases

Breaking Changes

1. Function Signature:

   • decode() now returns ([]byte, Error) instead of []byte
   • Error handling available but not required

2. Performance:

   • Minor overhead from bounds checking
   • No additional allocations for error handling

3. Error Handling:

   • Programs can handle errors if desired
   • No more unexpected crashes on invalid input

Testing

Test coverage includes:

• RFC 4648 test vectors and validation rules
• Character validation (invalid characters and case sensitivity)
• Padding validation (placement and length requirements)
• Length validation
• Roundtrip encoding/decoding
• Custom alphabet support

All tests pass with 100% coverage of the new functionality and error cases.

Future Work

• Add similar error handling improvements to encoding/base64 package
• Consider adding error handling to encode() for input validation

Thank you for considering this contribution.

Happy New Year! :)

[bullyingteen]

encoding/base64 needs the same treatment I guess?
See base64.decode_into

[zolk3ri]

encoding/base64 needs the same treatment I guess? See base64.decode_into

Yup! That is why I mentioned it under "Future Work". If this happens to get merged, I will work on encoding/base64 as well.

[bullyingteen]

encoding/base64 needs the same treatment I guess? See base64.decode_into

Yup! That is why I mentioned it under "Future Work". If this happens to get merged, I will work on encoding/base64 as well.

Missed it ngl 🙂

[Waqar144]

can skip 0 initialization here by using ---

[Waqar144]

style: you can use range style loop: for j in 0 ..< block_size if you want

[Waqar144]

style: you can use range style loop: for c in data if you want.

alternatively perhaps bytes.index_byte from core:bytes can be used to find the index of first PADDING. and then use that for this loop and the one below to eliminate the PADDING branch.

Also #no_bounds_check can be used here

[Waqar144]

#no_bounds_check can be used here

[Waqar144]

#no_bounds_check can be used here

[Waqar144]

can #no_bounds_check can be used for this proc?