Skip to content

release/v2.0.1#7

Merged
melbasiouny-riverside merged 9 commits into
microhammerfrom
fix/parser-input-validation
Apr 17, 2026
Merged

release/v2.0.1#7
melbasiouny-riverside merged 9 commits into
microhammerfrom
fix/parser-input-validation

Conversation

@melbasiouny-riverside
Copy link
Copy Markdown
Collaborator

@melbasiouny-riverside melbasiouny-riverside commented Apr 16, 2026

  • Widen HToken.len and bits_env.length from uint8_t to size_t, fixing Integer Truncation in h_token Allows Parser Bypass (/src/parsers/token.c) #3. h_bits(264, false) no longer silently wraps to h_bits(8, false). For HToken, sizeof(HToken) is unchanged on common targets as the trailing struct padding absorbs the difference.
  • Introduce HAMMER_ASSERT in internal.h and replace assert()-based programmer-error guards across bitwriter.c, glue.c, glue.h, and datastructures.c, closing assert() used as sole input validation which is silently disabled under -DNDEBUG #6. Unlike assert(), HAMMER_ASSERT always aborts regardless of -DNDEBUG. Runtime boundary conditions such as the sloballoc size check retain explicit safe returns.
  • Update h_assert_type macro to use HAMMER_ASSERT, aborting on type mismatch in all builds.
  • Update test_token_len_assert to reflect the new behavior: tokens longer than 255 bytes are now valid, and the test verifies a 256-byte token parses correctly end-to-end.
  • Add ruff linting of Python and SCons files to the CI pipeline alongside other formatting checks before the build-deb step.

@jhjones-riverside
Copy link
Copy Markdown
Collaborator

We should investigate how we want to go about replacing the asserts. Some cases returning NULL, 0, etc may be appropriate or calling abort() may be the appropriate.

Comment thread src/parsers/bits.c
Comment thread src/parsers/bits.c
Comment thread src/bitwriter.c Outdated
Comment thread src/datastructures.c
Comment thread tests/parsers/test_token.c
Copy link
Copy Markdown
Collaborator

@GJames-RiversideResearch GJames-RiversideResearch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think utilizing a fixed size data structure instead of size_t could be valuable and we should also consider keeping in asserts for debug builds to give rich error messages

@melbasiouny-riverside
Copy link
Copy Markdown
Collaborator Author

Thanks for your feedback @jhjones-riverside and @GJames-RiversideResearch. I agree that a blanket silent-fail approach isn't ideal. I'm thinking of adding a HAMMER_ASSERT macro that always calls abort() regardless of -DNDEBUG for programmer-error guards, while keeping explicit safe returns for true runtime boundary conditions like the sloballoc size check. Open to other approaches if you all have something else in mind.

Copy link
Copy Markdown
Collaborator

@GJames-RiversideResearch GJames-RiversideResearch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments have been adressed, happy with the changes

@melbasiouny-riverside melbasiouny-riverside removed the request for review from jhjones-riverside April 17, 2026 17:48
Comment thread src/bitwriter.c Outdated
Copy link
Copy Markdown
Collaborator

@GJames-RiversideResearch GJames-RiversideResearch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with 1 slightly pedantic comment

@melbasiouny-riverside melbasiouny-riverside merged commit b1508af into microhammer Apr 17, 2026
4 checks passed
@melbasiouny-riverside melbasiouny-riverside deleted the fix/parser-input-validation branch April 17, 2026 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants