[Github-comments] [geany/geany] Crash after update to Scintilla 5.1.5 (Issue #3095)

[Jiří Techet]

After the update to Scintilla 5.1.5, I started getting memory corruption crashes when saving some files. (Sorry for not noticing this before, I thought the Scintilla update didn't contain any changes on the Geany side.) I may be wrong but I think the problem is in

https://github.com/geany/geany/pull/3046/commits/d7c985e47412f8a53c5a8202a0f8b669c4fd5e08

and misunderstanding if the following Scintilla API change

>When calling SCI_GETTEXT, SCI_GETSELTEXT, and SCI_GETCURLINE with a NULL buffer argument to discover the length that should be allocated, do not include the terminating NUL in the returned value. The value returned is 1 less than previous versions of Scintilla. Applications should allocate a buffer 1 more than this to accommodate the NUL. The wParam (length) argument to SCI_GETTEXT and SCI_GETCURLINE also omits the NUL. This is more consistent with other APIs.

This part

>When calling SCI_GETTEXT, SCI_GETSELTEXT, and SCI_GETCURLINE with a NULL buffer argument to discover the length that should be allocated, do not include the terminating NUL in the returned value...

> ...The wParam (length) argument to SCI_GETTEXT and SCI_GETCURLINE also omits the NUL.

says that that the returned __length__ doesn't include NUL, not that the returned string misses the NUL character. The rest of the change description says that we should allocate `+1` buffer to accommodate for that.

I believe we should revert the above patch and then go through all the uses of `SCI_GETTEXT`, `SCI_GETSELTEXT`, and `SCI_GETCURLINE` and corresponding `sci_` wrappers and update the code where needed (including plugins). I don't think it's a good idea to update the `sci_` wrappers alone to mirror previous behavior since the whole Scintilla API is part of Geany's API so this is a API change anyway and making `sci_` functions do something else than direct Scintilla calls is confusing.

Rant: I really don't understand why Neil made this change - even though it fixes some inconsistency in Scintilla API, this is a hard to spot backwards incompatible change and such things shouldn't be made in minor releases of a library.

@kugel- @eht16 @elextr What do you think?


-- 
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/issues/3095
You are receiving this because you are subscribed to this thread.

Message ID: <geany/geany/issues/3095 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20220111/6d5c21ba/attachment.htm>