tree-sitter-grammars / tree-sitter-hcl

HCL grammar for tree-sitter
https://tree-sitter-grammars.github.io/tree-sitter-hcl/
Apache License 2.0
92 stars 20 forks source link

fix: serialize buffer size check #47

Closed MichaHoffmann closed 3 months ago

MichaHoffmann commented 3 months ago

During the check that we dont overflow the serialization buffer we erroneously checked with size(uint32_t) == 1 which could cause us to crash with some input strings.

might fix #46

clason commented 3 months ago

@amaanq This change doesn't compile with tree-sitter build (nor with nvim-treesitter flags).

MichaHoffmann commented 3 months ago

@amaanq This change doesn't compile with tree-sitter build (nor with nvim-treesitter flags).

Oh sorry! We should add a CI pass for that. What are the flags and the compiler again? I'll see ti fixing it after work today.

clason commented 3 months ago

Just tree-sitter build will do the trick. (You might wish to rewrite the scanner to use the new headers you get with tree-sitter generate -- @amaanq should be able to do this quickly, he has practice ;))

clason commented 3 months ago

You could also use the upstream workflow while you're at it: https://github.com/tree-sitter-grammars/template/tree/master/.github/workflows

MichaHoffmann commented 3 months ago

Weird, I did run the tests and all multiple times using the tree sitter cli, I wonder why build should break if the compilation for tests worked.

clason commented 3 months ago

Which CLI version?

MichaHoffmann commented 3 months ago
 fedora  ~  git  tree-sitter-hcl   mhoffmann/fix-size-accounting-check-in-serialize   
$ tree-sitter build
 fedora  ~  git  tree-sitter-hcl   mhoffmann/fix-size-accounting-check-in-serialize   
$ tree-sitter
tree-sitter 0.22.5
...
clason commented 3 months ago

Could be compiler version-specific, of course. (Latest CLI -- and the one we test with -- is 0.22.6.)

And you're compiling from the dialects/terraform directory? (Multi-grammar repos are cursed.)

clason commented 3 months ago

I just pushed a fixup with the missing changes. @amaanq You probably still want to rewrite these scanners.