#2308 Question about SCI_GETTEXT & friends API change in Scintilla 5.1.5

[Jiri Techet]

I'd like to ask about the recent change in Scintilla 5.1.5:

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.

We had some misnunderstanding in Geany what the change does and we can fix that quite easily, see

https://github.com/geany/geany/issues/3095

What I would rather want to ask is if this change was made taking all the consequences into account. This is quite a major API change which is rather hard to spot because evertything compiles as before but which leads to memory corruptions when not noticed and I'm not sure if such big changes should be made in point.point releases. For Geany it for instance breaks the API towards plugins because Scintilla is exposed to them. I understand this change from the API point of view where it makes things more consistent but IMO this is the kind of change that should come to say Scintilla 6, marked with bold letters and warnings to be noticed.

[Neil Hodgson]

This change was motivated by bug reports about additional NULs at the end of returned strings. It was proposed on the mailing list on November 10 and the only reply was positive. Much client code was generic over the string returning APIs and was already allocating return value + 1 with the terminating NUL appearing inside the value for the mentioned APIs.

If you have strong stability needs then the mailing list is the place to push back on changes. As I said at the time:

However, this could break applications that will no longer allocate enough memory to include the NUL terminator. Downstream projects should check their use of these APIs.

The change was also mentioned in the 5.1.5 release notes.

A different approach could have been implemented at that time. Reverting this now, 2 months after the change and a month after 5.1.5 was released, may cause bugs in applications that have been updated to match the change.

There is some confusion in the referenced issue: SCI_GETTEXT always terminates with a NUL.

case Message::GetText: {
        if (lParam == 0)
            return pdoc->Length();
        char *ptr = CharPtrFromSPtr(lParam);
        const Sci_Position len = std::min<Sci_Position>(wParam, pdoc->Length());
        pdoc->GetCharRange(ptr, 0, len);
        ptr[len] = '\0';
        return len;
    }

[Jiri Techet]

Well, if it's going to stay, we just have to deal with it. If it were for Geany itself, it wouldn't be such a problem but there are also plugins that have access to Scintilla. We can update the plugins in the geany-plugins repository by ourselves but we don't have any control over external plugins.

I was reporting this because we updated to Scintilla 5 from Scintilla 3 recently and this was totally problem-free and didn't require any changes in code despite the 4 years development of Scintilla. Now after updating from 5.1.4 to 5.1.5 it's suddenly necessary to update both Geany and plugins and it is something unexpected from a minor release.

[Neil Hodgson]

It's still negotiable but the argument that Geany shouldn't have been subjected to an unexpected change also applies to projects using 5.1.5.

For plugins, depending on Geany's implementation, you could intercept calls to the three APIs and add compatibility shims that match previous behaviour. Add extra APIs with the new behaviour (preferably with counted rather than NUL-terminated strings) and ask plugin authors to migrate over time.

If that would be too difficult inside Geany then you could implement a 'string return compatibility mode' in Scintilla that reproduces old behaviour. It may just a matter of adding or subtracting 0 or 1 in a bunch of places. Since the new behaviour has been in a release, I'd expect the default state for the compatibility flag would be off.