TextView GetText() doesn't straddle the "Gap" properly

[Pete]

If a segment of text in a TextView is edited, and then a Gettext() is used on a selection that straddles the edited section, the returned string will be wrong.

The reason turns out to be that there is a missing term in a statement in TextGapBuffer.cpp::GetString(int32 offset, int32 length, char* buffer). I'm attaching a patch.

[Pete]

Fix for GetString error in TextGapBuffer.cpp

[waddlesplash]

(In #11132) Duplicate of #8447.

[waddlesplash]

Moving to A5 milestone and bumping priority as this is a really bad bug.

[stippi]

Patch looks like it makes a lot of sense, if fGapIndex is the start of the gap in fBuffer in Bytes and fGapCount is the number of Bytes that the gap is long. If any of them are in glyph offsets however (so not counting UTF-8 multi-byte chars), then the patch would be wrong (and the existing code broken in one more way).

[stippi]

Oh, and I don't know just looking from the patch if the calculation of "afterLen" is indeed correct. It would only be if "length" already accounted for fGapCount in the code above the patch.

[waddlesplash]

I would think that testing this patch solves the situation described in #11132 in a doc that has UTF8 chars would be enough.

[Pete]

Replying to stippi:

Patch looks like it makes a lot of sense, if fGapIndex is the start of the gap in fBuffer in Bytes and fGapCount is the number of Bytes that the gap is long. If any of them are in glyph offsets however (so not counting UTF-8 multi-byte chars), then the patch would be wrong (and the existing code broken in one more way).

Looking through the code again, as far as I can see, everything is always in bytes, so I think it should work in all situations.

Shamefully, I don't think I ever noticed the other GetText --just followed the failing call -- but now I see that it uses RealCharAt(...), which has exactly the logic in my patch, except that it's single chars.

[Pete]

Replying to stippi:

Oh, and I don't know just looking from the patch if the calculation of "afterLen" is indeed correct. It would only be if "length" already accounted for fGapCount in the code above the patch.

"length" doesn't have to know anything about the gap. It's checked against "Length()" to be clipped if necessary, but that too doesn't care about the gap -- it's just the total number of actual "Items" (bytes, again).

[jackburton]

It's been a while since I last touched that code, but it seems to me that "offset" should be added to the second parameter of the second call to memcpy(), like we do for the first. IOW:

    memcpy(buffer, fBuffer + offset, beforeLen); 
    memcpy(buffer + beforeLen, fBuffer + fGapIndex + fGapCount + offset, afterLen); 

Otherwise the code wouldn't work with offset != 0.

[Pete]

Replying to jackburton:

It's been a while since I last touched that code, but it seems to me that "offset" should be added to the second parameter of the second call to memcpy(), like we do for the first. IOW:

    memcpy(buffer, fBuffer + offset, beforeLen); 
    memcpy(buffer + beforeLen, fBuffer + fGapIndex + fGapCount + offset, afterLen); 

Otherwise the code wouldn't work with offset != 0.

No, because the offset is irrelevant for the second memcpy. The first one copies the segment of text from offset up to the gap (pointed to by fGapIndex); beforelen is calculated to do this. The second memcpy has to start immediately after the gap, which is pointed to by fGapIndex + fGapCount. Adding in offset as well would push things too far down.

FTR I ran a check earlier on a patched libbe.so, using Weaver and two connected StreamView elements. (That was where I noticed the problem in the first place, as it uses BTextView's 'GetText'.) I ran extensive edits in one StreamView -- including UTF-8 characters -- and transmitted arbitrary clips to the other. Seemed flawless under all conditions.

[jackburton]

Replying to Pete:

Replying to jackburton:

No, because the offset is irrelevant for the second memcpy. The first one copies the segment of text from offset up to the gap (pointed to by fGapIndex); beforelen is calculated to do this. The second memcpy has to start immediately after the gap, which is pointed to by fGapIndex + fGapCount. Adding in offset as well would push things too far down.

Yeah, you're right, indeed.

[jackburton]

Applied in hrev47878. Thank you!