Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail if Append()ing string to Rope in excess of TInlineString size #155

Merged

Conversation

sideshowbarker
Copy link
Contributor

Something like what’s in the patch in this PR would’ve prevented the problem introduced in #152 and reported in #153.

The problem is: the Append() method for Ropes allows passing in either a string or a pointer to a string. And in the case where a string is passed in, the runtime silently truncates the string if it exceeds a certain length (UTF8InlineSize, which in practice seems to be 15). But when instead a pointer to a string is passed in, it doesn’t matter how long the string is.

The intent of allowing strings to be passed to Append() seems to have been just for very short string constants. So we really shouldn’t use Append() with strings other than constants; we should instead always pass in pointers to strings.

So the initial patch in this PR causes Wattsi to fail with an error if the string passed in would end up being truncated, and emits a message suggesting to instead call Append() with a string pointer.

The patch is inelegant, and someone with better familiarity with Pascal could probably find a more-clever way. But as far as I know, in the Rope.Append(const NewString: RopeInternals.TInlineString) procedure itself, we can’t check if the length of the original string passed to Append() exceeds the TInlineString type size, UTF8InlineSize.

We can’t check there because the runtime truncates the original string to the TInlineString type size when Append() is called. So if the length of the original string exceeds the size of the TInlineString type — UTF8InlineSize — then the length of NewString will be the length of the truncated string (truncated down to UTF8InlineSize).

So this existing assert:

Assert(Length(NewString) <= RopeInternals.UTF8InlineSize, 'Maximum size of short string is ' + IntToStr(RopeInternals.UTF8InlineSize));

…will never work unless we allow the size of TInlineString to be greater than UTF8InlineSize (as the initial patch in this PR does) — because otherwise, NewString will always be less than or equal to UTF8InlineSize; that’s because, if NewString comes from a string whose length is greater than UTF8InlineSize (the size of the TInlineString type), the runtime will always truncate it down to UTF8InlineSize.

So the initial patch in this PR allows the size of the TInlineString type to be UTF8InlineSize + 1, but then fails/exits with an error message if Append() is called with a string bigger than UTF8InlineSize.

Because Append() is the only place where TInlineString is ever used in the existing code, this seems safe.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@domenic domenic merged commit 0ad4342 into main Aug 8, 2023
1 check passed
@domenic domenic deleted the sideshowbarker/rope-Append-fail-if-TInlineString-size-exceeded branch August 8, 2023 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants