Skip to content

validate trace packet payload layout before using its accessors#9230

Open
Nashit-h wants to merge 1 commit into
halide:mainfrom
Nashit-h:trace-packet-payload-validation
Open

validate trace packet payload layout before using its accessors#9230
Nashit-h wants to merge 1 commit into
halide:mainfrom
Nashit-h:trace-packet-payload-validation

Conversation

@Nashit-h

Copy link
Copy Markdown

Both trace-packet readers take size, dimensions, lanes and the element type straight from the stream and then hand out pointers computed from them: value() is coordinates() + dimensions and func() is value() + lanes * type().bytes(), so a packet declaring 2000 dimensions with a four-byte payload puts func() 8001 bytes into a 4096-byte payload array. HalideTraceDump runs string(p.func()) on that (ASAN reports a heap-buffer-overflow READ in strlen; without ASAN it prints whatever memory follows the packet as a Func name) and HalideTraceViz indexes coordinates() the same way; size is also used unchecked in size - header_size, so a size below the header underflows the payload length. Both readers now reject an undersized size and check, once the payload is in, that the coordinates and value fit in the bytes actually read and that the func name and trace tag are terminated inside them, which every packet written by src/runtime/tracing.cpp already satisfies since it derives size from exactly those parts.

The trace tools aren't built or exercised by the test suite, so there's no natural home for a regression test; I checked this with crafted trace files against a standalone harness around the reader, and with a well-formed packet to confirm valid input still parses identically.

Checklist

  • Tests added or updated (not required for docs, CI config, or typo fixes)
  • Documentation updated (if public API changed)
  • Python bindings updated (if public API changed)
  • Benchmarks are included here if the change is intended to affect performance.
  • Commits include AI attribution where applicable (see Code of Conduct)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant