test(point): regression test for Point row/column reference counting (#466) - #473
Closed
blarghmatey wants to merge 1 commit into
Closed
test(point): regression test for Point row/column reference counting (#466)#473blarghmatey wants to merge 1 commit into
blarghmatey wants to merge 1 commit into
Conversation
…sion tree-sitter#466 fixed the `Point.row` / `Point.column` getters to return a new reference instead of a borrowed one, but added no test. The borrowed reference left the underlying row/column int one refcount too low, prematurely freeing it and corrupting the allocator free list — surfacing later as a hard SIGSEGV when indexing real files (the crash only reproduces for non-immortal ints, i.e. line/column numbers past the small-int cache, so small fixtures never tripped it). Add a regression test that asserts each getter takes its own reference, using a node whose start point is (500, 500) so the values are outside the immortal-int range. The test fails cleanly on the pre-tree-sitter#466 binding and passes on the fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
|
Thanks but this isn't necessary. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a regression test for the
Point.row/Point.columnreference-counting bug fixed in #466. That PR corrected the getters but did not add a test, and the existing suite can't catch this class of bug (itsstart_pointassertions all use small, immortal-int values).Background
Before #466,
point_get_row/point_get_columnreturnedPyTuple_GetItem(self, i)— a borrowed reference — where a get/set getter must return a new reference. The row/columnintwas left one refcount too low, so it was freed prematurely and corrupted the pymalloc free list, eventually causing a hard SIGSEGV.It only reproduces for non-immortal ints — i.e. line/column numbers past CPython's small-int cache (>256) — which is why it surfaced when indexing real, large source files but never with small test fixtures. I ran into it downstream and tracked it to this getter (independently confirmed with AddressSanitizer: a wild
TSNodepointer ints_node_end_byte, one over-decref removed and the crash disappears). See #472 for the original report.The test
test_point_getters_return_new_referencesbuilds a node whose start point is(500, 500)(both values outside the immortal range) and asserts each getter increments the referent's refcount by exactly one. It measures refcounts via tuple subscripting (point[i], which is correctly owned) and asserts before the borrowed reference can do harm, so it fails cleanly on the pre-#466 binding rather than crashing.Verified:
master(post-fix(point): fix reference counting issues #466).AssertionError: 2 != 3 : Point.row must return a new reference…) when fix(point): fix reference counting issues #466'spoint.cchange is reverted — no segfault.61 passed, 20 subtests passed.The test is skipped on non-CPython implementations (no
sys.getrefcount).