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, fschumac...@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
signature.asc
Description: OpenPGP digital signature
