Fix seg fault in scanner.c: Allocate the stack array if null

[afroozeh]

#115 introduces a segfault in the following function in scanner.c. I noticed
(signal: 11, SIGSEGV: invalid memory reference in some of our tests after upgrading.

void tree_sitter_kotlin_external_scanner_deserialize(void *payload, const char *buffer, unsigned length) {
  Stack *stack = (Stack *)payload;
  if (length > 0) {
    memcpy(stack->contents, buffer, length); // <<< stack.contents can be null
    stack->size = length;
  } else {
    array_clear(stack);
  }
}

This PR fixes the segfault by adding a null check here.

[afroozeh]

@ObserverOfTime @fwcd please check this PR. This is a serious issue. Probably there is another cause for stack.content being null here, so I'm not sure if this is a good fix, also not sure about the de-allocation.

[ObserverOfTime]

Can you post the crashing tests?

[afroozeh]

@ObserverOfTime It's not easy to reproduce it with a simple example. It's happening in our private repo and there are a lot of rewriting happening. I think something goes wrong in the Stack serialization/deserialization. Do other tree-sitter parsers use the same mechanism for serializing/deserializing as in your refactoring?
btw, this is the stack trace when the bad memory access happens.

[afroozeh]

Also, regardless of the example, under which circumstances stack->contents can be null here?

[ObserverOfTime]

It's not easy to reproduce it with a simple example. It's happening in our private repo and there are a lot of rewriting happening.

Can you at least run the fuzzer (on a Linux machine) using your repo's Kotlin files as the corpus?

make fuzz LANG_NAME=kotlin LANG_DIR=/path/to/tree-sitter-kotlin CORPUS_DIR=/path/to/corpus

Also, regardless of the example, under which circumstances stack->contents can be null here?

The built-in array API handles the contents field. You should ask @amaanq for details.

[afroozeh]

I don’t have access to Linux unfortunately, only Mac. I looked a bit more into the example that triggered the unsafe memory access, and it's not the example that is problematic. What's happening is that there are many changes to the file, and the incremental parser tries to deserialize the scanner and this happens. Two questions here: - Is this the way Scanner deserialization is supposed to work in tree-sitter? Do other languages work like this? - What are the guarantees here for `stack->contents` not being null? @amaanq

[fwcd]

Just fyi, the fuzzer does actually work on macOS, you just need to use the Homebrew-installed version of LLVM (brew install llvm) since Apple Clang apparently does not bundle libfuzzer:

PATH="/opt/homebrew/opt/llvm/bin:$PATH" make LANG_NAME=kotlin LANG_DIR=path/to/tree-sitter-kotlin ...

(Perhaps a note on this could be added to the fuzzer repo, might be useful for other developers on macOS?)

[afroozeh]

I don't know what's going on but when I run the fuzzer using the instruction, I get this:

fuzzer(20459,0x1f3664c00) malloc: nano zone abandoned due to inability to reserve vm space.
ParseDictionaryFile: file does not exist or is empty
make: *** [fuzz] Error 1

Also, not sure why it would help. As I mentioned, this is happening when modifying a single file, serializing/deserializing. Not gonna hit this bug with a single file parsing. You'll gonna hit this bug in an IDE, for example.

Anyway, I think now I have the more proper fix. I was looking which other tree-sitter grammars use an external scanner and saw Python. If you look here:

https://github.com/tree-sitter/tree-sitter-python/blob/0dee05ef958ba2eae88d1e65f24b33cad70d4367/src/scanner.c#L403

You'll see that before memcpy the size of the array is checked and enlarged if needed using array_reserve. In this function, there is this check that malloc is called if array is null:

if (self->contents) {
      self->contents = ts_realloc(self->contents, new_capacity * element_size);
    } else {
      self->contents = ts_malloc(new_capacity * element_size);
    }

Please check my new commit that uses array_resize. This should fix the problem.

[fwcd]

Neat, thanks for digging into this and for the fix!