|Summary:||org.apache.tomcat.util.ByteChunk throws NegativeArray SizeException|
|Product:||Tomcat 7||Reporter:||Dave Crighton <davecrighton>|
|Component:||Catalina||Assignee:||Tomcat Developers Mailing List <dev>|
|Priority:||P2||Keywords:||Beginner, PatchAvailable, PortForward|
|Attachments:||If growing the buffer would overflow int then just set to max value|
Description Dave Crighton 2018-01-12 13:04:33 UTC
Created attachment 35675 [details] If growing the buffer would overflow int then just set to max value We use ByteChunk to read in the request body and we noticed 2 things. 1.) The length of ByteChunk is limited to MAX_INT so for our purposes we can only process 2Gb requests before we need to alter our reading code. 2.) The way that ByteChunk grows means that in practice you can often only use up to 1Gb of space. The followign exception stack is thrown when append is called on and ByteChunk over 1Gb in size: java.lang.NegativeArraySizeException:java.lang.NegativeArraySizeException at org.apache.tomcat.util.buf.ByteChunk.makeSpace(ByteChunk.java:527) at org.apache.tomcat.util.buf.ByteChunk.append(ByteChunk.java:327) Looking at issue number 1 it looks like a lot of classes would need to be changed to use long but I do have a patch for a trivial check that allowed us to process 2Gb requests. Obviously this could silently truncate once you actually reach INT_MAX but it seems from the previous check based on the limit that this is the desired behaviour rather than to throw an exception.
Comment 1 Mark Thomas 2018-01-12 14:18:12 UTC
Thanks for the report. On closer inspection there appear to be a couple of other edge cases that could be handled better. I plan on putting together some unit tests to cover this case and the other edge cases. Note that CharChunk is likely to have similar issues.
Comment 2 Dave Crighton 2018-01-12 14:27:23 UTC
Thanks Mark, I did consider submitting my own JUnit with the patch but it causes large allocations which, at least for our own build systems, makes it unsuitable at an operational level for automated unit test (requires higher heap sizes, slows down the build etc). Wasn't sure what the best practice for the project is.
Comment 3 Mark Thomas 2018-01-15 21:45:42 UTC
Typically, we add tests like that with the @Ignore annotation so we can run them easily from the IDE but they aren't run by the CI systems. I've looked at this further over the weekend and there are subtle differences between ByteChunk and CharChunk that don't look right - as well as quite a lot of duplication.
Comment 4 Mark Thomas 2018-01-16 22:17:25 UTC
Fixed in: - trunk for 9.0.4 onwards - 8.5.x for 8.5.27 onwards - 8.0.x for 8.0.49 onwards - 7.0.x for 7.0.84 onwards Thanks for the patch - it is appreciated. It is just that with all the refactoring to reduce duplication and fix the various other edge cases I ended up taking a slightly different approach.
Comment 5 Dave Crighton 2018-01-17 10:59:43 UTC
Thanks Mark. We'll pick up the fix for our main product builds in 7.0.84.