helix icon indicating copy to clipboard operation
helix copied to clipboard

Crash when rendering certain SQL files

Open paholg opened this issue 1 year ago • 7 comments

Summary

Helix crashes with a "buffer overflow detected" when rendering certain SQL files. This happens on the latest master, but was not an issue in the previous version of helix I was using, which is a custom branch based off of commit c0fd8bc61b4c1611a48312938aaf0e3121f393b1 from September 6th.

Reproduction Steps

I created a directory with the following sql file:

CREATE OR REPLACE FUNCTION trigger_updated_at ()
    RETURNS TRIGGER
    AS $$
BEGIN
    NEW.updated_at = NOW();
    RETURN NEW;
END;
$$
LANGUAGE plpgsql;

I opened hx in this directory, and opened the file picker. As soon as that sql file was highlighted, helix crashed: 2024-02-06-073046-s

Helix log

No log entries are created.

Platform

Linux ubuntu 6.2.0-39-generic #40~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu Nov 16 10:53:04 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Terminal Emulator

alacritty 0.13.1

Installation Method

compiled nix flake from master

Helix Version

helix 23.10 -- commit 5c567f31e236134c0e27dc689c8ad07ef5f9b5d1

paholg avatar Feb 06 '24 15:02 paholg

This looks like an issue with the grammar. The SQL grammar has caused problems before. You would need to report this upstream to get that fixed (or maybe just updating the grammar could be enough)

pascalkuthe avatar Feb 06 '24 18:02 pascalkuthe

I couldn't reproduce it with Helix master.... mmm.

Regardless, I think we should update to https://github.com/DerekStride/tree-sitter-sql/releases/tag/v0.2.0 as there has been a lot of activity since the last Oct 4th Helix version, that was a pre-release version, first release version was v0.1.0 Nov 17, 2023.

@ds-cbo Do you feel like updating tree-sitter-sql again?

David-Else avatar Feb 06 '24 19:02 David-Else

Hm, I just tried again with an updated master, and it still crashes. I'd be happy to do anything else if it helps reproduce or figure out what's going wrong.

But if you just want to update tree-sitter-sql, I'd be happy to just try again when that's done.

paholg avatar Feb 06 '24 20:02 paholg

I can reproduce this and updating to the latest tree-sitter-sql revision fixes it

the-mikedavis avatar Feb 07 '24 02:02 the-mikedavis

@David-Else I'll gladly bump it again if that's still necessary

ds-cbo avatar Feb 13 '24 10:02 ds-cbo

@ds-cbo Awesome! It seems the bump is needed to avoid this crash. I am afraid I don't have the knowledge needed for making any new new queries if they are required.

David-Else avatar Feb 13 '24 11:02 David-Else

I've found some oddities upstream while bumping: https://github.com/DerekStride/tree-sitter-sql/pull/239

Play stay tuned while I resolve those first, otherwise I might be bumping again in a few days 😛

ds-cbo avatar Feb 14 '24 16:02 ds-cbo