2015-07-09 15:29 GMT+03:00 Violeta Georgieva <miles...@gmail.com>: > Hi, > > 2015-07-09 14:50 GMT+03:00 Konstantin Kolinko <knst.koli...@gmail.com>: >> >> 2015-07-09 12:06 GMT+03:00 <violet...@apache.org>: >> > Author: violetagg >> > Date: Thu Jul 9 09:06:25 2015 >> > New Revision: 1690035 >> > >> > URL: http://svn.apache.org/r1690035 >> > Log: >> > Merged revisions 1690011, 1690021 from tomcat/trunk: >> > Fix possible resource leaks by closing streams properly. Issues > reported by Coverity Scan. >> > >> > Modified: >> > tomcat/tc7.0.x/trunk/ (props changed) >> > tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java >> > tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/ValidatorTask.java >> > > tomcat/tc7.0.x/trunk/java/org/apache/catalina/ha/deploy/FarmWarDeployer.java >> > > tomcat/tc7.0.x/trunk/java/org/apache/catalina/ha/deploy/LocalStrings.properties >> > tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml >> > >> > >> > Modified: > tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java >> > URL: > http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java?rev=1690035&r1=1690034&r2=1690035&view=diff >> > > ============================================================================== >> > --- tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java > (original) >> > +++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/ant/DeployTask.java > Thu Jul 9 09:06:25 2015 >> > @@ -199,6 +199,13 @@ public class DeployTask extends Abstract >> > sb.append(URLEncoder.encode(tag, getCharset())); >> > } >> > } catch (UnsupportedEncodingException e) { >> > + if (stream != null) { >> > + try { >> > + stream.close(); >> > + } catch (IOException ioe) { >> > + // Ignore >> > + } >> > + } >> > throw new BuildException("Invalid 'charset' attribute: " + > getCharset()); >> > } >> >> The above is OK (it fixes an obvious error of non closing the stream >> when BuildException is thrown), but I think that it would be slightly >> better to close the stream in a finally{}. I mean: >> >> a) move "execute(sb.toString(), stream, contentType, contentLength);" >> line that follows the above lines into main try/catch block >> b) close the stream in a finally{} >> >> The good side that it will take care if there is an unexpected > RuntimeException. >> >> It will change operation slightly that under normal conditions it will >> close the stream twice (once in execute() and second in finally()), >> but closing the same stream twice is allowed in Java. >> >> > http://docs.oracle.com/javase/6/docs/api/java/io/Closeable.html#close%28%29 >> "If the stream is already closed then invoking this method has no effect." >> > > I agree. > Feedback was applied. > > Thanks for the review, > Violeta >
Thank you! Reviewing DeployTask.java further (the code touched by http://svn.apache.org/r1689941 ) For current trunk: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/ant/DeployTask.java?view=markup#l156 1. Line 156 and following: There is "throw new UnsupportedOperationException("DeployTask does not support WAR files " + "greater than 2 Gb");" lines. As java.lang.UnsupportedOperationException is not an IOException, a catch(IOException) block is not visited,so 1) this error is not wrapped by BuildException 2) fsInput stream is not closed. The "stream" variable can be s/BufferedInputStream/InputStream/ with further simplification of stream closing code, as execute() method does not expect a "Buffered" one. 2. I wonder whether "2Gb" file upload size limit here is needed. Maybe we can s/int contentLength/long contentLength/ and allow such big files. There is a configurable file upload limit on Tomcat side (50Mb, in web.xml of manager webapp), though maybe it is not applied to PUT request used here. Best regards, Konstantin Kolinko --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
