NUL-terminating APIs, was [scite] Serious problem with lua GetSelText function in Scite 5.1.5

"'Neil Hodgson' via scintilla-interest" <[email protected]> Wed, 10 Nov 2021 17:06:57 +1100

Newsgroups	gmane.comp.lib.scintilla.devel,gmane.editors.scite.general
Message-ID	<[email protected]>

Paolo Gotti:

> I'm sorry to bother you, but the behaviour of lua GetSelText function has changed.
> The returned string now always include the terminating 0.

   In previous versions of SciTE, string results from Scintilla API calls that included NUL bytes were truncated early at the NUL. A change was made to pass the whole string including any NUL bytes.

   There have been some inconsistencies in the way that string returning APIs were handled by Scintilla, particularly SCI_GETTEXT, SCI_GETSELTEXT, and SCI_GETCURLINE. Most APIs return the length of the string excluding any NUL byte terminator but the mentioned APIs returned one more, including the NUL.

   It is likely best to change these 3 APIs to be consistent with others. 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 normal technique is to call the Scintilla API with NULL as the string argument which returns the length. Then allocate a string of length+1 bytes (the +1 may be set to NUL). Then call the API with this allocated string. Use the resulting string in a way that ignores the trailing NUL. If using the C++ std::string type, an extra NUL byte is automatically allocated at the end which is not counted in std::string::length() so works without problems.

   The attached LengthFix.patch changes Scintilla to return the string length for the 3 above mentioned APIs.

   The attached LengthFixSciTE.patch changes the Lua Scintilla interface to return an empty string instead of nil when the API returns an empty string. This was previously hidden by the +1 on the length.

> As a consequence, even no selection at all gets length=1.
> Many scripts of mine are broken, but I have already reverted to the previous version.
> Nonetheless,  somebody could get strange results.

   The above changes modify behaviour too much to be pushed in a quick update, so users should either strip terminating NULs off API results or return to the previous SciTE release for now.

   Neil

-- 
You received this message because you are subscribed to the Google Groups "scintilla-interest" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/scintilla-interest/123D6F83-B0DC-4B87-B10A-8ACE206BB6D9%40me.com.

LengthFix.patch (application/octet-stream, 1.9 KB)

diff -r 2a700fc24c59 src/Editor.cxx
--- a/src/Editor.cxx	Wed Nov 10 09:52:11 2021 +1100
+++ b/src/Editor.cxx	Wed Nov 10 16:28:49 2021 +1100
@@ -5888,7 +5888,7 @@
 
 	case Message::GetText: {
 			if (lParam == 0)
-				return pdoc->Length() + 1;
+				return pdoc->Length();
 			if (wParam == 0)
 				return 0;
 			char *ptr = CharPtrFromSPtr(lParam);
@@ -6034,16 +6034,14 @@
 			SelectionText selectedText;
 			CopySelectionRange(&selectedText);
 			if (lParam == 0) {
-				return selectedText.LengthWithTerminator();
+				return selectedText.Length();
 			} else {
 				char *ptr = CharPtrFromSPtr(lParam);
 				size_t iChar = selectedText.Length();
 				if (iChar) {
 					memcpy(ptr, selectedText.Data(), iChar);
-					ptr[iChar++] = '\0';
-				} else {
-					ptr[0] = '\0';
 				}
+				ptr[iChar] = '\0';
 				return iChar;
 			}
 		}
@@ -6554,11 +6552,10 @@
 			const Sci::Position lineStart = pdoc->LineStart(lineCurrentPos);
 			const Sci::Position lineEnd = pdoc->LineStart(lineCurrentPos + 1);
 			if (lParam == 0) {
-				return 1 + lineEnd - lineStart;
+				return lineEnd - lineStart;
 			}
-			PLATFORM_ASSERT(wParam > 0);
 			char *ptr = CharPtrFromSPtr(lParam);
-			const Sci::Position len = std::min<uptr_t>(lineEnd - lineStart, wParam - 1);
+			const Sci::Position len = std::min<uptr_t>(lineEnd - lineStart, wParam);
 			pdoc->GetCharRange(ptr, lineStart, len);
 			ptr[len] = '\0';
 			return sel.MainCaret() - lineStart;
diff -r 2a700fc24c59 test/simpleTests.py
--- a/test/simpleTests.py	Wed Nov 10 09:52:11 2021 +1100
+++ b/test/simpleTests.py	Wed Nov 10 16:28:49 2021 +1100
@@ -128,7 +128,7 @@
 		self.assertEquals(self.ed.SelectionStart, 1)
 		self.assertEquals(self.ed.SelectionEnd, 3)
 		result = self.ed.GetSelText(0)
-		self.assertEquals(result, b"bc\0")
+		self.assertEquals(result, b"bc")
 		self.ed.ReplaceSel(0, b"1234")
 		self.assertEquals(self.ed.Length, 6)
 		self.assertEquals(self.ed.Contents(), b"a1234d")

LengthFixSciTE.patch (application/octet-stream, 793 B)

diff -r d55658ae076a src/LuaExtension.cxx
--- a/src/LuaExtension.cxx	Mon Nov 08 14:36:16 2021 +1100
+++ b/src/LuaExtension.cxx	Wed Nov 10 16:28:58 2021 +1100
@@ -921,14 +921,8 @@
 
 	if (needStringResult) {
 		const intptr_t stringResultLen = host->Send(p, static_cast<SA::Message>(func.value), params[0], 0);
-		if (stringResultLen > 0) {
-			stringResult.assign(stringResultLen, '\0');
-			params[1] = SptrFromPointer(&stringResult[0]);
-		} else {
-			// Is this an error?  Are there any cases where it's not an error,
-			// and where the right thing to do is just return a blank string?
-			return 0;
-		}
+		stringResult.assign(stringResultLen, '\0');
+		params[1] = SptrFromPointer(stringResult.data());
 		if (func.paramType[0] == iface_length) {
 			params[0] = stringResultLen;
 		}