Skip to content

Still buffer overflow.#19411

Closed
mattn wants to merge 1 commit intovim:masterfrom
mattn:fix-netbeans
Closed

Still buffer overflow.#19411
mattn wants to merge 1 commit intovim:masterfrom
mattn:fix-netbeans

Conversation

@mattn
Copy link
Member

@mattn mattn commented Feb 15, 2026

@chrisbra I suspect this fix does not fully resolve the issue.

c5f312a#diff-012c27cfc933c3fb6030c48e84f44e5cd2706af0b1ac94631f6a5d31b13170b0R2322

  • keybuf[KEYBUFLEN] is a fixed-size buffer of 64 bytes (KEYBUFLEN = 64)
  • strlen(tok) returns the string length (NUL terminator not included)
  • i is the number of characters already written to keybuf

After copying, we need 1 additional byte for the NUL terminator (\0). And the original check strlen(tok) + i < 64 only checked up to 63 bytes.

Also use vim_strncpy
@chrisbra
Copy link
Member

chrisbra commented Feb 15, 2026

I am not sure I understand. Technically those two statements do exactly the same:

-if (strlen(tok) + i < KEYBUFLEN)
+if (strlen(tok) + i + 1 <= KEYBUFLEN)

I think the vim_strncpy() is an improvement however.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a remaining off-by-one buffer overflow risk in Vim’s NetBeans integration when building keybuf for specialKeys mappings, ensuring space for the terminating NUL byte.

Changes:

  • Adjusts the length check to account for the required NUL terminator (+ 1).
  • Replaces strcpy() with vim_strncpy() to enforce bounded copying into keybuf.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mattn
Copy link
Member Author

mattn commented Feb 15, 2026

Ooops, Sorry. It seems I must have been asleep...

@mattn mattn closed this Feb 15, 2026
@chrisbra
Copy link
Member

Well, do you think we should include the vim_strncpy() change anyhow?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants