#1480 Invalid memory access upon drop from DnD

[Colomban Wendling]

Scintilla 3.3.2 suffers from a memory access bug upon DnD. When moving a potion of text with DnD, 1 invalid byte is read. 3.3.1 works fine.

My first researches makes me believe that it's because the recent move to std::string (and precisely d3644b41bee8, from a bisect) changed the length passed to SelectionText::Copy() by one, missing the '\0' on some calls. That is, allocating one more byte for SelectionText::s and manually adding it after the copy[1] seems to fix the issue.

I'm not sure what the right/preferred fix is -- either fixing the calls or making SelectionText::Copy() take care of the terminator.

Attached is the Valgrind output right after a drop.

[1] BTW, why using a manual loop instead of memcpy()? memcpy() is probably faster but on very tiny strings.

[Neil Hodgson]

Potential fix committed as [06f36a] which tries to ensure there is always a \0 in non-empty SelectionText instances. DropAt should really take a length argument so that it can be used for text that includes \0.

Functions aren't always in the standard headers so are mostly avoided when implementations are in a header. These days I'd prefer to use std::copy.

[Colomban Wendling]

Potential fix committed as [06f36a] which tries to ensure there is always a \0 in non-empty SelectionText instances.

Looks like it indeed fixes the issue, and Valgrind is now happy upon drop, thanks!

Functions aren't always in the standard headers so are mostly avoided when implementations are in a header.

I'm not sure I understand what you mean here. For memcpy() one only need to include string.h, and the header should do it if it uses it, but I don't see how it could be a problem?

These days I'd prefer to use std::copy.

I'll probably say something stupid, but why not simply make SelectionText use std::string or something? IIUC it'd handle all current use case, as well as managing the memory and taking care of the terminator for C strings, while still supporting embedded NUL bytes. Or maybe this would break some API?

[Neil Hodgson]

memcpy should be in string.h but I recall either it or memmove being in a different header with one compiler. For C++ you are really supposed to include cstring instead of string.h.

I'd like to switch to std::vector or std::string but didn't want to change too much at once. The empty case may be problematic and its hard to test since copy and cut are normally unavailable for empty selections. std::string does not necessarily have a \0 terminator: when c_str() is called, it may allocate a copy that has the \0.

There are some more changes in this area in Hg.