tomcat-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rémy Maucherat <r...@apache.org>
Subject Re: Bug in org.apache.tomcat.util.http.fileupload.MultipartStream.ItemInputStream#read(byte[], int, int)
Date Tue, 03 Jul 2018 08:24:02 GMT
On Mon, Jul 2, 2018 at 5:14 PM Piotr Joński <p.jonski@pojo.pl> wrote:

> Hi, of course I use it together with multipart request.
> I have spring boot 2 + zuul on tomcat 8.5.31. And I cannot proxy traffic
> with multipart request due to that error.
> I know that available return the right number of bytes but later you have
> method makeAvailable() which tries to read more than allowed! Some greedy
> developer wrote that :)
> Please check unit tests which I added. The should explain you everything.
>

Hum, ok, but there's no multipart boundary in your test. Do you have an
example of multipart content that fails to be processed correctly ?
makeAvailable will never try to read beyond the boundary position.
Personally, I don't see it as a problem if there are exceptions trying to
process non multipart content, but this sort of cleaner error handling is
often added.


>
> Also here is example issue:
>
> https://stackoverflow.com/questions/3263809/apache-commons-file-upload-stream-ended-unexpectedly
> I saw a lot of them -- all unresolved.
>

Maybe, but this one is about Tomcat 6, quite a while ago.

Fileupload is a separate component. Of course, we do fix and update it as
needed.

Rémy


>
> On 2 July 2018 at 16:58, Rémy Maucherat <remm@apache.org> wrote:
>
> > On Mon, Jul 2, 2018 at 4:35 PM Piotr Joński <p.jonski@pojo.pl> wrote:
> >
> > > Java: openjdk version "1.8.0_163"
> > > OpenJDK Runtime Environment (Zulu 8.28.0.1-linux64) (build
> 1.8.0_163-b01)
> > > OpenJDK 64-Bit Server VM (Zulu 8.28.0.1-linux64) (build 25.163-b01,
> mixed
> > > mode)
> > >
> > > OS: Ubuntu 18.04 Linux local 4.15.0-23-generic #25-Ubuntu SMP Wed May
> 23
> > > 18:02:16 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
> > >
> > >
> > > The problem is that
> > >
> > > org.apache.tomcat.util.http.fileupload.MultipartStream.
> > ItemInputStream#read(byte[],
> > > int, int) method does not update pos field after reading from buffer /
> > > stream.
> > >
> >
> > pos is set to the next separator which (IMO) should work fine since
> > available returns the right amount of bytes which are allowed to be read.
> > Doesn't this work for you ? This is not a generic utility class, it's
> > supposed to be used with multipart content, is it at least what you are
> > doing ?
> >
> > Rémy
> >
> >
> > >
> > > Unfortunately I cannot provide full example as this is private project.
> > >
> > > Here are sample unit tests. First reproduces the error and second use
> > > reflection to set proper field value to simulate proper behaviour:
> > >
> > > package org.apache.tomcat.util.http.fileupload;
> > >
> > > import org.junit.jupiter.api.Test;
> > >
> > > import java.io.ByteArrayInputStream;
> > > import java.io.IOException;
> > > import java.lang.reflect.Field;
> > >
> > > import static org.assertj.core.api.Assertions.assertThat;
> > >
> > > class ItemInputStreamTest {
> > >
> > >     @Test
> > >     void Should_Read_Bytes_But_Throws_Exception() throws IOException {
> > >         // given
> > >         byte[] bytes = new byte[]{1, 2, 3};
> > >         final ByteArrayInputStream inputStream = new
> > > ByteArrayInputStream(bytes);
> > >         final MultipartStream.ProgressNotifier progressNotifier = new
> > > MultipartStream.ProgressNotifier(null, 1111);
> > >         final MultipartStream multipartStream = new
> > > MultipartStream(inputStream,
> > >
> >  bytes,
> > >
> > > progressNotifier);
> > >         MultipartStream.ItemInputStream itemInputStream =
> > > multipartStream.new ItemInputStream();
> > >
> > >         // when
> > >         byte[] buffer = new byte[8196];
> > >         int result = itemInputStream.read(buffer, 0, 8196);
> > >
> > >         // then
> > >         assertThat(result).isEqualTo(3);
> > >     }
> > >
> > >     @Test
> > >     void Should_Read_Bytes_Fixed() throws IOException,
> > > NoSuchFieldException, IllegalAccessException {
> > >         // given
> > >         byte[] bytes = new byte[]{1, 2, 3};
> > >         final ByteArrayInputStream inputStream = new
> > > ByteArrayInputStream(bytes);
> > >         final MultipartStream.ProgressNotifier progressNotifier = new
> > > MultipartStream.ProgressNotifier(null, 1111);
> > >         final MultipartStream multipartStream = new
> > > MultipartStream(inputStream,
> > >
> >  bytes,
> > >
> > > progressNotifier);
> > >         MultipartStream.ItemInputStream itemInputStream =
> > > multipartStream.new ItemInputStream();
> > >
> > >         Field pos = itemInputStream.getClass()
> > >                                    .getDeclaredField("pos");
> > >         pos.setAccessible(true);
> > >         pos.set(itemInputStream, 3);
> > >
> > >         // when
> > >         byte[] buffer = new byte[8196];
> > >         int result = itemInputStream.read(buffer, 0, 8196);
> > >
> > >         // then
> > >         assertThat(result).isEqualTo(3);
> > >     }
> > > }
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message