You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2026/06/30 19:18:06 UTC
(tomcat) branch main updated: Fix another potential fd leak
This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 2a3a07dff7 Fix another potential fd leak
2a3a07dff7 is described below
commit 2a3a07dff73f399d39264b7247c2718e034efc7d
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Jun 30 20:17:57 2026 +0100
Fix another potential fd leak
---
.../catalina/ssi/SSIServletExternalResolver.java | 25 ++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/catalina/ssi/SSIServletExternalResolver.java b/java/org/apache/catalina/ssi/SSIServletExternalResolver.java
index c39f338016..56ccefc9ce 100644
--- a/java/org/apache/catalina/ssi/SSIServletExternalResolver.java
+++ b/java/org/apache/catalina/ssi/SSIServletExternalResolver.java
@@ -36,6 +36,7 @@ import jakarta.servlet.http.HttpServletResponse;
import org.apache.catalina.connector.Connector;
import org.apache.catalina.connector.Request;
+import org.apache.tomcat.util.ExceptionUtils;
import org.apache.tomcat.util.buf.B2CConverter;
import org.apache.tomcat.util.buf.UDecoder;
import org.apache.tomcat.util.http.Method;
@@ -600,11 +601,21 @@ public class SSIServletExternalResolver implements SSIExternalResolver {
@Override
public long getFileLastModified(String path, boolean virtual) throws IOException {
long lastModified = 0;
+ URLConnection urlConnection = null;
try {
- URLConnection urlConnection = getURLConnection(path, virtual);
+ urlConnection = getURLConnection(path, virtual);
lastModified = urlConnection.getLastModified();
} catch (IOException ignore) {
// Ignore this. It will always fail for non-file based includes
+ } finally {
+ if (urlConnection != null) {
+ try {
+ urlConnection.getInputStream().close();
+ } catch (IOException ioe) {
+ ExceptionUtils.handleThrowable(ioe);
+ lastModified = 0;
+ }
+ }
}
return lastModified;
}
@@ -623,11 +634,21 @@ public class SSIServletExternalResolver implements SSIExternalResolver {
@Override
public long getFileSize(String path, boolean virtual) throws IOException {
long fileSize = -1;
+ URLConnection urlConnection = null;
try {
- URLConnection urlConnection = getURLConnection(path, virtual);
+ urlConnection = getURLConnection(path, virtual);
fileSize = urlConnection.getContentLengthLong();
} catch (IOException ignore) {
// Ignore this. It will always fail for non-file based includes
+ } finally {
+ if (urlConnection != null) {
+ try {
+ urlConnection.getInputStream().close();
+ } catch (IOException ioe) {
+ ExceptionUtils.handleThrowable(ioe);
+ fileSize = -1;
+ }
+ }
}
return fileSize;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) branch main updated: Fix another potential fd leak
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Jun 30, 2026 at 10:00 PM Mark Thomas <ma...@apache.org> wrote:
>
> On 30/06/2026 20:53, Rémy Maucherat wrote:
>
> > So it should be possible to refactor it to:
> >
> > public long getFileLastModified(String path, boolean virtual)
> > throws IOException {
> > long lastModified = 0;
> > URLConnection urlConnection = null;
> > try {
> > urlConnection = getURLConnection(path, virtual);
> > try (InputStream _ = urlConnection.getInputStream()) {
> > lastModified = urlConnection.getLastModified();
> > } catch (Exception e) {
> > ExceptionUtils.handleThrowable(e);
> > }
> > } catch (IOException ignore) {
> > // Ignore this. It will always fail for non-file based includes
> > }
> > return lastModified;
> > }
> > > Do your Claude actually like the code pattern we have in Tomcat right
> > now ? Somehow I doubt it.
> I haven't been asking it about implementation detail.
>
> At the moment, it seems to be more concerned about code duplication.
> That is mostly fixable although we'd need a couple of copies.
Ah ok, weird. My model is more interested in the patterns it does not
like or that it thinks are weird.
> There is also a query whether the fd leak is real or not. It certainly
> was real when these fixes were originally written. I suspect it might
> not be considering the more extreme edge cases where the resource may be
> in a JAR packaged in another JAR/WAR. If I had (a lot more) time, I'd
> run some tests to look into that.
Let's say the URL is a https one. In that case, reading the JVM code,
you have to cast to HttpURLConnection and call disconnect() otherwise
getInputStream().close() does not do the same thing.
Actually, I see CloseableURLConnection in org.apache.commons.io, which
has AutoCloseable and all it does in its close() is:
if (conn instanceof HttpURLConnection) {
((HttpURLConnection) conn).disconnect();
}
Coincidentally, our code will not do that. However, for JAR URLs, our
strategy works perfectly (getInputStream().close() will close the JAR
file), but only as long as .setUseCaches(false) has been called. We do
it in the JreLeakPreventionListener using
URLConnection.setDefaultUseCaches("jar", false). If any of that
scaffolding is missing, getInputStream().close() does nothing useful.
Maybe this refactoring would be better:
1) Wrap into an AutoCloseable URL connection like commons.io is doing.
2) On connect, always set setsetUseCaches to false whenever possible,
especially for JarURLConnection.
3) On close, handle HttpURLConnection and call disconnect.
4) On close, handle JarURLConnection and make sure to call
getInputStream().close(), unless getInputStream has been called, in
which case it has to be assumed that the caller should close it.
I'll add the utility class for review.
About duplication, the pattern looks to be in plenty of spots:
org/apache/jasper/compiler/Compiler.java:
iuc.getInputStream().close();
org/apache/jasper/compiler/TldCache.java:
conn.getInputStream().close();
org/apache/jasper/JspCompilationContext.java:
uc.getInputStream().close();
org/apache/tomcat/Jar.java: urlConn.getInputStream().close();
org/apache/tomcat/util/file/ConfigurationSource.java:
connection.getInputStream().close();
org/apache/catalina/startup/ContextConfig.java:
uc.getInputStream().close();
org/apache/catalina/startup/ContextConfig.java:
uc.getInputStream().close();
org/apache/catalina/ssi/SSIServletExternalResolver.java:
urlConnection.getInputStream().close();
org/apache/catalina/users/MemoryUserDatabase.java:
uConn.getInputStream().close();
Rémy
> 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: (tomcat) branch main updated: Fix another potential fd leak
Posted by Mark Thomas <ma...@apache.org>.
On 30/06/2026 20:53, Rémy Maucherat wrote:
> So it should be possible to refactor it to:
>
> public long getFileLastModified(String path, boolean virtual)
> throws IOException {
> long lastModified = 0;
> URLConnection urlConnection = null;
> try {
> urlConnection = getURLConnection(path, virtual);
> try (InputStream _ = urlConnection.getInputStream()) {
> lastModified = urlConnection.getLastModified();
> } catch (Exception e) {
> ExceptionUtils.handleThrowable(e);
> }
> } catch (IOException ignore) {
> // Ignore this. It will always fail for non-file based includes
> }
> return lastModified;
> }
> > Do your Claude actually like the code pattern we have in Tomcat right
> now ? Somehow I doubt it.
I haven't been asking it about implementation detail.
At the moment, it seems to be more concerned about code duplication.
That is mostly fixable although we'd need a couple of copies.
There is also a query whether the fd leak is real or not. It certainly
was real when these fixes were originally written. I suspect it might
not be considering the more extreme edge cases where the resource may be
in a JAR packaged in another JAR/WAR. If I had (a lot more) time, I'd
run some tests to look into that.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) branch main updated: Fix another potential fd leak
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Jun 30, 2026 at 9:18 PM <ma...@apache.org> wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> markt-asf pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/main by this push:
> new 2a3a07dff7 Fix another potential fd leak
> 2a3a07dff7 is described below
>
> commit 2a3a07dff73f399d39264b7247c2718e034efc7d
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Tue Jun 30 20:17:57 2026 +0100
>
> Fix another potential fd leak
> ---
> .../catalina/ssi/SSIServletExternalResolver.java | 25 ++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/java/org/apache/catalina/ssi/SSIServletExternalResolver.java b/java/org/apache/catalina/ssi/SSIServletExternalResolver.java
> index c39f338016..56ccefc9ce 100644
> --- a/java/org/apache/catalina/ssi/SSIServletExternalResolver.java
> +++ b/java/org/apache/catalina/ssi/SSIServletExternalResolver.java
> @@ -36,6 +36,7 @@ import jakarta.servlet.http.HttpServletResponse;
>
> import org.apache.catalina.connector.Connector;
> import org.apache.catalina.connector.Request;
> +import org.apache.tomcat.util.ExceptionUtils;
> import org.apache.tomcat.util.buf.B2CConverter;
> import org.apache.tomcat.util.buf.UDecoder;
> import org.apache.tomcat.util.http.Method;
> @@ -600,11 +601,21 @@ public class SSIServletExternalResolver implements SSIExternalResolver {
> @Override
> public long getFileLastModified(String path, boolean virtual) throws IOException {
> long lastModified = 0;
> + URLConnection urlConnection = null;
> try {
> - URLConnection urlConnection = getURLConnection(path, virtual);
> + urlConnection = getURLConnection(path, virtual);
> lastModified = urlConnection.getLastModified();
> } catch (IOException ignore) {
> // Ignore this. It will always fail for non-file based includes
> + } finally {
> + if (urlConnection != null) {
> + try {
> + urlConnection.getInputStream().close();
> + } catch (IOException ioe) {
> + ExceptionUtils.handleThrowable(ioe);
> + lastModified = 0;
> + }
> + }
> }
So it should be possible to refactor it to:
public long getFileLastModified(String path, boolean virtual)
throws IOException {
long lastModified = 0;
URLConnection urlConnection = null;
try {
urlConnection = getURLConnection(path, virtual);
try (InputStream _ = urlConnection.getInputStream()) {
lastModified = urlConnection.getLastModified();
} catch (Exception e) {
ExceptionUtils.handleThrowable(e);
}
} catch (IOException ignore) {
// Ignore this. It will always fail for non-file based includes
}
return lastModified;
}
Do your Claude actually like the code pattern we have in Tomcat right
now ? Somehow I doubt it.
Rémy
> return lastModified;
> }
> @@ -623,11 +634,21 @@ public class SSIServletExternalResolver implements SSIExternalResolver {
> @Override
> public long getFileSize(String path, boolean virtual) throws IOException {
> long fileSize = -1;
> + URLConnection urlConnection = null;
> try {
> - URLConnection urlConnection = getURLConnection(path, virtual);
> + urlConnection = getURLConnection(path, virtual);
> fileSize = urlConnection.getContentLengthLong();
> } catch (IOException ignore) {
> // Ignore this. It will always fail for non-file based includes
> + } finally {
> + if (urlConnection != null) {
> + try {
> + urlConnection.getInputStream().close();
> + } catch (IOException ioe) {
> + ExceptionUtils.handleThrowable(ioe);
> + fileSize = -1;
> + }
> + }
> }
> return fileSize;
> }
>
>
> ---------------------------------------------------------------------
> 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