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

neovim-unwrapped: fix byte index bounds checking #349230

Closed
wants to merge 1 commit into from

Conversation

eljamm
Copy link
Contributor

@eljamm eljamm commented Oct 17, 2024

Fix byte index encoding bounds, which were previously only checked against the UTF-8 length, whereas the default is UTF-16, which caused undefined behaviour.

See:

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Fix byte index encoding bounds, which were previously only checked
against the UTF-8 length, whereas the default is UTF-16, which caused
undefined behaviour.

See:
- neovim/neovim#30747
- nix-community/nixvim#2390
Copy link
Member

@teto teto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not patch neovim. It's easy enough to use nightly if you have the issue https://github.com/nix-community/neovim-nightly-overlay/

@eljamm
Copy link
Contributor Author

eljamm commented Oct 17, 2024

Should I close this, then? This is a regression with neovim 0.10.2.

@matejc
Copy link
Contributor

matejc commented Oct 17, 2024

Merged or not, this is beneficial at least for me since neovim-nightly-overlay is not functional atm ( nix-community/neovim-nightly-overlay#533 (comment) )

@teto
Copy link
Member

teto commented Oct 17, 2024

I prefer to see it closed. If the overlay is not working, you can still revert to 0.10.1, people can cherry-pick this as well.Patching software like that doesn't scale, also it makes upstream work more difficult.

@eljamm
Copy link
Contributor Author

eljamm commented Oct 17, 2024

Okay, I'll close this. If anyone wants to use the fix without having to install nightly, you can use the following overlay:

nixpkgs.overlays = [
  (final: prev: {
    neovim-unwrapped = prev.neovim-unwrapped.overrideAttrs (oldAttrs: {
      patches = oldAttrs.patches ++ [
        # Fix byte index encoding bounds.
        # - https://github.com/neovim/neovim/pull/30747
        # - https://github.com/nix-community/nixvim/issues/2390
        (pkgs.fetchpatch {
          name = "fix-lsp-str_byteindex_enc-bounds-checking-30747.patch";
          url = "https://patch-diff.githubusercontent.com/raw/neovim/neovim/pull/30747.patch";
          hash = "sha256-2oNHUQozXKrHvKxt7R07T9YRIIx8W3gt8cVHLm2gYhg=";
        })
      ];
    });
  })
];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants