Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#8447 closed bug (fixed)

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

Reported by: Pete Owned by: axeld
Priority: critical Milestone: R1/beta1
Component: Kits/Interface Kit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking: #11132
Has a Patch: yes Platform: All

Description

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.

Attachments (1)

TextGapBuffer.diff (531 bytes) - added by Pete 7 years ago.
Fix for GetString error in TextGapBuffer.cpp

Download all attachments as: .zip

Change History (14)

Changed 7 years ago by Pete

Attachment: TextGapBuffer.diff added

Fix for GetString error in TextGapBuffer.cpp

comment:1 Changed 7 years ago by Pete

Has a Patch: set

comment:2 Changed 5 years ago by waddlesplash

Blocking: 11132 added

(In #11132) Duplicate of #8447.

comment:3 Changed 5 years ago by waddlesplash

Milestone: R1R1/alpha5
Priority: normalcritical
Version: R1/alpha3R1/Development

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

comment:4 Changed 5 years ago by 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).

comment:5 Changed 5 years ago by 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.

comment:6 Changed 5 years ago by waddlesplash

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

comment:7 in reply to:  4 Changed 5 years ago by 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 GetString --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.

Last edited 5 years ago by Pete (previous) (diff)

comment:8 in reply to:  5 Changed 5 years ago by 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).

comment:9 Changed 5 years ago by 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.

comment:10 in reply to:  9 ; Changed 5 years ago by 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.

comment:11 in reply to:  10 Changed 5 years ago by 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.

comment:12 Changed 5 years ago by jackburton

Resolution: fixed
Status: newclosed

Applied in hrev47878. Thank you!

comment:13 Changed 5 years ago by pulkomandy

Milestone: R1/alpha5R1/beta1
Note: See TracTickets for help on using tickets.