spider-gazelle/bindata — comprehensive audit (road to 1.0), produced with Claude Opus 4.8 [1M] and hand-verified

bindata-audit.md

spider-gazelle/bindata — comprehensive audit (road to 1.0), produced with Claude Opus 4.8 [1M] and hand-verified

spider-gazelle/bindata — comprehensive audit (road to 1.0)

The foundations are good. The declarative macro DSL (field/bit_field/group/endian) is elegant, the CustomException hierarchy is clean, the generated ParseError/WriteError wrapping is exactly right, and the ASN.1/BER support is ambitious. The suite passes (50 examples, 0 failures). What follows is a constructive map of what stands between today and a 1.0 tag — and I'm offering to contribute the fixes, starting with the blockers.

ℹ️ Transparency: this audit was produced with Claude Opus 4.8 [1M] (multi-pass static review), then the headline findings were hand-verified by compiling/running repros against the source (Crystal 1.20.2). Items tagged VERIFIED have program output behind them.

Summary by tier

Tier	Theme	Headline
0	Blockers	5 defects that pass crystal build but break when exercised: large-arc OID silently corrupted, OverflowError on OID, value: on Float won't compile, macro-name collision that breaks the DSL, unbounded allocation / memory DoS from attacker-controlled length
1	Robustness / error model	IndexError on empty payloads, not_nil! on truncated input, untyped raise "string", ASN.1 overrides bypass the error envelope
2	Concurrency / fiber-safety	BitField is a per-class singleton mutated in place → two fibers (de)serializing the same class corrupt each other
3	ASN.1/BER protocol completeness	multi-byte OID sub-identifiers unimplemented, long/short-form off-by-one, 4-byte length → negative Int32, bitfields always big-endian, format ignored
4	API / SemVer	every type injects a global macro → namespace pollution + O(n²) compile time; endian is order-dependent
5	Tests / CI	OID specs only cover arcs < 16384 (so #0.1 went unnoticed), no indefinite/extended/truncated tests, dead .travis.yml, ameba not run in CI

---

Tier 0 — Blockers (verified)

0.1 — Large-arc OID silently corrupted 🔴 VERIFIED

src/bindata/asn1/data_types.cr:87-99 (encode) and :57-69 (decode) only implement 2-byte sub-identifiers (14 bits, max 16383). Any arc ≥ 16384 — i.e. the most common RSA/PKCS/X.509 OIDs — is mis-encoded with no error:

set_object_id("1.2.840.113549.1.1.1")
encoded payload : 2a8648f70d010101          # one byte (86) missing
expected (DER)  : 2a864886f70d010101
round-trip      : 1.2.840.15245.1.1.1       # 113549 corrupted to 15245
MATCH           : false

The standard encoding is base-128 with a continuation bit (7 useful bits/byte, unbounded byte count). Fix: a generic base-128 encoder/decoder.

0.2 — OverflowError on large second arc 🔴 VERIFIED

data_types.cr:90: (40 * value[0] + value[1]).to_u8. Under joint-iso-itu-t (first arc = 2) the second arc may exceed 47, and 40*2 + 999 = 1079 overflows UInt8:

set_object_id("2.999.3")  ->  Unhandled exception: Arithmetic overflow (OverflowError)
                              from data_types.cr:90 in 'set_object_id'

The combined 40*X+Y value must also be base-128 multi-byte encoded.

0.3 — value: on a Float field won't compile 🔴 VERIFIED

src/bindata.cr:281: @{{name}} = {{cls}}.new(0) | %value. The | (bitwise-or, used to coerce integers) does not exist on floats:

field x : Float64, value: -> { 1.5 }
# Error: undefined method '|' for Float64

So value: is unusable for Float32/Float64. Needs a dedicated branch (direct assignment + .new/.to_f coercion).

0.4 — Macro-name collision breaks the DSL 🔴 VERIFIED

src/bindata.cr:20-27: the inherited hook generates, for every type, a shortcut macro named after the type (Header → header). But RESERVED_NAMES (:8) doesn't protect the DSL names. A subclass whose underscored name equals field/bits/bool/string/bytes/group clobbers the built-in macro, globally:

class Field < BinData; end
class UsesField < BinData
  field x : UInt8 = 1_u8
end
# Error: expanding macro 'field' -> `field x : UInt8 = 1_u8 : Field = Field.new` -> unexpected token ":"

Fix: extend RESERVED_NAMES to cover all DSL macro names.

0.5 — Unbounded allocation / memory DoS from attacker-controlled length 🔴 VERIFIED-by-design

src/bindata/asn1.cr BER#read trusts the declared length before reading any payload, so a tiny hostile message forces a huge allocation or an unbounded read. This is distinct from finding 3.3 (which only covers the negative-Int32 overflow), and is the relevant angle for any network ASN.1 consumer (e.g. LDAP/SNMP):

• definite branch (asn1.cr:83): @payload = Bytes.new(@length.length) allocates up to ~2 GiB (asn1/length.cr:12,21 caps long-form at 4 bytes → Int32) before io.read_fully. A 5-byte header (30 84 7F FF FF FF) forces a ~2 GiB allocation against an essentially empty input.
• indefinite branch (asn1.cr:67-80): loop { io.read_byte.not_nil! } until 00 00 grows unbounded if the terminator never arrives (overlaps finding 1.2, but the memory-exhaustion angle was missing).
• amplification via children (asn1.cr:109-116): each child re-runs read_bytes(ASN1::BER), so a ~50-byte message can nest a child announcing 2 GiB. Bounding the outer frame is therefore not enough — the guard must live in read and propagate through children.

Fix (per-instance, not a global/class cap, so independent connections get independent limits): add property max_content_length : Int32 = 0 (0 = unlimited → existing users unchanged). Check the cap before Bytes.new in the definite branch; check the accumulated size each iteration in the indefinite branch; and propagate child.max_content_length = @max_content_length inside children. Consumers set the cap by reading the root via BER.new.tap(&.max_content_length=(n)).read(io) rather than io.read_bytes (which can't pass the cap before parsing). Raise a typed ASN1::BER::ContentTooLarge on breach.

---

Tier 1 — Robustness / error model

• 1.1 IndexError on empty payload VERIFIED — get_boolean (data_types.cr:160 @payload[0]) and get_bitstring (:265) index without a bounds check:

  get_boolean(empty)   -> IndexError: Index out of bounds
  get_bitstring(empty) -> IndexError: Index out of bounds
  

  On untrusted input this should be a typed error (InvalidTag/ParseError), not an IndexError.
• 1.2 not_nil! on truncated input — asn1.cr:72 io.read_byte.not_nil! in the indefinite loop raises NilAssertionError (unwrapped) at EOF; same for (length).call.not_nil! (bindata.cr:138,162,170,212).
• 1.3 untyped raise "string" — bitfield.cr:12,145, bindata.cr:399, asn1.cr:45, data_types.cr:269 raise a bare Exception, inconsistent with the CustomException hierarchy (impossible to rescue narrowly).
• 1.4 ASN.1 overrides bypass the envelope — the hand-written read/write of BER/Length/Identifier short-circuit the generated ParseError/WriteError wrapping: errors surface raw.

Tier 2 — Concurrency / fiber-safety

• 2.1 BitField singleton mutated in place VERIFIED — @@bit_fields (bindata.cr:38) is populated once at class init; all instances share the same BitField object:

  instance A and B share the SAME BitField: true  (key "ByteSized_8")
  

  But BitField#read does input.read_fully(@buffer) + @values[name] = … (bitfield.cr:67,156) and #write mutates @values (:335 on the macro side). Two fibers (de)serializing two instances of the same class race on @buffer/@values with no synchronization → values leak between fibers. Fix: instantiate the BitField per operation (or keep operation-local state instead of shared instance attributes). This is the central concurrency finding — the lib is implicitly not fiber-safe per class.

Tier 3 — ASN.1/BER protocol completeness

• 3.1 multi-byte OID sub-identifiers unimplemented (see 0.1/0.2).
• 3.2 long/short-form off-by-one VERIFIED — length.cr:45 self.long = true if @length >= 127. Length 127 fits in short form (7f) but is encoded in non-minimal long form:

  127-byte payload -> length octet(s): 81 7f   (should be: 7f)
  

  Should be > 127.
• 3.3 4-byte length → negative Int32 — length.cr:12-17,21 caps at 4 bytes in an Int32; a length with the high bit set becomes silently negative (later caught as ArgumentError, asn1.cr:85, but the model is fragile).
• 3.4 bitfields always big-endian — explicit TODO bitfield.cr:69-70; read/shift/write force BigEndian and ignore format. Bitfields in a little-endian structure may be wrong.
• 3.5 to_io/from_io ignore format — bindata.cr:92-100 accept an IO::ByteFormat then ignore it (the declared endianness wins). Misleading signature.
• 3.6 remaining_bytes/bytes require io.size — bindata.cr:212 io.size - io.pos → doesn't work on a streaming IO (socket).

Tier 4 — API / SemVer

• 4.1 per-type global macro → O(n²) compile time — bindata.cr:18-27: every inherited re-loops over CUSTOM_TYPES (which grows unbounded) and (re)defines a macro in the class. For n BinData types, quadratic compile cost + global namespace pollution (see footgun 0.4).
• 4.2 endian order-dependent — group (bindata.cr:474-490) captures ENDIAN[0] at declaration point; a group placed before endian big silently inherits system.
• 4.3 RESERVED_NAMES incomplete (bindata.cr:8) — includes no DSL macro name.

Tier 5 — Tests / CI / under-covered files

• 5.1 shallow OID specs — spec/bindata_asn1_spec.cr only tests 1.3.6.1.4.1.311.21.20 (max arc 311 < 16384), so #0.1 went undetected. Add arcs ≥ 16384 (RSA 1.2.840.113549.…) and a large first-arc-2 case.
• 5.2 coverage gaps — no tests for: indefinite length, multi-byte long form, multi-byte ExtendedIdentifier tag, empty/truncated payloads, mixed-endian bitfields, error callbacks (after_deserialize/before_serialize that raise), value: on group, verify on remaining_bytes.
• 5.3 dead .travis.yml coexists with .github/workflows/CI.yml → remove it.
• 5.4 ameba declared but not run in CI — dev dependency present, no lint step.
• 5.5 minor perf — BitField#read allocates an IO::Memory.new(buffer) per field inside the loop (bitfield.cr:75) + shift re-allocates; BER#children fully reparses on every call.

---

Proposed plan

I'd open one child issue per tier for independent discussion, and start with a Tier 0 PR: the 4 blockers, each with a failing spec first, plus long-arc OIDs added to the suite so they can't recur.

Two questions before sending code:

1. Spec framework for new specs — stdlib spec (what the suite uses today)?
2. For the protocol-completeness gaps (Tier 3): prefer I implement them, or document the limitation for now? Happy either way.

Thanks for building this — glad to help push it over the line. 🙏