search

DerekStride / tree-sitter-sql

SQL grammar for tree-sitter
http://derek.stride.host/tree-sitter-sql/
MIT License
147 stars 47 forks source link

fix: segfault in scanner.c #226

Closed matthias-Q closed 9 months ago

matthias-Q commented 9 months ago

fixes #221

I think I have found the issue, but I suspect that there are more since we are missing a lot of bounds checks in the scanner.c.

We should also avoid strcpy in favour of strncpy or even better memcpy. But I am by no means an expert in C.

Steps to reproduce: (I use tree-sitter-cli from crates.io)

  1. Setup the tree-sitter-cli config in ~/.config/tree-sitter/config.json and specify the parser-directories. This should point to this repo.
  2. run tree-sitter generate to create the parser.c
  3. compile with cc -shared -O -D_FORTIFY_SOURCE=3 -fPIE -fPIC -I./src src/parser.c src/scanner.c -o sql.so
  4. run valgrind tree-sitter parse <statements.sql>

The examples from issue #221 reveal that in line 185 (almost at the end) there was a malloc with length - 1

Valgrind reports some more issues though.

On my second machine the issue was not reproducable. I suspect the versions of gcc and glibc. On this machine I have GNU C Library (GNU libc) stable release version 2.38. and cc --version cc (GCC) 13.2.1 20230801

On my second machine, I have older versions (gcc 11, glibc 2.35) and it did not segfault.

one more note. The SQL statement in the issue is not parsed correctly anyway. It just does not segfault anymore.

kirillrdy commented 9 months ago

@matthias-Q I can confirm that this fixes segfault for me !

mortezadadgar commented 9 months ago

yes this fixes the issue.

I like to mention a few things:

  1. they're some useless malloc in the code that can be removed like (text_size) but this is not related to this pr
  2. other strcpy usages can be replaced with strncpy as well
matthias-Q commented 9 months ago

It just fixes the segfault. The statements are not parsed correctly, here :disappointed: