fix(lib): replace raw array accesses with array_get#4430
fix(lib): replace raw array accesses with array_get#4430amaanq merged 1 commit intotree-sitter:masterfrom
Conversation
3f4f710 to
b4cc734
Compare
fef1784 to
f98755b
Compare
|
Maybe pull the actual fix out into a separate PR so we can get it into 0.25.4 while still waiting on resolving the refactor question? |
f98755b to
e1d3529
Compare
|
Do we still want to backport this? Normally, one would only backport fixes, not refactors. |
I still view it as a "fix" because the library will now terminate (in debug mode) instead of entering into undefined behavior territory. I don't feel super strongly and am ok with removing the backport, though. |
|
If the effects are only noticeable in Debug mode, I don't think there's much to be gained from backporting. (Unless it makes future backports easier.) |
|
My main thought was that these checks would be accessible through the rust bindings, since tests are run in debug mode, and programs are typically built in debug during development. It wouldn't make any difference for the published CLI tool, though. |
|
Either way is fine with me; that was an honest question. |
|
Gotcha. I'm leaning towards keeping it as a backport, although probably not for this upcoming release because of pending reviews. :) |
|
Successfully created backport PR for |
Following up on #4422, this replaces all raw array accesses with
array_get.array_getadds an assertion for OOB accesses only for debug builds, meaning this will add 0 overhead to release builds. This should be a great help in finding other instances of UB in the library.In adding these assertions, I found another OOB access in the query library from one of our existing Rust tests (
test_highlighting_cancellation).The Problem
If a query consists of multiple patterns, and some number of patterns had predicates associated with them at the start of the query, but later patterns did not have predicates, an OOB access would occur in
self-predicate_steps. This is due to the incorrect check here insidets_query_predicates_for_pattern:tree-sitter/lib/src/query.c
Lines 2981 to 2983 in b1d2b7c
This if condition will evaluate to true only if the query has no predicates at all, not if the given pattern doesn't have any predicates. This bug in theory wasn't observable from the Rust side, as the
step_countout parameter was still correctly set to 0. However, this could pose an issue for consumers of the C library if there was a query where some patterns at the start that had predicates, some number in the middle that didn't, and at least one at the end that did. Calls tots_query_predicates_for_patternfor those middle patterns with no predicates would incorrectly return the predicate steps for later patterns that did have predicates.The solution
Correct the early return to check if the slice's length is 0 for this particular pattern.
EDIT: Bug fix merged separately in #4440, CI fix taken out as it appears the failures were spurious