You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by pb...@apache.org on 2007/07/04 17:27:09 UTC
svn commit: r553240 -
/struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
Author: pbenedict
Date: Wed Jul 4 08:27:07 2007
New Revision: 553240
URL: http://svn.apache.org/viewvc?view=rev&rev=553240
Log:
STR-2700: Clear input stream on aborted upload
Modified:
struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
Modified: struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
URL: http://svn.apache.org/viewvc/struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java?view=diff&rev=553240&r1=553239&r2=553240
==============================================================================
--- struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java (original)
+++ struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java Wed Jul 4 08:27:07 2007
@@ -33,6 +33,7 @@
import javax.servlet.ServletContext;
import javax.servlet.ServletException;
+import javax.servlet.ServletInputStream;
import javax.servlet.http.HttpServletRequest;
import java.io.File;
@@ -188,10 +189,11 @@
// Special handling for uploads that are too big.
request.setAttribute(MultipartRequestHandler.ATTRIBUTE_MAX_LENGTH_EXCEEDED,
Boolean.TRUE);
-
+ clearInputStream(request);
return;
} catch (FileUploadException e) {
log.error("Failed to parse multipart request", e);
+ clearInputStream(request);
throw new ServletException(e);
}
@@ -266,6 +268,23 @@
}
// -------------------------------------------------------- Support Methods
+
+ /**
+ * Finishes reading the input stream from an aborted upload. Fix for
+ * STR-2700 to prevent Window machines from hanging.
+ */
+ protected void clearInputStream(HttpServletRequest request) {
+ try {
+ ServletInputStream is = request.getInputStream();
+ byte[] data = new byte[DEFAULT_SIZE_THRESHOLD];
+ int bytesRead = 0;
+ do {
+ bytesRead = is.read(data);
+ } while (bytesRead > -1);
+ } catch (Exception e) {
+ log.error(e.getMessage(), e);
+ }
+ }
/**
* <p> Returns the maximum allowable size, in bytes, of an uploaded file.
Re: svn commit: r553240 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
Posted by Niall Pemberton <ni...@gmail.com>.
On 7/6/07, Paul Benedict <pb...@apache.org> wrote:
> Niall Pemberton wrote:
> > I see no discussion on FILEUPLOAD-140 with Jochen about this and that
> > would seem a more logical place to fix than here in Struts. If it has
> > merit then you should be able to convince him - or at least try. I'm
> > no expert on file upload or DoS, but my gut feel is its a hack to fix
> > a problem that has nothing to do with Struts - which we've generally
> > resisted in the past.
> >
> > Niall
> >
> You make a really good point. I thought about discussing this with him,
> but I wasn't quite sure he would care. It sounds like he might, based on
> your post, so I'll give it a shot.
He may not, but he has more of a clue about this stuff than me.
> It is true that the problem is not inside of Struts, but it's also true
> the request, when meeting the specific criteria, will hang indefinitely
> until the client's socket is terminated. Based on the ticket, it sounds
> like the user didn't find it in the example app but in his own
> development. Because this is likely to occur during normal development
> and isn't too-edgy of a use case, I found it important to fix. It can
> block development as well as production operations on a Windows box. The
> argument cuts both ways: would you prefer an indefinite blocking socket,
> or just completing the request with perhaps a large no-op upload? I
> believe the former is less ideal and the latter less likely to occur.
I have no clue as to the problem or solution - which is one reason I
was hoping you would discuss with Jochen. If you can't convince Jochen
its a good idea, then IMO its probably not a good idea for Struts
either - if you can convince him, then he'll change FileUpload and we
can remove it from Struts!
Niall
> Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r553240 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
Posted by Paul Benedict <pb...@apache.org>.
Niall Pemberton wrote:
> I see no discussion on FILEUPLOAD-140 with Jochen about this and that
> would seem a more logical place to fix than here in Struts. If it has
> merit then you should be able to convince him - or at least try. I'm
> no expert on file upload or DoS, but my gut feel is its a hack to fix
> a problem that has nothing to do with Struts - which we've generally
> resisted in the past.
>
> Niall
>
You make a really good point. I thought about discussing this with him,
but I wasn't quite sure he would care. It sounds like he might, based on
your post, so I'll give it a shot.
It is true that the problem is not inside of Struts, but it's also true
the request, when meeting the specific criteria, will hang indefinitely
until the client's socket is terminated. Based on the ticket, it sounds
like the user didn't find it in the example app but in his own
development. Because this is likely to occur during normal development
and isn't too-edgy of a use case, I found it important to fix. It can
block development as well as production operations on a Windows box. The
argument cuts both ways: would you prefer an indefinite blocking socket,
or just completing the request with perhaps a large no-op upload? I
believe the former is less ideal and the latter less likely to occur.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r553240 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
Posted by Niall Pemberton <ni...@gmail.com>.
On 7/6/07, Paul Benedict <pb...@apache.org> wrote:
> In the ticket, you will see my concerns were equal to his. But then I
> changed my mind because the concerned is founded on the false principle
> that fileuploads are somehow special. There is no difference between a
> large file upload and a long text string to Struts. You could send a
> large 2GB string to the validator and that could be considered a DoS --
> however, no one believes so. The problem is not with the solution, but
> with one's server configuration. All modern servers can be configured
> for a maximum packet size, so if there is a DoS, it's not in any one
> particular data point (text or file) in Struts, but with the server
> setting itself.
I see no discussion on FILEUPLOAD-140 with Jochen about this and that
would seem a more logical place to fix than here in Struts. If it has
merit then you should be able to convince him - or at least try. I'm
no expert on file upload or DoS, but my gut feel is its a hack to fix
a problem that has nothing to do with Struts - which we've generally
resisted in the past.
Niall
> Paul
>
> Niall Pemberton wrote:
> > I assume this is related to FILEUPLOAD-140[1] - Jochen points out on
> > that ticket that this could be used for a DOS attack - so this change
> > doesn't look like a good idea.
> >
> > Niall
> >
> > [1] https://issues.apache.org/jira/browse/FILEUPLOAD-140
> >
> > On 7/4/07, pbenedict@apache.org <pb...@apache.org> wrote:
> >> Author: pbenedict
> >> Date: Wed Jul 4 08:27:07 2007
> >> New Revision: 553240
> >>
> >> URL: http://svn.apache.org/viewvc?view=rev&rev=553240
> >> Log:
> >> STR-2700: Clear input stream on aborted upload
> >>
> >> Modified:
> >>
> >> struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
> >>
> >>
> >> Modified:
> >> struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
> >>
> >> URL:
> >> http://svn.apache.org/viewvc/struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java?view=diff&rev=553240&r1=553239&r2=553240
> >>
> >> ==============================================================================
> >>
> >> ---
> >> struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
> >> (original)
> >> +++
> >> struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
> >> Wed Jul 4 08:27:07 2007
> >> @@ -33,6 +33,7 @@
> >>
> >> import javax.servlet.ServletContext;
> >> import javax.servlet.ServletException;
> >> +import javax.servlet.ServletInputStream;
> >> import javax.servlet.http.HttpServletRequest;
> >>
> >> import java.io.File;
> >> @@ -188,10 +189,11 @@
> >> // Special handling for uploads that are too big.
> >>
> >> request.setAttribute(MultipartRequestHandler.ATTRIBUTE_MAX_LENGTH_EXCEEDED,
> >>
> >> Boolean.TRUE);
> >> -
> >> + clearInputStream(request);
> >> return;
> >> } catch (FileUploadException e) {
> >> log.error("Failed to parse multipart request", e);
> >> + clearInputStream(request);
> >> throw new ServletException(e);
> >> }
> >>
> >> @@ -266,6 +268,23 @@
> >> }
> >>
> >> // --------------------------------------------------------
> >> Support Methods
> >> +
> >> + /**
> >> + * Finishes reading the input stream from an aborted upload. Fix
> >> for
> >> + * STR-2700 to prevent Window machines from hanging.
> >> + */
> >> + protected void clearInputStream(HttpServletRequest request) {
> >> + try {
> >> + ServletInputStream is = request.getInputStream();
> >> + byte[] data = new byte[DEFAULT_SIZE_THRESHOLD];
> >> + int bytesRead = 0;
> >> + do {
> >> + bytesRead = is.read(data);
> >> + } while (bytesRead > -1);
> >> + } catch (Exception e) {
> >> + log.error(e.getMessage(), e);
> >> + }
> >> + }
> >>
> >> /**
> >> * <p> Returns the maximum allowable size, in bytes, of an
> >> uploaded file.
> >>
> >>
> >>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> > For additional commands, e-mail: dev-help@struts.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r553240 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
Posted by Paul Benedict <pb...@apache.org>.
In the ticket, you will see my concerns were equal to his. But then I
changed my mind because the concerned is founded on the false principle
that fileuploads are somehow special. There is no difference between a
large file upload and a long text string to Struts. You could send a
large 2GB string to the validator and that could be considered a DoS --
however, no one believes so. The problem is not with the solution, but
with one's server configuration. All modern servers can be configured
for a maximum packet size, so if there is a DoS, it's not in any one
particular data point (text or file) in Struts, but with the server
setting itself.
Paul
Niall Pemberton wrote:
> I assume this is related to FILEUPLOAD-140[1] - Jochen points out on
> that ticket that this could be used for a DOS attack - so this change
> doesn't look like a good idea.
>
> Niall
>
> [1] https://issues.apache.org/jira/browse/FILEUPLOAD-140
>
> On 7/4/07, pbenedict@apache.org <pb...@apache.org> wrote:
>> Author: pbenedict
>> Date: Wed Jul 4 08:27:07 2007
>> New Revision: 553240
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=553240
>> Log:
>> STR-2700: Clear input stream on aborted upload
>>
>> Modified:
>>
>> struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
>>
>>
>> Modified:
>> struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
>>
>> URL:
>> http://svn.apache.org/viewvc/struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java?view=diff&rev=553240&r1=553239&r2=553240
>>
>> ==============================================================================
>>
>> ---
>> struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
>> (original)
>> +++
>> struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
>> Wed Jul 4 08:27:07 2007
>> @@ -33,6 +33,7 @@
>>
>> import javax.servlet.ServletContext;
>> import javax.servlet.ServletException;
>> +import javax.servlet.ServletInputStream;
>> import javax.servlet.http.HttpServletRequest;
>>
>> import java.io.File;
>> @@ -188,10 +189,11 @@
>> // Special handling for uploads that are too big.
>>
>> request.setAttribute(MultipartRequestHandler.ATTRIBUTE_MAX_LENGTH_EXCEEDED,
>>
>> Boolean.TRUE);
>> -
>> + clearInputStream(request);
>> return;
>> } catch (FileUploadException e) {
>> log.error("Failed to parse multipart request", e);
>> + clearInputStream(request);
>> throw new ServletException(e);
>> }
>>
>> @@ -266,6 +268,23 @@
>> }
>>
>> // --------------------------------------------------------
>> Support Methods
>> +
>> + /**
>> + * Finishes reading the input stream from an aborted upload. Fix
>> for
>> + * STR-2700 to prevent Window machines from hanging.
>> + */
>> + protected void clearInputStream(HttpServletRequest request) {
>> + try {
>> + ServletInputStream is = request.getInputStream();
>> + byte[] data = new byte[DEFAULT_SIZE_THRESHOLD];
>> + int bytesRead = 0;
>> + do {
>> + bytesRead = is.read(data);
>> + } while (bytesRead > -1);
>> + } catch (Exception e) {
>> + log.error(e.getMessage(), e);
>> + }
>> + }
>>
>> /**
>> * <p> Returns the maximum allowable size, in bytes, of an
>> uploaded file.
>>
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: svn commit: r553240 - /struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
Posted by Niall Pemberton <ni...@gmail.com>.
I assume this is related to FILEUPLOAD-140[1] - Jochen points out on
that ticket that this could be used for a DOS attack - so this change
doesn't look like a good idea.
Niall
[1] https://issues.apache.org/jira/browse/FILEUPLOAD-140
On 7/4/07, pbenedict@apache.org <pb...@apache.org> wrote:
> Author: pbenedict
> Date: Wed Jul 4 08:27:07 2007
> New Revision: 553240
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=553240
> Log:
> STR-2700: Clear input stream on aborted upload
>
> Modified:
> struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
>
> Modified: struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java
> URL: http://svn.apache.org/viewvc/struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java?view=diff&rev=553240&r1=553239&r2=553240
> ==============================================================================
> --- struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java (original)
> +++ struts/struts1/trunk/core/src/main/java/org/apache/struts/upload/CommonsMultipartRequestHandler.java Wed Jul 4 08:27:07 2007
> @@ -33,6 +33,7 @@
>
> import javax.servlet.ServletContext;
> import javax.servlet.ServletException;
> +import javax.servlet.ServletInputStream;
> import javax.servlet.http.HttpServletRequest;
>
> import java.io.File;
> @@ -188,10 +189,11 @@
> // Special handling for uploads that are too big.
> request.setAttribute(MultipartRequestHandler.ATTRIBUTE_MAX_LENGTH_EXCEEDED,
> Boolean.TRUE);
> -
> + clearInputStream(request);
> return;
> } catch (FileUploadException e) {
> log.error("Failed to parse multipart request", e);
> + clearInputStream(request);
> throw new ServletException(e);
> }
>
> @@ -266,6 +268,23 @@
> }
>
> // -------------------------------------------------------- Support Methods
> +
> + /**
> + * Finishes reading the input stream from an aborted upload. Fix for
> + * STR-2700 to prevent Window machines from hanging.
> + */
> + protected void clearInputStream(HttpServletRequest request) {
> + try {
> + ServletInputStream is = request.getInputStream();
> + byte[] data = new byte[DEFAULT_SIZE_THRESHOLD];
> + int bytesRead = 0;
> + do {
> + bytesRead = is.read(data);
> + } while (bytesRead > -1);
> + } catch (Exception e) {
> + log.error(e.getMessage(), e);
> + }
> + }
>
> /**
> * <p> Returns the maximum allowable size, in bytes, of an uploaded file.
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org