Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

free(): invalid pointer crash on php_only parser #6573

Closed
przepompownia opened this issue May 4, 2024 · 13 comments
Closed

free(): invalid pointer crash on php_only parser #6573

przepompownia opened this issue May 4, 2024 · 13 comments
Labels
bug Something isn't working

Comments

@przepompownia
Copy link
Contributor

przepompownia commented May 4, 2024

Describe the bug

When using a parser from 4e21361 I get free(): invalid pointer crash on some file (details in the reproduction below).

I cannot reproduce this failure with parser library compiled from cloned https://github.com/tree-sitter/tree-sitter-php sources and copied in place of the mentioned above. That's why I reported it here.

To Reproduce

With init.lua:

local thisInitFile = debug.getinfo(1).source:match('@?(.*)')
local configDir = vim.fs.dirname(thisInitFile)
local xdgDir = vim.fs.joinpath(configDir, '.xdg')
vim.fn.mkdir(xdgDir, 'p')

vim.env['XDG_CONFIG_HOME'] = configDir
vim.env['XDG_DATA_HOME'] = vim.fs.joinpath(xdgDir, 'data')
vim.env['XDG_STATE_HOME'] = vim.fs.joinpath(xdgDir, 'state')
vim.env['XDG_CACHE_HOME'] = vim.fs.joinpath(xdgDir, 'cache')
local stdPathConfig = vim.fn.stdpath('config')

vim.opt.runtimepath:prepend(stdPathConfig)
vim.opt.packpath:prepend(stdPathConfig)

local function gitClone(url, installPath, branch)
  if vim.fn.isdirectory(installPath) ~= 0 then
    return
  end

  local command = {'git', 'clone', '--', url, installPath}
  if branch then
    table.insert(command, 3, '--branch')
    table.insert(command, 4, branch)
  end
  local sysObj = vim.system(command, {}):wait()
  if sysObj.code ~= 0 then
    error(sysObj.stderr)
  end
  vim.notify(sysObj.stdout)
  vim.notify(sysObj.stderr, vim.log.levels.WARN)
end

local pluginsPath = 'nvim/pack/plugins/opt'
vim.fn.mkdir(pluginsPath, 'p')
pluginsPath = vim.uv.fs_realpath(pluginsPath)

--- @type table<string, {url:string, branch: string?}>
local plugins = {
  ['nvim-treesitter'] = {url = 'https://github.com/nvim-treesitter/nvim-treesitter'},
}

for name, repo in pairs(plugins) do
  local installPath = vim.fs.joinpath(pluginsPath, name)
  gitClone(repo.url, installPath, repo.branch)
  vim.cmd.packadd({args = {name}, bang = true})
end
require 'nvim-treesitter.configs'.setup {
  ensure_installed = {
    'php_only',
  },
  highlight = {
    enable = true,
  },
  incremental_selection = {
    enable = true,
    keymaps = tsKeymaps,
  },
  indent = {
    enable = true,
  },
}

vim.treesitter.language.register('php_only', {'php'})

run

nvim --clean -u init.lua https://raw.githubusercontent.com/phpactor/phpactor/ac99b4d8831eb5a249a2b1788cbb5b64a4a1c61c/lib/Diff/Tests/RangesForDiffTest.php

For me it's enough to crash.

Expected behavior

No crash

Output of :checkhealth nvim-treesitter

nvim-treesitter: require("nvim-treesitter.health").check()

Installation
- OK tree-sitter found 0.20.8 (parser generator, only needed for :TSInstallFromGrammar)
- OK node found v18.20.1 (only needed for :TSInstallFromGrammar)
- OK git executable found.
- OK cc executable found. Selected from { vim.NIL, "cc", "gcc", "clang", "cl", "zig" }
  Version: cc (Debian 13.2.0-24) 13.2.0
- OK Neovim was compiled with tree-sitter runtime ABI version 14 (required >=13). Parsers must be compatible with runtime ABI.

OS Info:
{
  machine = "x86_64",
  release = "6.7.12-amd64",
  sysname = "Linux",
  version = "#1 SMP PREEMPT_DYNAMIC Debian 6.7.12-1 (2024-04-24)"
}

Parser/Features         H L F I J
  - bash                ✓ ✓ ✓ . ✓
  - c                   ✓ ✓ ✓ ✓ ✓
  - lua                 ✓ ✓ ✓ ✓ ✓
  - markdown            ✓ . ✓ ✓ ✓
  - markdown_inline     ✓ . . . ✓
  - php_only            ✓ ✓ ✓ ✓ ✓
  - python              ✓ ✓ ✓ ✓ ✓
  - query               ✓ ✓ ✓ ✓ ✓
  - vim                 ✓ ✓ ✓ . ✓
  - vimdoc              ✓ . . . ✓

  Legend: H[ighlight], L[ocals], F[olds], I[ndents], In[j]ections
         +) multiple parsers found, only one will be used
         x) errors found in the query, try to run :TSUpdate {lang}

Output of nvim --version

NVIM v0.10.0-dev-3078+g3a8265266
Build type: RelWithDebInfo
LuaJIT 2.1.1713484068

Additional context

  • libc6: 2.38-7
@przepompownia przepompownia added the bug Something isn't working label May 4, 2024
@clason
Copy link
Contributor

clason commented May 4, 2024

Then you should report that with that parser; there's nothing we can do about it here.

@clason clason closed this as not planned Won't fix, can't repro, duplicate, stale May 4, 2024
@przepompownia
Copy link
Contributor Author

przepompownia commented May 5, 2024

Not completely. Maybe it would be better to revert 4e21361 while the parser crashes on tree-sitter/tree-sitter-php@58054be.

On the tree-sitter-php side I still haven't investigated how to get the same parser binary as created with nvim-treesitter. It differs from the build by make -C php_only there. That's (at least) why I can't report a bug there for now.

@clason
Copy link
Contributor

clason commented May 5, 2024

You can check the code to see exactly how we build it. This needs to be fixed upstream; if you rely on the parser, just lock your nvim-treesitter version until then.

@przepompownia
Copy link
Contributor Author

Yet one note here (on Linux with gcc).

  1. nvim-treesitter builds the parser with
cc -o parser.so -I./src src/parser.c src/scanner.c -Os -shared -fPIC`
  1. make's output on the project shows another way of building the binary that doesn't crash:
    cc -Isrc -std=c11 -fPIC   -c -o src/parser.o src/parser.c
    cc -Isrc -std=c11 -fPIC   -c -o src/scanner.o src/scanner.c
    ar rv libtree-sitter-php_only.a src/parser.o src/scanner.o
    ar: creating libtree-sitter-php_only.a
    a - src/parser.o
    a - src/scanner.o
    cc  -shared -Wl,-soname,libtree-sitter-php_only.so.0 src/parser.o src/scanner.o  -o libtree-sitter-php_only.so
  1. The CLI seems to build its own parser binary (stored in ~/.cache folder) in an unknown way, which makes reproduction difficult (CLI: how tu use parser binary built earlier? tree-sitter/tree-sitter#3336).

Should we assume that the way used by nvim-treesitter is always correct?

@clason
Copy link
Contributor

clason commented May 5, 2024

I think they are functionally equivalent. You can test tree-sitter build -o php_only.so to see how that behaves. (Nvim-treesitter will switch to that in 1.0 anyway.)

@przepompownia
Copy link
Contributor Author

I think they are functionally equivalent.

For some reason, 1. produces crashing binary unlike 2.

tree-sitter build -o php_only.so

I'm confused: currently I have CLI written in Rust, from Debian package (0.20.8-5). It does not have build subcommand. I tried tree-sitter generate --build. It resulted with ~/.cache/tree-sitter/lib/php_only.so but without details about compiling.

@clason
Copy link
Contributor

clason commented May 5, 2024

Yeah, that's too old. Current is 0.22.5 (soon to be 6), with massive changes. That's what we target, exclusively.

@przepompownia
Copy link
Contributor Author

Indeed, in the new version build exists and builds the correct binary.

@liljaylj
Copy link

liljaylj commented May 6, 2024

I faced simlar issue. when trying to open some php files I get munmap_chunk(): invalid pointer. after bisecting and looking at tree-sitter-php's makefile and nvim-treesitter's source code, I found out that if we don't include compiler flag -Os (optimize for size), then error would not occur.

@clason
Copy link
Contributor

clason commented May 6, 2024

Please report that upstream; a valid compiler flag should not lead to a segfault. (Also, try a newer gcc, or clang.)

@liljaylj
Copy link

liljaylj commented May 6, 2024

@clason reported upstream.

I don't know if it will help, I found that error occurs on injections (and when compiled with -Os of course).

reproducer:

<?php

$query = <<<SQL
select * from users;
SQL;

@amaanq
Copy link
Member

amaanq commented May 14, 2024

Fixed in tree-sitter/tree-sitter-php@27afeb0

@przepompownia
Copy link
Contributor Author

Thank you all for your involvement. After fixing the parser built by nvim-treesitter works for me as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants