Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of serialization buffer overflows #3318

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

maxbrunsfeld
Copy link
Contributor

@maxbrunsfeld maxbrunsfeld commented Apr 25, 2024

Refs tree-sitter/tree-sitter-julia#128

This is a bug in the Julia grammar's external scanner, where the scanner will write past the size of the serialization buffer. Previously, there was no explicit guard against this happening, it would just corrupt other state, and cause confusing crashes downstream.

Now, it is handled better in two different ways:

  • When running a parser natively, we can at least crash with a more informative error location if the scanner indicates that it has written beyond the end of the buffer:

    Assertion failed: (length <= TREE_SITTER_SERIALIZATION_BUFFER_SIZE), function ts_parser__external_scanner_serialize, file parser.c, line 404.
    
  • When running a parser via WASM (e.g. in Zed), because the parser is only writing to the wasm memory, we can completely prevent a crash, and prevent it from affecting anything, except for failing the current parse.

@maxbrunsfeld maxbrunsfeld merged commit 8040bae into master Apr 25, 2024
12 checks passed
@maxbrunsfeld maxbrunsfeld deleted the serialization-buffer-overflows branch April 25, 2024 21:49
@amaanq
Copy link
Member

amaanq commented Apr 25, 2024

nice, that assertion will be useful in debugging when this happens 😁

maxbrunsfeld added a commit that referenced this pull request Apr 26, 2024
Improve handling of serialization buffer overflows
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.

None yet

2 participants