-
Notifications
You must be signed in to change notification settings - Fork 642
Fix memory corruption in nk_str_insert_text_char when pasting UTF-8 text
#841
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
base: master
Are you sure you want to change the base?
Fix memory corruption in nk_str_insert_text_char when pasting UTF-8 text
#841
Conversation
|
@PavelSharp Not sure, if the code is right or not, but I just wanted to note few things: This is how pasting So this one bug seems fixed, however, I have some problems with your PR. 2025-10-19_17-40-13.mp4Note that those issues do not appear on master, only on PR branch. I'm running |
|
Hi, @sleeptightAnsiC! 1 Cursor position issue state->cursor += len;it should be: state->cursor += glyphs;2 Clipboard copy bug - this one is mind-blowing edit->clip.copy(edit->clip.userdata, text, end - begin);The backend must provide its own implementation of this function. text = nk_str_at_const(&edit->string, begin, &unicode, &glyph_len);
text2 = nk_str_at_const(&edit->string, end, &unicode, &glyph_len);
if (edit->clip.copy)
edit->clip.copy(edit->clip.userdata, text, text2 - text);After these changes, everything starts working properly, though this is still just a draft fix. |
|
@PavelSharp |
|
The issues that I mentioned in #841 (comment) does not occur after 3342ffc But a possibility that you used LLM to fix those worries me a lot... |
|
@sleeptightAnsiC |
|
The problem is that #841 (comment) fells like LLM read, found solution and responded, all on its own without human involved. I would rather read whatever you put into LLM than its pseudo-correct output.
It may save your time when writing this, but it wastes everyone's time when reading and validating it. @RobLoach Please, be very careful when merging any of this. |
| { | ||
| return nk_str_insert_text_utf8(str, pos, text, len); | ||
| nk_str_insert_at_rune(str, pos, text, len); | ||
| /* TODO. Most likely, each of these functions(nk_str_insert_*, nk_str_append_*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a TODO here. Planning on tackling it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good evening, @RobLoach and thanks for reviewing this PR.
The TODO suggests that these functions probably shouldn’t just return the length, but instead check for success before returning.
However, this is just a hypothesis, and some other functions might depend on the current behavior.
I don’t yet have enough evidence to change it, so I prefer to leave it as-is for now.
Besides, this is a very minor detail that would affect fewer than 0.01% of cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this relates to the problem of sparse documentation, and we can’t know for sure what the original developer intended.
In the future, we could try to figure it out through style, observation, and testing,
but for now I think it’s more important to focus on the critical bug that will give us a working clipboard.
|
@sleeptightAnsiC When working with complex code, some of us don't just ask an LLM a single question. They use it to help examine code, test ideas, and debug iteratively. Every choice is thoroughly validated. Basically, LLMs are used as assistants for summarizing and presenting information. They do not replace human reasoning, verification, or the rigorous search for root causes. The technical work behind these patches: analysis, testing, and verification is entirely done by me. Saying that an "LLM-generated style" implies a lack of human expertise seriously undervalues the time and professional skill dedicated to producing these issues and PRs.
Such a statement is counterproductive and does not reflect the spirit of open source software development. Instead of such actions, it would be much better to focus on reviewing the proposed PR. |

This pull request fixes an unpleasant bug in
nk_str_insert_text_charandnk_str_insert_str_char.nk_str_insert_text_charandnk_str_insert_str_charhas wrong implementation #840See the issue for more details and a minimal reproducible example.