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

TextBuffer contentsToWriter does not validate string length #934

Open
pjfanning opened this issue Mar 1, 2023 · 1 comment
Open

TextBuffer contentsToWriter does not validate string length #934

pjfanning opened this issue Mar 1, 2023 · 1 comment

Comments

@pjfanning
Copy link
Member

The StreamReadConstraints maxStringLen may kick in when the buffer size is updated but you can get scenarios where like with contentsAsString, you need to check at the end too. The maxStringLen is documented as approximate, so we could choose not to worry about this extra use case.

Ironically, since contentsToWriter already supports throwing IOException, this method is better suited to reporting exceptions that methods like contentsAsString.

@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 2, 2023

Ok good to note. It looks like it probably would make sense to validate it here.

I did also realize that we are actually not validating sizes on various resetXxx() calls for shared buffers, but we are waiting for accessor. If validation was done upfront, we could avoid checks for shared buffers later on. That probably won't have much performance impact tho.

However... thinking further, I think it should never be necessary to check lengths on contentAsString() since there must have been one call to "finish" contents before it gets called: one of:

  • setCurrentAndReturn()
  • finishCurrentSegment() (intermediate buffer fill, not necessarily last)
  • finishAndReturn() (relies on contentAsString() for its check)

so I think that checks should ideally be moved into only these 3, plus appends, resetXxx(), and removed from accessors.
There's still need to change signatures I suppose.

EDIT: above is incorrect as there were append() methods for which final check was needed. And trying to avoid check in contentsAsString() ended up not being all that useful in the end, so checks remain in there and other places.

Whether this issue should be tackled is different story; will leave open for possibly addressing in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants