You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by fs...@apache.org on 2015/01/24 15:42:27 UTC
svn commit: r1654524 -
/tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
Author: fschumacher
Date: Sat Jan 24 14:42:27 2015
New Revision: 1654524
URL: http://svn.apache.org/r1654524
Log:
Close input and output streams in expandCGIScript to
avoid resource leaks. Issue reported by Coverity Scan.
Modified:
tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
Modified: tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java?rev=1654524&r1=1654523&r2=1654524&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java (original)
+++ tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java Sat Jan 24 14:42:27 2015
@@ -1133,6 +1133,10 @@ public final class CGIServlet extends Ht
File f = new File(destPath.toString());
if (f.exists()) {
+ try {
+ is.close();
+ } catch (IOException ignore) {
+ }
// Don't need to expand if it already exists
return;
}
@@ -1162,10 +1166,16 @@ public final class CGIServlet extends Ht
}
FileOutputStream fos = new FileOutputStream(f);
- // copy data
- IOTools.flow(is, fos);
- is.close();
- fos.close();
+ try {
+ // copy data
+ IOTools.flow(is, fos);
+ } finally {
+ try {
+ is.close();
+ } catch (IOException ignore) {
+ }
+ fos.close();
+ }
if (debug >= 2) {
log("expandCGIScript: expanded '" + srcPath + "' to '" + destPath + "'");
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1654524 - /tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
Posted by Felix Schumacher <fe...@internetallee.de>.
Am 26.01.2015 um 09:28 schrieb Mark Thomas:
> On 25/01/2015 22:57, Christopher Schultz wrote:
>> Felix,
>>
>> On 1/24/15 2:33 PM, Felix Schumacher wrote:
>>> Am 24.01.2015 um 17:13 schrieb Christopher Schultz:
>>>> Felix,
>>>>
>>>> On 1/24/15 9:42 AM, fschumacher@apache.org wrote:
>>>>> Author: fschumacher
>>>>> Date: Sat Jan 24 14:42:27 2015
>>>>> New Revision: 1654524
>>>>>
>>>>> URL: http://svn.apache.org/r1654524
>>>>> Log:
>>>>> Close input and output streams in expandCGIScript to
>>>>> avoid resource leaks. Issue reported by Coverity Scan.
>>>>>
>>>>> Modified:
>>>>> tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>>>>
>>>>> Modified: tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java?rev=1654524&r1=1654523&r2=1654524&view=diff
>>>>>
>>>>> ==============================================================================
>>>>>
>>>>> --- tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>>>> (original)
>>>>> +++ tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>>>> Sat Jan 24 14:42:27 2015
>>>>> @@ -1133,6 +1133,10 @@ public final class CGIServlet extends Ht
>>>>> File f = new File(destPath.toString());
>>>>> if (f.exists()) {
>>>>> + try {
>>>>> + is.close();
>>>>> + } catch (IOException ignore) {
>>>>> + }
>>>> Should this be logged? It should very rarely happen, but it would be
>>>> good to know if there was a problem (which might represent a resource
>>>> leak).
>>> I looked for other examples in the source code before and the first few
>>> examples I found where ignoring the exception while closing, too. So I
>>> thought it would be ok, do ignore this exception.
>>>
>>> If we don't want this exception ignored, at what level should the
>>> information be logged? I would go for debug or info.
>> I think I'm in a minority when it comes to wanting to log these
>> exceptions. I think it might even be appropriate to log them at the WARN
>> level. If they happen -- which should be very rare -- it could indicate
>> a serious problem with the system or the JVM.
> Ideally, the only time we would ignore exceptions is when we genuinely
> don't care. That could include:
> - something has already gone wrong (which we logged) and we are just
> trying to clean up the mess, exceptions are expected and aren't going to
> tell us anything useful
> - we know that the exception is impossible but we have to catch it
>
> I think the main thing is not to fill the logs with exception messages
> the user can do nothing about.
>
> In this case WARN seems reasonable.
I have added the logging of the exceptions.
Felix
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1654524 - /tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
Posted by Mark Thomas <ma...@apache.org>.
On 25/01/2015 22:57, Christopher Schultz wrote:
> Felix,
>
> On 1/24/15 2:33 PM, Felix Schumacher wrote:
>> Am 24.01.2015 um 17:13 schrieb Christopher Schultz:
>>> Felix,
>>>
>>> On 1/24/15 9:42 AM, fschumacher@apache.org wrote:
>>>> Author: fschumacher
>>>> Date: Sat Jan 24 14:42:27 2015
>>>> New Revision: 1654524
>>>>
>>>> URL: http://svn.apache.org/r1654524
>>>> Log:
>>>> Close input and output streams in expandCGIScript to
>>>> avoid resource leaks. Issue reported by Coverity Scan.
>>>>
>>>> Modified:
>>>> tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>>>
>>>> Modified: tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java?rev=1654524&r1=1654523&r2=1654524&view=diff
>>>>
>>>> ==============================================================================
>>>>
>>>> --- tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>>> (original)
>>>> +++ tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>>> Sat Jan 24 14:42:27 2015
>>>> @@ -1133,6 +1133,10 @@ public final class CGIServlet extends Ht
>>>> File f = new File(destPath.toString());
>>>> if (f.exists()) {
>>>> + try {
>>>> + is.close();
>>>> + } catch (IOException ignore) {
>>>> + }
>>> Should this be logged? It should very rarely happen, but it would be
>>> good to know if there was a problem (which might represent a resource
>>> leak).
>> I looked for other examples in the source code before and the first few
>> examples I found where ignoring the exception while closing, too. So I
>> thought it would be ok, do ignore this exception.
>>
>> If we don't want this exception ignored, at what level should the
>> information be logged? I would go for debug or info.
>
> I think I'm in a minority when it comes to wanting to log these
> exceptions. I think it might even be appropriate to log them at the WARN
> level. If they happen -- which should be very rare -- it could indicate
> a serious problem with the system or the JVM.
Ideally, the only time we would ignore exceptions is when we genuinely
don't care. That could include:
- something has already gone wrong (which we logged) and we are just
trying to clean up the mess, exceptions are expected and aren't going to
tell us anything useful
- we know that the exception is impossible but we have to catch it
I think the main thing is not to fill the logs with exception messages
the user can do nothing about.
In this case WARN seems reasonable.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1654524 -
/tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Felix,
On 1/24/15 2:33 PM, Felix Schumacher wrote:
> Am 24.01.2015 um 17:13 schrieb Christopher Schultz:
>> Felix,
>>
>> On 1/24/15 9:42 AM, fschumacher@apache.org wrote:
>>> Author: fschumacher
>>> Date: Sat Jan 24 14:42:27 2015
>>> New Revision: 1654524
>>>
>>> URL: http://svn.apache.org/r1654524
>>> Log:
>>> Close input and output streams in expandCGIScript to
>>> avoid resource leaks. Issue reported by Coverity Scan.
>>>
>>> Modified:
>>> tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>>
>>> Modified: tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>> URL:
>>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java?rev=1654524&r1=1654523&r2=1654524&view=diff
>>>
>>> ==============================================================================
>>>
>>> --- tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>> (original)
>>> +++ tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>> Sat Jan 24 14:42:27 2015
>>> @@ -1133,6 +1133,10 @@ public final class CGIServlet extends Ht
>>> File f = new File(destPath.toString());
>>> if (f.exists()) {
>>> + try {
>>> + is.close();
>>> + } catch (IOException ignore) {
>>> + }
>> Should this be logged? It should very rarely happen, but it would be
>> good to know if there was a problem (which might represent a resource
>> leak).
> I looked for other examples in the source code before and the first few
> examples I found where ignoring the exception while closing, too. So I
> thought it would be ok, do ignore this exception.
>
> If we don't want this exception ignored, at what level should the
> information be logged? I would go for debug or info.
I think I'm in a minority when it comes to wanting to log these
exceptions. I think it might even be appropriate to log them at the WARN
level. If they happen -- which should be very rare -- it could indicate
a serious problem with the system or the JVM.
>>
>>> // Don't need to expand if it already exists
>>> return;
>>> }
>>> @@ -1162,10 +1166,16 @@ public final class CGIServlet extends Ht
>>> }
>>> FileOutputStream fos = new FileOutputStream(f);
>>> - // copy data
>>> - IOTools.flow(is, fos);
>>> - is.close();
>>> - fos.close();
>>> + try {
>>> + // copy data
>>> + IOTools.flow(is, fos);
>>> + } finally {
>>> + try {
>>> + is.close();
>>> + } catch (IOException ignore) {
>>> + }
>> Same here.
>>
>>> + fos.close();
>> Why not just call fos.close() to close everything all at once?
> Do you mean is.close() and fos.close() inside the same try block? I
> separated them, so that a failed is.close() will not interfere with the
> close call on fos. I didn't put fos.close in a try catch block, since it
> was not in one before the fix.
Oh, I misread the patch. There is no question here about that: your
change is good. I do have the same concern about logging the
exception(s), though.
Thanks,
-chris
Re: svn commit: r1654524 - /tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
Posted by Felix Schumacher <fe...@internetallee.de>.
Am 24.01.2015 um 17:13 schrieb Christopher Schultz:
> Felix,
>
> On 1/24/15 9:42 AM, fschumacher@apache.org wrote:
>> Author: fschumacher
>> Date: Sat Jan 24 14:42:27 2015
>> New Revision: 1654524
>>
>> URL: http://svn.apache.org/r1654524
>> Log:
>> Close input and output streams in expandCGIScript to
>> avoid resource leaks. Issue reported by Coverity Scan.
>>
>> Modified:
>> tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>>
>> Modified: tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java?rev=1654524&r1=1654523&r2=1654524&view=diff
>> ==============================================================================
>> --- tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java (original)
>> +++ tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java Sat Jan 24 14:42:27 2015
>> @@ -1133,6 +1133,10 @@ public final class CGIServlet extends Ht
>>
>> File f = new File(destPath.toString());
>> if (f.exists()) {
>> + try {
>> + is.close();
>> + } catch (IOException ignore) {
>> + }
> Should this be logged? It should very rarely happen, but it would be
> good to know if there was a problem (which might represent a resource leak).
I looked for other examples in the source code before and the first few
examples I found where ignoring the exception while closing, too. So I
thought it would be ok, do ignore this exception.
If we don't want this exception ignored, at what level should the
information be logged? I would go for debug or info.
>
>> // Don't need to expand if it already exists
>> return;
>> }
>> @@ -1162,10 +1166,16 @@ public final class CGIServlet extends Ht
>> }
>> FileOutputStream fos = new FileOutputStream(f);
>>
>> - // copy data
>> - IOTools.flow(is, fos);
>> - is.close();
>> - fos.close();
>> + try {
>> + // copy data
>> + IOTools.flow(is, fos);
>> + } finally {
>> + try {
>> + is.close();
>> + } catch (IOException ignore) {
>> + }
> Same here.
>
>> + fos.close();
> Why not just call fos.close() to close everything all at once?
Do you mean is.close() and fos.close() inside the same try block? I
separated them, so that a failed is.close() will not interfere with the
close call on fos. I didn't put fos.close in a try catch block, since it
was not in one before the fix.
Felix
>
> Thanks,
> -chris
>
>> + }
>> if (debug >= 2) {
>> log("expandCGIScript: expanded '" + srcPath + "' to '" + destPath + "'");
>> }
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1654524 -
/tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Felix,
On 1/24/15 9:42 AM, fschumacher@apache.org wrote:
> Author: fschumacher
> Date: Sat Jan 24 14:42:27 2015
> New Revision: 1654524
>
> URL: http://svn.apache.org/r1654524
> Log:
> Close input and output streams in expandCGIScript to
> avoid resource leaks. Issue reported by Coverity Scan.
>
> Modified:
> tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
>
> Modified: tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java?rev=1654524&r1=1654523&r2=1654524&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/servlets/CGIServlet.java Sat Jan 24 14:42:27 2015
> @@ -1133,6 +1133,10 @@ public final class CGIServlet extends Ht
>
> File f = new File(destPath.toString());
> if (f.exists()) {
> + try {
> + is.close();
> + } catch (IOException ignore) {
> + }
Should this be logged? It should very rarely happen, but it would be
good to know if there was a problem (which might represent a resource leak).
> // Don't need to expand if it already exists
> return;
> }
> @@ -1162,10 +1166,16 @@ public final class CGIServlet extends Ht
> }
> FileOutputStream fos = new FileOutputStream(f);
>
> - // copy data
> - IOTools.flow(is, fos);
> - is.close();
> - fos.close();
> + try {
> + // copy data
> + IOTools.flow(is, fos);
> + } finally {
> + try {
> + is.close();
> + } catch (IOException ignore) {
> + }
Same here.
> + fos.close();
Why not just call fos.close() to close everything all at once?
Thanks,
-chris
> + }
> if (debug >= 2) {
> log("expandCGIScript: expanded '" + srcPath + "' to '" + destPath + "'");
> }
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>