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

Iso88591StringBuilder.append seems to have a logic error #10397

Closed
cquezel opened this issue Aug 24, 2023 · 1 comment · Fixed by #10399
Closed

Iso88591StringBuilder.append seems to have a logic error #10397

cquezel opened this issue Aug 24, 2023 · 1 comment · Fixed by #10399
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@cquezel
Copy link

cquezel commented Aug 24, 2023

Jetty version(s)
10.0.5, 11.0.5, 12.0.0

Jetty Environment
all

Java version/vendor
N/A

OS type/version
N/A

Description
class: org.eclipse.jetty.util.CharsetStringBuilder.Iso88591StringBuilder
method: public void append(CharSequence chars, int offset, int length)

The Iso88591StringBuilder.append method:

    @Override
    public void append(CharSequence chars, int offset, int length)
    {
        _builder.append(chars, offset, length);
    }

calls _builder.append(chars, offset, length) where _builder is a java java.lang.StringBuilder.

The formal parameters to StringBuilder.append are:

 * @param   s the sequence to append.
 * @param   start   the starting index of the subsequence to be appended.
 * @param   end     the end index of the subsequence to be appended.

Note that the end parameter is not correct. It expects offset + length instead of length.
This creates an java.lang.IndexOutOfBoundsException: start 109, end 0, length 241 (for example)

So the Iso88591StringBuilder.append method should be fixed to:

_builder.append(chars, offset, offset + length);

This is called by
class: org.eclipse.jetty.util.UrlEncoded
method: public static String decodeString(String encoded, int offset, int length, Charset charset)
line: 800 (and many others)

                    buffer = CharsetStringBuilder.forCharset(charset); // line 799
                    buffer.append(encoded, offset, i); // line 800

Line 799 instantiates a Iso88591StringBuilder.
Line 800 calls buffer.append

The actual parameters to buffer.append are:
encoded: the string
offset: start position in encoded
i: the character index after offset (i.e. 0, 1, 2 ...)

How to reproduce?

Any url encoded request using Iso88591

@cquezel cquezel added the Bug For general bugs on Jetty side label Aug 24, 2023
@cquezel cquezel changed the title UrlEncoded.decodeString seems to have a logic error Iso88591StringBuilderappend seems to have a logic error Aug 24, 2023
@cquezel cquezel changed the title Iso88591StringBuilderappend seems to have a logic error Iso88591StringBuilder.append seems to have a logic error Aug 24, 2023
@gregw gregw self-assigned this Aug 25, 2023
@gregw
Copy link
Contributor

gregw commented Aug 25, 2023

Thanks for the report. I have reproduced and it is indeed a bug that is actually all the way back from jetty 10. Fix coming.

gregw added a commit that referenced this issue Aug 25, 2023
Fix #10397 CharsetStringBuilder needs to convert length parameter to an end index.

Signed-off-by: gregw <gregw@webtide.com>
gregw added a commit that referenced this issue Aug 25, 2023
Fix #10397 CharsetStringBuilder needs to convert length parameter to an end index.

Signed-off-by: gregw <gregw@webtide.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants