[java-opensaml] branch master updated: OSJ-285: Ensure that Closeable instances are actually closed after use
Brent Putman
putmanb at georgetown.edu
Fri Feb 14 21:14:16 EST 2020
This is an automated email from the git hooks/post-receive script.
putmanb pushed a commit to branch master
in repository java-opensaml.
View the commit online:
http://git.shibboleth.net/view/?p=java-opensaml.git;a=commit;h=9ad211c1c8d071c13ef220583d55553e1b93b612
The following commit(s) were added to refs/heads/master by this push:
new 9ad211c OSJ-285: Ensure that Closeable instances are actually closed after use
9ad211c is described below
commit 9ad211c1c8d071c13ef220583d55553e1b93b612
Author: Brent Putman <putmanb at georgetown.edu>
AuthorDate: Fri Feb 14 21:13:33 2020 -0500
OSJ-285: Ensure that Closeable instances are actually closed after use
---
.../AbstractXMLObjectProviderInitializer.java | 15 ++++++----
.../opensaml/core/xml/config/XMLConfigurator.java | 28 ++++++++++--------
.../saml/common/xml/SAMLSchemaBuilder.java | 26 +++++++++++++----
.../StorageServiceSAMLArtifactMapEntryFactory.java | 5 +++-
.../impl/AbstractReloadingMetadataResolver.java | 11 +++++--
.../binding/encoding/impl/HTTPPostEncoder.java | 7 +++--
.../binding/decoding/impl/HTTPPostDecoder.java | 14 ++++++---
.../binding/encoding/impl/HTTPArtifactEncoder.java | 7 +++--
.../binding/encoding/impl/HTTPPostEncoder.java | 7 +++--
.../impl/EvaluableCredentialCriteriaRegistry.java | 22 +++++++-------
.../opensaml/soap/client/http/HttpSOAPClient.java | 34 +++++++++++++++-------
.../storage/impl/client/ClientStorageService.java | 14 ++-------
12 files changed, 118 insertions(+), 72 deletions(-)
diff --git a/opensaml-core/src/main/java/org/opensaml/core/xml/config/AbstractXMLObjectProviderInitializer.java b/opensaml-core/src/main/java/org/opensaml/core/xml/config/AbstractXMLObjectProviderInitializer.java
index b9fc7de..6321cc6 100644
--- a/opensaml-core/src/main/java/org/opensaml/core/xml/config/AbstractXMLObjectProviderInitializer.java
+++ b/opensaml-core/src/main/java/org/opensaml/core/xml/config/AbstractXMLObjectProviderInitializer.java
@@ -17,6 +17,7 @@
package org.opensaml.core.xml.config;
+import java.io.IOException;
import java.io.InputStream;
import org.opensaml.core.config.InitializationException;
@@ -49,11 +50,15 @@ public abstract class AbstractXMLObjectProviderInitializer implements Initialize
}
// Checkstyle: ModifiedControlVariable ON
log.debug("Loading XMLObject provider configuration from resource '{}'", resource);
- final InputStream is = Thread.currentThread().getContextClassLoader().getResourceAsStream(resource);
- if (is != null) {
- configurator.load(is);
- } else {
- throw new XMLConfigurationException("Resource not found");
+ try (final InputStream is =
+ Thread.currentThread().getContextClassLoader().getResourceAsStream(resource)) {
+ if (is != null) {
+ configurator.load(is);
+ } else {
+ throw new XMLConfigurationException("Resource not found: " + resource);
+ }
+ } catch (final IOException e) {
+ throw new XMLConfigurationException("Error loading resource: " + resource, e);
}
}
} catch (final XMLConfigurationException e) {
diff --git a/opensaml-core/src/main/java/org/opensaml/core/xml/config/XMLConfigurator.java b/opensaml-core/src/main/java/org/opensaml/core/xml/config/XMLConfigurator.java
index 11c08b7..2152cac 100644
--- a/opensaml-core/src/main/java/org/opensaml/core/xml/config/XMLConfigurator.java
+++ b/opensaml-core/src/main/java/org/opensaml/core/xml/config/XMLConfigurator.java
@@ -132,20 +132,24 @@ public class XMLConfigurator {
return;
}
- try {
- if (configurationFile.isDirectory()) {
- final File[] configurations = configurationFile.listFiles();
- for (int i = 0; i < configurations.length; i++) {
- log.debug("Parsing configuration file {}", configurations[i].getAbsolutePath());
- load(new FileInputStream(configurations[i]));
+ if (configurationFile.isDirectory()) {
+ final File[] configurations = configurationFile.listFiles();
+ for (int i = 0; i < configurations.length; i++) {
+ log.debug("Parsing configuration file {}", configurations[i].getAbsolutePath());
+ try (final FileInputStream fis = new FileInputStream(configurations[i])) {
+ load(fis);
+ } catch (final IOException e) {
+ throw new XMLConfigurationException("Error loading config file: " + configurations[i]);
}
- } else {
- // Given file is not a directory so try to load it directly
- log.debug("Parsing configuration file {}", configurationFile.getAbsolutePath());
- load(new FileInputStream(configurationFile));
}
- } catch (final FileNotFoundException e) {
- // ignore, we already have the files
+ } else {
+ // Given file is not a directory so try to load it directly
+ log.debug("Parsing configuration file {}", configurationFile.getAbsolutePath());
+ try (final FileInputStream fis = new FileInputStream(configurationFile)) {
+ load(fis);
+ } catch (final IOException e) {
+ throw new XMLConfigurationException("Error loading config file: " + configurationFile);
+ }
}
}
diff --git a/opensaml-saml-api/src/main/java/org/opensaml/saml/common/xml/SAMLSchemaBuilder.java b/opensaml-saml-api/src/main/java/org/opensaml/saml/common/xml/SAMLSchemaBuilder.java
index 8145dbb..bcee3bd 100644
--- a/opensaml-saml-api/src/main/java/org/opensaml/saml/common/xml/SAMLSchemaBuilder.java
+++ b/opensaml-saml-api/src/main/java/org/opensaml/saml/common/xml/SAMLSchemaBuilder.java
@@ -17,6 +17,9 @@
package org.opensaml.saml.common.xml;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
import java.io.InputStream;
import javax.annotation.Nonnull;
@@ -36,6 +39,8 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.xml.sax.SAXException;
+import com.google.common.io.ByteStreams;
+
/**
* A convenience builder for creating {@link Schema}s for validating SAML 1.0, 1.1, and 2.0.
*
@@ -246,13 +251,22 @@ public class SAMLSchemaBuilder {
private void addSchemaToBuilder(@Nonnull final String source) {
final Class<SAMLSchemaBuilder> clazz = SAMLSchemaBuilder.class;
- final InputStream stream = clazz.getResourceAsStream(source);
- if (stream != null) {
- schemaBuilder.addSchema(stream);
- } else {
- log.error("Failed to locate schema resource: {}", source);
+ // To be safe, rather than pass the resource InputStream directly to the consumer,
+ // copy the bytes and ensure that the resource stream is closed.
+ try (final InputStream stream = clazz.getResourceAsStream(source)) {
+ if (stream != null) {
+ final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ ByteStreams.copy(stream, baos);
+ schemaBuilder.addSchema(new ByteArrayInputStream(baos.toByteArray()));
+ } else {
+ log.error("Failed to locate schema resource: {}", source);
+ if (unresolvedSchemaFatal) {
+ throw new XMLRuntimeException("Failed to locate schema resource: " + source);
+ }
+ }
+ } catch (final IOException e) {
if (unresolvedSchemaFatal) {
- throw new XMLRuntimeException("Failed to locate schema resource: " + source);
+ throw new XMLRuntimeException("Error loading schema resource: " + source);
}
}
}
diff --git a/opensaml-saml-impl/src/main/java/org/opensaml/saml/common/binding/artifact/impl/StorageServiceSAMLArtifactMapEntryFactory.java b/opensaml-saml-impl/src/main/java/org/opensaml/saml/common/binding/artifact/impl/StorageServiceSAMLArtifactMapEntryFactory.java
index ca60e08..7a16306 100644
--- a/opensaml-saml-impl/src/main/java/org/opensaml/saml/common/binding/artifact/impl/StorageServiceSAMLArtifactMapEntryFactory.java
+++ b/opensaml-saml-impl/src/main/java/org/opensaml/saml/common/binding/artifact/impl/StorageServiceSAMLArtifactMapEntryFactory.java
@@ -144,7 +144,10 @@ public class StorageServiceSAMLArtifactMapEntryFactory extends AbstractInitializ
}
try {
- final Element rootElement = getParserPool().parse(new StringReader(value)).getDocumentElement();
+ Element rootElement = null;
+ try (final StringReader sr = new StringReader(value)) {
+ rootElement = getParserPool().parse(sr).getDocumentElement();
+ }
final Node messageElement = rootElement.getFirstChild();
if (!ElementSupport.isElementNamed(rootElement, null, "Mapping")) {
throw new IOException("SAMLArtifactMapEntry XML not rooted by expected element");
diff --git a/opensaml-saml-impl/src/main/java/org/opensaml/saml/metadata/resolver/impl/AbstractReloadingMetadataResolver.java b/opensaml-saml-impl/src/main/java/org/opensaml/saml/metadata/resolver/impl/AbstractReloadingMetadataResolver.java
index efbcba5..2608251 100644
--- a/opensaml-saml-impl/src/main/java/org/opensaml/saml/metadata/resolver/impl/AbstractReloadingMetadataResolver.java
+++ b/opensaml-saml-impl/src/main/java/org/opensaml/saml/metadata/resolver/impl/AbstractReloadingMetadataResolver.java
@@ -625,7 +625,7 @@ public abstract class AbstractReloadingMetadataResolver extends AbstractBatchMet
/**
* Converts an InputStream into a byte array.
*
- * @param ins input stream to convert
+ * @param ins input stream to convert. The stream will be closed after its data is consumed.
*
* @return resultant byte array
*
@@ -642,11 +642,18 @@ public abstract class AbstractReloadingMetadataResolver extends AbstractBatchMet
output.write(buffer, 0, n);
}
- ins.close();
return output.toByteArray();
} catch (final IOException e) {
throw new ResolverException(e);
+ } finally {
+ try {
+ ins.close();
+ } catch (final IOException e) {
+ // Ignore here. If the read() threw also, then that should be reported, not this.
+ // If the close() throws, we don't care b/c we've already read the bytes.
+ }
}
+
}
/** Background task that refreshes metadata. */
diff --git a/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml1/binding/encoding/impl/HTTPPostEncoder.java b/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml1/binding/encoding/impl/HTTPPostEncoder.java
index ce979f0..f84086f 100644
--- a/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml1/binding/encoding/impl/HTTPPostEncoder.java
+++ b/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml1/binding/encoding/impl/HTTPPostEncoder.java
@@ -184,9 +184,10 @@ public class HTTPPostEncoder extends BaseSAML1MessageEncoder {
HttpServletSupport.setUTF8Encoding(response);
HttpServletSupport.setContentType(response, "text/html");
- final Writer out = new OutputStreamWriter(response.getOutputStream(), "UTF-8");
- velocityEngine.mergeTemplate(velocityTemplateId, "UTF-8", context, out);
- out.flush();
+ try (final Writer out = new OutputStreamWriter(response.getOutputStream(), "UTF-8")) {
+ velocityEngine.mergeTemplate(velocityTemplateId, "UTF-8", context, out);
+ out.flush();
+ }
} catch (final UnsupportedEncodingException e) {
log.error("UTF-8 encoding is not supported, this VM is not Java compliant");
throw new MessageEncodingException("Unable to encode message, UTF-8 encoding is not supported");
diff --git a/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/decoding/impl/HTTPPostDecoder.java b/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/decoding/impl/HTTPPostDecoder.java
index dda88d1..80509a4 100644
--- a/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/decoding/impl/HTTPPostDecoder.java
+++ b/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/decoding/impl/HTTPPostDecoder.java
@@ -18,6 +18,7 @@
package org.opensaml.saml.saml2.binding.decoding.impl;
import java.io.ByteArrayInputStream;
+import java.io.IOException;
import java.io.InputStream;
import javax.annotation.Nonnull;
@@ -87,10 +88,15 @@ public class HTTPPostDecoder extends BaseHttpServletRequestXMLMessageDecoder imp
log.debug("Decoded SAML relay state of: {}", relayState);
SAMLBindingSupport.setRelayState(messageContext, relayState);
- final InputStream base64DecodedMessage = getBase64DecodedMessage(request);
- final SAMLObject inboundMessage = (SAMLObject) unmarshallMessage(base64DecodedMessage);
- messageContext.setMessage(inboundMessage);
- log.debug("Decoded SAML message");
+ // The default impl is a ByteArrayInputStream, which really doesn't need to be closed. But this could
+ // be overridden, so be safe and make sure it gets closed. Also for style and consistency.
+ try (final InputStream base64DecodedMessage = getBase64DecodedMessage(request)) {
+ final SAMLObject inboundMessage = (SAMLObject) unmarshallMessage(base64DecodedMessage);
+ messageContext.setMessage(inboundMessage);
+ log.debug("Decoded SAML message");
+ } catch (final IOException e) {
+ throw new MessageDecodingException(e);
+ }
populateBindingContext(messageContext);
diff --git a/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/encoding/impl/HTTPArtifactEncoder.java b/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/encoding/impl/HTTPArtifactEncoder.java
index c37681e..f46b0b8 100644
--- a/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/encoding/impl/HTTPArtifactEncoder.java
+++ b/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/encoding/impl/HTTPArtifactEncoder.java
@@ -259,9 +259,10 @@ public class HTTPArtifactEncoder extends BaseSAML2MessageEncoder {
try {
log.debug("Invoking velocity template");
final HttpServletResponse response = getHttpServletResponse();
- final OutputStreamWriter outWriter = new OutputStreamWriter(response.getOutputStream());
- velocityEngine.mergeTemplate(velocityTemplateId, "UTF-8", context, outWriter);
- outWriter.flush();
+ try (final OutputStreamWriter outWriter = new OutputStreamWriter(response.getOutputStream())) {
+ velocityEngine.mergeTemplate(velocityTemplateId, "UTF-8", context, outWriter);
+ outWriter.flush();
+ }
} catch (final Exception e) {
log.error("Error invoking velocity template to create POST form: {}", e.getMessage());
throw new MessageEncodingException("Error creating output document", e);
diff --git a/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/encoding/impl/HTTPPostEncoder.java b/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/encoding/impl/HTTPPostEncoder.java
index 07b65af..3750c75 100644
--- a/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/encoding/impl/HTTPPostEncoder.java
+++ b/opensaml-saml-impl/src/main/java/org/opensaml/saml/saml2/binding/encoding/impl/HTTPPostEncoder.java
@@ -169,9 +169,10 @@ public class HTTPPostEncoder extends BaseSAML2MessageEncoder {
HttpServletSupport.setUTF8Encoding(response);
HttpServletSupport.setContentType(response, "text/html");
- final Writer out = new OutputStreamWriter(response.getOutputStream(), "UTF-8");
- velocityEngine.mergeTemplate(velocityTemplateId, "UTF-8", context, out);
- out.flush();
+ try (final Writer out = new OutputStreamWriter(response.getOutputStream(), "UTF-8")) {
+ velocityEngine.mergeTemplate(velocityTemplateId, "UTF-8", context, out);
+ out.flush();
+ }
} catch (final Exception e) {
log.error("Error invoking Velocity template: {}", e.getMessage());
throw new MessageEncodingException("Error creating output document", e);
diff --git a/opensaml-security-impl/src/main/java/org/opensaml/security/credential/criteria/impl/EvaluableCredentialCriteriaRegistry.java b/opensaml-security-impl/src/main/java/org/opensaml/security/credential/criteria/impl/EvaluableCredentialCriteriaRegistry.java
index 9c312be..ed08cd8 100644
--- a/opensaml-security-impl/src/main/java/org/opensaml/security/credential/criteria/impl/EvaluableCredentialCriteriaRegistry.java
+++ b/opensaml-security-impl/src/main/java/org/opensaml/security/credential/criteria/impl/EvaluableCredentialCriteriaRegistry.java
@@ -173,31 +173,31 @@ public final class EvaluableCredentialCriteriaRegistry {
initialized = true;
}
-// Checkstyle: ReturnCount OFF
/**
* Load the default set of criteria-evaluator mappings from the default mappings properties file.
*/
public static synchronized void loadDefaultMappings() {
final Logger log = getLogger();
log.debug("Loading default evaluable credential criteria mappings");
- final InputStream inStream =
- EvaluableCredentialCriteriaRegistry.class.getResourceAsStream(DEFAULT_MAPPINGS_FILE);
- if (inStream == null) {
- log.error("Could not open resource stream from default mappings file '{}'", DEFAULT_MAPPINGS_FILE);
- return;
- }
+ try (final InputStream inStream =
+ EvaluableCredentialCriteriaRegistry.class.getResourceAsStream(DEFAULT_MAPPINGS_FILE) ) {
+
+ if (inStream == null) {
+ log.error("Could not open resource stream from default mappings file '{}'", DEFAULT_MAPPINGS_FILE);
+ return;
+ }
- final Properties defaultMappings = new Properties();
- try {
+ final Properties defaultMappings = new Properties();
defaultMappings.load(inStream);
+
+ loadMappings(defaultMappings);
+
} catch (final IOException e) {
log.error("Error loading properties file from resource stream", e);
return;
}
- loadMappings(defaultMappings);
}
-// Checkstyle: ReturnCount OFF
/**
* Load a set of criteria-evaluator mappings from the supplied properties set.
diff --git a/opensaml-soap-api/src/main/java/org/opensaml/soap/client/http/HttpSOAPClient.java b/opensaml-soap-api/src/main/java/org/opensaml/soap/client/http/HttpSOAPClient.java
index c426c87..06eccd4 100644
--- a/opensaml-soap-api/src/main/java/org/opensaml/soap/client/http/HttpSOAPClient.java
+++ b/opensaml-soap-api/src/main/java/org/opensaml/soap/client/http/HttpSOAPClient.java
@@ -42,6 +42,7 @@ import org.apache.http.HttpEntity;
import org.apache.http.HttpResponse;
import org.apache.http.HttpStatus;
import org.apache.http.client.HttpClient;
+import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.entity.ByteArrayEntity;
import org.apache.http.entity.ContentType;
@@ -211,17 +212,28 @@ public class HttpSOAPClient extends AbstractInitializableComponent implements SO
try {
post = createPostMethod(endpoint, soapRequestParams, soapCtx.getEnvelope());
- final HttpResponse result = httpClient.execute(post);
- final int code = result.getStatusLine().getStatusCode();
- log.debug("Received HTTP status code of {} when POSTing SOAP message to {}", code, endpoint);
-
- if (code == HttpStatus.SC_OK) {
- processSuccessfulResponse(result, context);
- } else if (code == HttpStatus.SC_INTERNAL_SERVER_ERROR) {
- processFaultResponse(result, context);
- } else {
- throw new SOAPClientException("Received " + code + " HTTP response status code from HTTP request to "
- + endpoint);
+ HttpResponse response = null;
+ try {
+ response = httpClient.execute(post);
+ final int code = response.getStatusLine().getStatusCode();
+ log.debug("Received HTTP status code of {} when POSTing SOAP message to {}", code, endpoint);
+
+ if (code == HttpStatus.SC_OK) {
+ processSuccessfulResponse(response, context);
+ } else if (code == HttpStatus.SC_INTERNAL_SERVER_ERROR) {
+ processFaultResponse(response, context);
+ } else {
+ throw new SOAPClientException("Received " + code + " HTTP response status code from HTTP request to "
+ + endpoint);
+ }
+ } finally {
+ try {
+ if (response != null && response instanceof CloseableHttpResponse) {
+ ((CloseableHttpResponse)response).close();
+ }
+ } catch (final IOException e) {
+ log.error("Error closing HttpResponse", e);
+ }
}
} catch (final IOException e) {
throw new SOAPClientException("Unable to send request to " + endpoint, e);
diff --git a/opensaml-storage-impl/src/main/java/org/opensaml/storage/impl/client/ClientStorageService.java b/opensaml-storage-impl/src/main/java/org/opensaml/storage/impl/client/ClientStorageService.java
index c243f61..37021a0 100644
--- a/opensaml-storage-impl/src/main/java/org/opensaml/storage/impl/client/ClientStorageService.java
+++ b/opensaml-storage-impl/src/main/java/org/opensaml/storage/impl/client/ClientStorageService.java
@@ -156,20 +156,14 @@ public class ClientStorageService extends AbstractMapBackedStorageService implem
}
/** {@inheritDoc} */
- // Checkstyle: CyclomaticComplexity ON
-
- /** {@inheritDoc} */
public boolean isServerSide() {
return false;
}
-
- /** {@inheritDoc} */
/** {@inheritDoc} */
public boolean isClustered() {
return true;
}
-
/**
* Set the servlet request in which to manage per-request data.
@@ -550,8 +544,7 @@ public class ClientStorageService extends AbstractMapBackedStorageService implem
return;
}
- try {
- final JsonReader reader = Json.createReader(new StringReader(raw));
+ try (final JsonReader reader = Json.createReader(new StringReader(raw))) {
final JsonStructure st = reader.read();
if (!(st instanceof JsonObject)) {
throw new JsonException("Found invalid data structure while parsing context map");
@@ -651,9 +644,8 @@ public class ClientStorageService extends AbstractMapBackedStorageService implem
final long now = System.currentTimeMillis();
boolean empty = true;
- try {
- final StringWriter sink = new StringWriter(128);
- final JsonGenerator gen = Json.createGenerator(sink);
+ try (final StringWriter sink = new StringWriter(128);
+ final JsonGenerator gen = Json.createGenerator(sink)) {
gen.writeStartObject();
for (final Map.Entry<String,Map<String, MutableStorageRecord<?>>> context : contextMap.entrySet()) {
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list