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

Some ArrayTrie methods throw StackOverflowError when cointaining a very large entry #7517

Closed
lorban opened this issue Feb 2, 2022 · 5 comments · Fixed by #7528 or #7533
Closed

Some ArrayTrie methods throw StackOverflowError when cointaining a very large entry #7517

lorban opened this issue Feb 2, 2022 · 5 comments · Fixed by #7528 or #7533
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@lorban
Copy link
Contributor

lorban commented Feb 2, 2022

Jetty version(s)
9.4.x

Description
keySet() and toString() recurse too deep on large entries, so they're likely to throw StackOverflowError.

Consider the following:

Trie<String> trie = new ArrayTrie<>(Character.MAX_VALUE - 1);

char[] c1 = new char[Character.MAX_VALUE - 1];
Arrays.fill(c1, 'a');
String huge = new String(c1);
assertTrue(trie.put(huge, "wow"));
assertThat(trie.get(huge), is("wow"));

assertThat(trie.keySet(), notNullValue());   // <--- throws StackOverflowError
assertThat(trie.toString(), notNullValue()); // <--- throws StackOverflowError
@lorban lorban added the Bug For general bugs on Jetty side label Feb 2, 2022
@lorban lorban linked a pull request Feb 3, 2022 that will close this issue
gregw added a commit that referenced this issue Feb 7, 2022
* Fix #7518 Trie.getBest with empty Key (#7527)

Fix #7518 Trie.getBest with empty Key

 * Only increment current row if not recursing.
 * Fixed match ending with big char

Signed-off-by: Greg Wilkins <[email protected]>

* Jetty 9.4.x 7517 trie stack overflow (#7528)

Fix #7518 Trie stack overflows

* Avoid recursion where possible

Signed-off-by: Greg Wilkins <[email protected]>

* Added extra tests

Signed-off-by: Greg Wilkins <[email protected]>

* removed empty file

Signed-off-by: Greg Wilkins <[email protected]>
@joakime
Copy link
Contributor

joakime commented Feb 17, 2022

@lorban is this done/complete now?

@lorban lorban linked a pull request Feb 18, 2022 that will close this issue
@lorban
Copy link
Contributor Author

lorban commented Feb 18, 2022

@joakime yes, it's all done and merged across the necessary branches. Thanks for the heads up.

@lorban lorban closed this as completed Feb 18, 2022
@gregw
Copy link
Contributor

gregw commented Feb 18, 2022

@lorban did these changes make it into the jetty-12 branch? I think we need a process to track such issues?

@gregw gregw reopened this Feb 18, 2022
@lorban
Copy link
Contributor Author

lorban commented Feb 18, 2022

@gregw no, they did not get merged to 12.0.x and you're right that we're going to need a process to remind us that.

I'm going to merge that right away.

lorban added a commit that referenced this issue Feb 18, 2022
Signed-off-by: Ludovic Orban <[email protected]>
@lorban
Copy link
Contributor Author

lorban commented Feb 18, 2022

Merged to 12.0.x.

@lorban lorban closed this as completed Feb 18, 2022
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
3 participants