Hi all,
I spent some time browsing through the usage of zip/jar input and
outpoutstreams.
I noticed that in the codebase often the source of the zip inpustream is
closed, but not the zipinputstream itself. this leads to leaking native
memory (as the inflater is open which holds on native memory)
I have attached a diff of my changes.
Shall I do issues for each or can somebody with enough powers maybe apply
them directly?
Fabian
--
Fabian Lange | Performance Expert
mobil: +49 (0) 160.3673393
codecentric AG | Hochstraße 11 | 42697 Solingen | Deutschland
Sitz der Gesellschaft: Solingen | HRB 25917| Amtsgericht Wuppertal
Vorstand: Michael Hochgürtel . Mirko Novakovic . Rainer Vehns
Aufsichtsrat: Patric Fedlmeier (Vorsitzender) . Klaus Jäger . Jürgen Schütz
diff --git
a/bundlerepository/src/main/java/org/apache/felix/bundlerepository/impl/DataModelHelperImpl.java
b/bundlerepository/src/main/java/org/apache/felix/bundlerepository/impl/DataModelHelperImpl.java
index 24947c1..3e59d36 100644
---
a/bundlerepository/src/main/java/org/apache/felix/bundlerepository/impl/DataModelHelperImpl.java
+++
b/bundlerepository/src/main/java/org/apache/felix/bundlerepository/impl/DataModelHelperImpl.java
@@ -108,10 +108,22 @@ public class DataModelHelperImpl implements
DataModelHelper
}
entry = zin.getNextEntry();
}
+ // as the ZipInputStream is not used further it would not be
closed.
+ if (is == null)
+ {
+ try
+ {
+ zin.close();
+ }
+ catch (IOException ex)
+ {
+ // Not much we can do.
+ }
+ }
}
else if (url.getPath().endsWith(".gz"))
{
- is = new GZIPInputStream(FileUtil.openURL(url));
+ is = new GZIPInputStream(FileUtil.openURL(url));
}
else
{
@@ -454,18 +466,17 @@ public class DataModelHelperImpl implements
DataModelHelper
}
private byte[] loadEntry(String name) throws IOException
{
- InputStream is = FileUtil.openURL(bundleUrl);
+ ZipInputStream zis = new
ZipInputStream(FileUtil.openURL(bundleUrl));
try
{
- ZipInputStream jis = new ZipInputStream(is);
- for (ZipEntry e = jis.getNextEntry(); e != null; e =
jis.getNextEntry())
+ for (ZipEntry e = zis.getNextEntry(); e != null; e =
zis.getNextEntry())
{
if (name.equalsIgnoreCase(e.getName()))
{
ByteArrayOutputStream baos = new
ByteArrayOutputStream();
byte[] buf = new byte[1024];
int n;
- while ((n = jis.read(buf, 0, buf.length)) > 0)
+ while ((n = zis.read(buf, 0, buf.length)) > 0)
{
baos.write(buf, 0, n);
}
@@ -475,7 +486,7 @@ public class DataModelHelperImpl implements DataModelHelper
}
finally
{
- is.close();
+ zis.close();
}
return null;
}
diff --git a/connect/src/main/java/org/apache/felix/connect/URLRevision.java
b/connect/src/main/java/org/apache/felix/connect/URLRevision.java
index 721b897..4df19ae 100644
--- a/connect/src/main/java/org/apache/felix/connect/URLRevision.java
+++ b/connect/src/main/java/org/apache/felix/connect/URLRevision.java
@@ -65,13 +65,11 @@ class URLRevision implements Revision
@Override
public Enumeration<String> getEntries()
{
- InputStream content = null;
JarInputStream jarInput = null;
try
{
- content = getUrlContent();
- jarInput = new JarInputStream(content);
+ jarInput = new JarInputStream(getUrlContent());
List<String> entries = new ArrayList<String>();
JarEntry jarEntry;
@@ -81,16 +79,13 @@ class URLRevision implements Revision
}
return Collections.enumeration(entries);
}
-
catch (IOException e)
{
e.printStackTrace();
return Collections.enumeration(Collections.EMPTY_LIST);
}
-
finally
{
- close(content);
close(jarInput);
}
}
diff --git
a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/DirectoryWatcher.java
b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/DirectoryWatcher.java
index e7c411a..9b5cf91 100644
---
a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/DirectoryWatcher.java
+++
b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/DirectoryWatcher.java
@@ -947,14 +947,7 @@ public class DirectoryWatcher extends Thread implements
BundleListener
URL transformed = artifact.getTransformedUrl();
String location = transformed.toString();
BufferedInputStream in = new
BufferedInputStream(transformed.openStream());
- try
- {
- bundle = installOrUpdateBundle(location, in,
artifact.getChecksum(), modified);
- }
- finally
- {
- in.close();
- }
+ bundle = installOrUpdateBundle(location, in,
artifact.getChecksum(), modified);
artifact.setBundleId(bundle.getBundleId());
}
// if the listener is an artifact transformer
@@ -968,14 +961,7 @@ public class DirectoryWatcher extends Thread implements
BundleListener
File transformed = artifact.getTransformed();
String location = path.toURI().normalize().toString();
BufferedInputStream in = new BufferedInputStream(new
FileInputStream(transformed != null ? transformed : path));
- try
- {
- bundle = installOrUpdateBundle(location, in,
artifact.getChecksum(), modified);
- }
- finally
- {
- in.close();
- }
+ bundle = installOrUpdateBundle(location, in,
artifact.getChecksum(), modified);
artifact.setBundleId(bundle.getBundleId());
}
installationFailures.remove(path);
@@ -997,53 +983,64 @@ public class DirectoryWatcher extends Thread implements
BundleListener
String bundleLocation, BufferedInputStream is, long checksum,
AtomicBoolean modified)
throws IOException, BundleException
{
- is.mark(256 * 1024);
- JarInputStream jar = new JarInputStream(is);
- Manifest m = jar.getManifest();
- if( m == null ) {
- throw new BundleException(
- "The bundle " + bundleLocation + " does not have a
META-INF/MANIFEST.MF! "+
- "Make sure, META-INF and MANIFEST.MF are the first 2
entries in your JAR!");
- }
- String sn =
m.getMainAttributes().getValue(Constants.BUNDLE_SYMBOLICNAME);
- String vStr = m.getMainAttributes().getValue(Constants.BUNDLE_VERSION);
- Version v = vStr == null ? Version.emptyVersion :
Version.parseVersion(vStr);
- Bundle[] bundles = context.getBundles();
- for (Bundle b : bundles) {
- if (b.getSymbolicName() != null && b.getSymbolicName().equals(sn))
{
- vStr = b.getHeaders().get(Constants.BUNDLE_VERSION);
- Version bv = vStr == null ? Version.emptyVersion :
Version.parseVersion(vStr);
- if (v.equals(bv)) {
- is.reset();
- if (Util.loadChecksum(b, context) != checksum) {
- log(Logger.LOG_WARNING,
- "A bundle with the same symbolic name ("
- + sn + ") and version (" + vStr
- + ") is already installed. Updating
this bundle instead.", null
- );
- stopTransient(b);
- Util.storeChecksum(b, checksum, context);
- b.update(is);
- modified.set(true);
+ JarInputStream jar = null;
+ try
+ {
+ is.mark(256 * 1024);
+ jar = new JarInputStream(is);
+ Manifest m = jar.getManifest();
+ if( m == null ) {
+ throw new BundleException(
+ "The bundle " + bundleLocation + " does not have a
META-INF/MANIFEST.MF! "+
+ "Make sure, META-INF and MANIFEST.MF are the first 2
entries in your JAR!");
+ }
+ String sn =
m.getMainAttributes().getValue(Constants.BUNDLE_SYMBOLICNAME);
+ String vStr =
m.getMainAttributes().getValue(Constants.BUNDLE_VERSION);
+ Version v = vStr == null ? Version.emptyVersion :
Version.parseVersion(vStr);
+ Bundle[] bundles = context.getBundles();
+ for (Bundle b : bundles) {
+ if (b.getSymbolicName() != null &&
b.getSymbolicName().equals(sn)) {
+ vStr = b.getHeaders().get(Constants.BUNDLE_VERSION);
+ Version bv = vStr == null ? Version.emptyVersion :
Version.parseVersion(vStr);
+ if (v.equals(bv)) {
+ is.reset();
+ if (Util.loadChecksum(b, context) != checksum) {
+ log(Logger.LOG_WARNING,
+ "A bundle with the same symbolic name ("
+ + sn + ") and version (" + vStr
+ + ") is already installed.
Updating this bundle instead.", null
+ );
+ stopTransient(b);
+ Util.storeChecksum(b, checksum, context);
+ b.update(is);
+ modified.set(true);
+ }
+ return b;
}
- return b;
}
}
+ is.reset();
+ Util.log(context, Logger.LOG_INFO, "Installing bundle " + sn
+ + " / " + v, null);
+ Bundle b = context.installBundle(bundleLocation, is);
+ Util.storeChecksum(b, checksum, context);
+ modified.set(true);
+
+ // Set default start level at install time, the user can override
it if he wants
+ if (startLevel != 0)
+ {
+ b.adapt(BundleStartLevel.class).setStartLevel(startLevel);
+ }
+
+ return b;
}
- is.reset();
- Util.log(context, Logger.LOG_INFO, "Installing bundle " + sn
- + " / " + v, null);
- Bundle b = context.installBundle(bundleLocation, is);
- Util.storeChecksum(b, checksum, context);
- modified.set(true);
-
- // Set default start level at install time, the user can override it
if he wants
- if (startLevel != 0)
+ finally
{
- b.adapt(BundleStartLevel.class).setStartLevel(startLevel);
+ if (jar != null)
+ {
+ jar.close();
+ }
}
-
- return b;
}
/**
diff --git
a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/Util.java
b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/Util.java
index db924e5..bc00c13 100644
--- a/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/Util.java
+++ b/fileinstall/src/main/java/org/apache/felix/fileinstall/internal/Util.java
@@ -232,35 +232,42 @@ public class Util
public static void jarDir(File directory, OutputStream os) throws
IOException
{
- // create a ZipOutputStream to zip the data to
- JarOutputStream zos = new JarOutputStream(os);
- zos.setLevel(Deflater.NO_COMPRESSION);
- String path = "";
- File manFile = new File(directory, JarFile.MANIFEST_NAME);
- if (manFile.exists())
+ JarOutputStream jar = null;
+ try
{
- byte[] readBuffer = new byte[8192];
- FileInputStream fis = new FileInputStream(manFile);
- try
+ jar = new JarOutputStream(os);
+ jar.setLevel(Deflater.NO_COMPRESSION);
+ String path = "";
+ File manFile = new File(directory, JarFile.MANIFEST_NAME);
+ if (manFile.exists())
{
- ZipEntry anEntry = new ZipEntry(JarFile.MANIFEST_NAME);
- zos.putNextEntry(anEntry);
- int bytesIn = fis.read(readBuffer);
- while (bytesIn != -1)
+ byte[] readBuffer = new byte[8192];
+ FileInputStream fis = new FileInputStream(manFile);
+ try
{
- zos.write(readBuffer, 0, bytesIn);
- bytesIn = fis.read(readBuffer);
+ ZipEntry anEntry = new ZipEntry(JarFile.MANIFEST_NAME);
+ zos.putNextEntry(anEntry);
+ int bytesIn = fis.read(readBuffer);
+ while (bytesIn != -1)
+ {
+ zos.write(readBuffer, 0, bytesIn);
+ bytesIn = fis.read(readBuffer);
+ }
+ }
+ finally
+ {
+ fis.close();
}
+ jar.closeEntry();
}
- finally
- {
- fis.close();
+ zipDir(directory, jar, path,
Collections.singleton(JarFile.MANIFEST_NAME));
+ }
+ finally
+ {
+ if (jar != null) {
+ jar.close();
}
- zos.closeEntry();
}
- zipDir(directory, zos, path,
Collections.singleton(JarFile.MANIFEST_NAME));
- // close the stream
- zos.close();
}
/**
diff --git
a/framework.security/src/main/java/org/apache/felix/framework/security/verifier/BundleDNParser.java
b/framework.security/src/main/java/org/apache/felix/framework/security/verifier/BundleDNParser.java
index b985b50..bec7814 100644
---
a/framework.security/src/main/java/org/apache/felix/framework/security/verifier/BundleDNParser.java
+++
b/framework.security/src/main/java/org/apache/felix/framework/security/verifier/BundleDNParser.java
@@ -276,134 +276,141 @@ public final class BundleDNParser
{
JarInputStream bundle = new JarInputStream(input, true);
- if (bundle.getManifest() == null)
+ try
{
- return null;
- }
-
- List certificateChains = new ArrayList();
-
- int count = certificateChains.size();
-
- // This is tricky: jdk1.3 doesn't say anything about what is happening
- // if a bad sig is detected on an entry - later jdk's do say that they
- // will throw a security Exception. The below should cater for both
- // behaviors.
- for (JarEntry entry = bundle.getNextJarEntry(); entry != null; entry =
bundle
- .getNextJarEntry())
- {
-
- if (entry.isDirectory() ||
- (entry.getName().startsWith("META-INF/") &&
- (entry.getName().indexOf('/', "META-INF/".length()) < 0)))
+ if (bundle.getManifest() == null)
{
- continue;
+ return null;
}
- for (byte[] tmp = new byte[4096]; bundle.read(tmp, 0, tmp.length)
!= -1;)
- {
- }
+ List certificateChains = new ArrayList();
- Certificate[] certificates = entry.getCertificates();
+ int count = certificateChains.size();
- // Workaround stupid bug in the sun jdk 1.5.x - getCertificates()
- // returns null there even if there are valid certificates.
- // This is a regression bug that has been fixed in 1.6.
- //
- // We use reflection to see whether we have a SignerCertPath
- // for the entry (available >= 1.5) and if so check whether
- // there are valid certificates - don't try this at home.
- if ((certificates == null) && (m_getCodeSigners != null))
+ // This is tricky: jdk1.3 doesn't say anything about what is
happening
+ // if a bad sig is detected on an entry - later jdk's do say that
they
+ // will throw a security Exception. The below should cater for both
+ // behaviors.
+ for (JarEntry entry = bundle.getNextJarEntry(); entry != null;
entry = bundle
+ .getNextJarEntry())
{
- try
+
+ if (entry.isDirectory() ||
+ (entry.getName().startsWith("META-INF/") &&
+ (entry.getName().indexOf('/', "META-INF/".length()) < 0)))
{
- Object[] signers = (Object[]) m_getCodeSigners.invoke(
- entry, null);
+ continue;
+ }
- if (signers != null)
+ for (byte[] tmp = new byte[4096]; bundle.read(tmp, 0,
tmp.length) != -1;)
+ {
+ }
+
+ Certificate[] certificates = entry.getCertificates();
+
+ // Workaround stupid bug in the sun jdk 1.5.x -
getCertificates()
+ // returns null there even if there are valid certificates.
+ // This is a regression bug that has been fixed in 1.6.
+ //
+ // We use reflection to see whether we have a SignerCertPath
+ // for the entry (available >= 1.5) and if so check whether
+ // there are valid certificates - don't try this at home.
+ if ((certificates == null) && (m_getCodeSigners != null))
+ {
+ try
{
- List certChains = new ArrayList();
+ Object[] signers = (Object[]) m_getCodeSigners.invoke(
+ entry, null);
- for (int i = 0; i < signers.length; i++)
+ if (signers != null)
{
- Object path = m_getSignerCertPath.invoke(
- signers[i], null);
+ List certChains = new ArrayList();
- certChains.addAll((List) m_getCertificates.invoke(
- path, null));
- }
+ for (int i = 0; i < signers.length; i++)
+ {
+ Object path = m_getSignerCertPath.invoke(
+ signers[i], null);
- certificates = (Certificate[]) certChains
- .toArray(new Certificate[certChains.size()]);
+ certChains.addAll((List)
m_getCertificates.invoke(
+ path, null));
+ }
+
+ certificates = (Certificate[]) certChains
+ .toArray(new Certificate[certChains.size()]);
+ }
+ }
+ catch (Exception ex)
+ {
+ ex.printStackTrace();
+ // Not much we can do - probably we are not on >= 1.5
}
}
- catch (Exception ex)
+
+ if ((certificates == null) || (certificates.length == 0))
{
- ex.printStackTrace();
- // Not much we can do - probably we are not on >= 1.5
+ return null;
}
- }
- if ((certificates == null) || (certificates.length == 0))
- {
- return null;
- }
+ List chains = new ArrayList();
- List chains = new ArrayList();
+ getRootChains(certificates, chains, check);
- getRootChains(certificates, chains, check);
-
- if (certificateChains.isEmpty())
- {
- certificateChains.addAll(chains);
- count = certificateChains.size();
- }
- else
- {
- for (Iterator iter2 = certificateChains.iterator(); iter2
- .hasNext();)
+ if (certificateChains.isEmpty())
+ {
+ certificateChains.addAll(chains);
+ count = certificateChains.size();
+ }
+ else
{
- X509Certificate cert = (X509Certificate) ((List) iter2
- .next()).get(0);
- boolean found = false;
- for (Iterator iter3 = chains.iterator(); iter3.hasNext();)
+ for (Iterator iter2 = certificateChains.iterator(); iter2
+ .hasNext();)
{
- X509Certificate cert2 = (X509Certificate) ((List) iter3
+ X509Certificate cert = (X509Certificate) ((List) iter2
.next()).get(0);
-
- if (cert.getSubjectDN().equals(cert2.getSubjectDN())
- && cert.equals(cert2))
+ boolean found = false;
+ for (Iterator iter3 = chains.iterator();
iter3.hasNext();)
+ {
+ X509Certificate cert2 = (X509Certificate) ((List)
iter3
+ .next()).get(0);
+
+ if
(cert.getSubjectDN().equals(cert2.getSubjectDN())
+ && cert.equals(cert2))
+ {
+ found = true;
+ break;
+ }
+ }
+ if (!found)
{
- found = true;
- break;
+ iter2.remove();
}
}
- if (!found)
+ }
+
+ if (certificateChains.isEmpty())
+ {
+ if (count > 0)
{
- iter2.remove();
+ throw new IOException("Bad signers");
}
+ return null;
}
}
- if (certificateChains.isEmpty())
+ List result = new ArrayList();
+
+ for (Iterator iter = certificateChains.iterator(); iter.hasNext();)
{
- if (count > 0)
- {
- throw new IOException("Bad signers");
- }
- return null;
+ result.addAll((List) iter.next());
}
- }
-
- List result = new ArrayList();
- for (Iterator iter = certificateChains.iterator(); iter.hasNext();)
+ return (X509Certificate[]) result.toArray(new
X509Certificate[result
+ .size()]);
+ }
+ finally
{
- result.addAll((List) iter.next());
+ bundle.close()
}
-
- return (X509Certificate[]) result.toArray(new X509Certificate[result
- .size()]);
}
private boolean isRevoked(Certificate certificate)
diff --git a/gogo/command/src/main/java/org/apache/felix/gogo/command/Util.java
b/gogo/command/src/main/java/org/apache/felix/gogo/command/Util.java
index 14759e9..25eb041 100644
--- a/gogo/command/src/main/java/org/apache/felix/gogo/command/Util.java
+++ b/gogo/command/src/main/java/org/apache/felix/gogo/command/Util.java
@@ -199,8 +199,7 @@ public class Util
if (extract)
{
- is = new FileInputStream(file);
- JarInputStream jis = new JarInputStream(is);
+ JarInputStream jis = new JarInputStream(new
FileInputStream(file));
out.println("Extracting...");
unjar(jis, localDir);
jis.close();
diff --git
a/webconsole/src/main/java/org/apache/felix/webconsole/internal/misc/LicenseServlet.java
b/webconsole/src/main/java/org/apache/felix/webconsole/internal/misc/LicenseServlet.java
index 12cbb0f..24edc90 100644
---
a/webconsole/src/main/java/org/apache/felix/webconsole/internal/misc/LicenseServlet.java
+++
b/webconsole/src/main/java/org/apache/felix/webconsole/internal/misc/LicenseServlet.java
@@ -234,10 +234,11 @@ public final class LicenseServlet extends
SimpleWebConsolePlugin implements Osgi
URL url = ( URL ) entries.nextElement();
InputStream ins = null;
+ ZipInputStream zin = null;
try
{
ins = url.openStream();
- ZipInputStream zin = new ZipInputStream( ins );
+ zin = new ZipInputStream( ins );
for ( ZipEntry zentry = zin.getNextEntry(); zentry !=
null; zentry = zin.getNextEntry() )
{
String name = zentry.getName();
@@ -266,6 +267,7 @@ public final class LicenseServlet extends
SimpleWebConsolePlugin implements Osgi
finally
{
IOUtils.closeQuietly( ins );
+ IOUtils.closeQuietly( zin );
}
}
