Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#41] Fail Gracefully when Debug Provider Invalid #42

Open
wants to merge 1 commit into
base: sustaining/13.5.x
Choose a base branch
from
Open

[#41] Fail Gracefully when Debug Provider Invalid #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion openam-shared/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS HEADER.
*
* Copyright 2011-2016 ForgeRock AS.
* Portions Copyright 2018-2019 Wren Security.
*
* The contents of this file are subject to the terms
* of the Common Development and Distribution License
Expand Down Expand Up @@ -75,6 +76,15 @@
</archive>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>

<configuration>
<source>8</source>
<target>8</target>
</configuration>
</plugin>
</plugins>
</build>

Expand All @@ -90,6 +100,7 @@
<artifactId>guice</artifactId>
<classifier>no_aop</classifier>
</dependency>

<dependency>
<groupId>com.google.inject.extensions</groupId>
<artifactId>guice-multibindings</artifactId>
Expand Down Expand Up @@ -146,6 +157,11 @@
<artifactId>forgerock-util</artifactId>
</dependency>

<dependency>
<groupId>org.forgerock.commons.guava</groupId>
<artifactId>forgerock-guava-base</artifactId>
</dependency>

<!-- IPv6 -->
<dependency>
<groupId>com.googlecode.java-ipv6</groupId>
Expand All @@ -161,6 +177,5 @@
<groupId>joda-time</groupId>
<artifactId>joda-time</artifactId>
</dependency>

</dependencies>
</project>
57 changes: 33 additions & 24 deletions openam-shared/src/main/java/com/sun/identity/shared/debug/Debug.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,22 +25,24 @@
* $Id: Debug.java,v 1.6 2009/08/19 05:41:17 veiming Exp $
*
* Portions Copyrighted 2013-2016 ForgeRock AS.
*
* Portions Copyright 2019 Wren Security.
*/

package com.sun.identity.shared.debug;

import java.util.Collection;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.ResourceBundle;

import org.slf4j.helpers.MessageFormatter;

import com.sun.identity.shared.configuration.SystemPropertiesManager;
import com.sun.identity.shared.debug.file.impl.StdDebugFile;
import com.sun.identity.shared.debug.impl.DebugProviderImpl;

import com.sun.identity.shared.locale.Locale;
import org.forgerock.guava.common.base.Strings;

// NOTE: Since JVM specs guarantee atomic access/updates to int variables
// (actually all variables except double and long), the design consciously
Expand Down Expand Up @@ -259,38 +261,45 @@ static IDebugProvider getDebugProvider() {
*/
private static synchronized void initialize() {
if (!serviceInitialized) {
String providerName = SystemPropertiesManager.get(DebugConstants.CONFIG_DEBUG_PROVIDER);
final String providerName;
IDebugProvider provider = null;
boolean providerLoadFailed = false;
Exception exceptionCatched = null;
if (providerName != null && providerName.trim().length() > 0) {
Exception exceptionCaught = null;

providerName =
Optional.ofNullable(
SystemPropertiesManager.get(DebugConstants.CONFIG_DEBUG_PROVIDER)
).map(String::trim)
.orElse(null);

if (!Strings.isNullOrEmpty(providerName)) {
try {
provider = (IDebugProvider) Class.forName(providerName).newInstance();
} catch (ClassNotFoundException cnex) {
provider = (IDebugProvider)Class.forName(providerName).newInstance();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that this code was already here but isn't newInstance considered evil? The docs say:

Note that this method propagates any exception thrown by the nullary constructor, including a checked exception. Use of this method effectively bypasses the compile-time exception checking that would otherwise be performed by the compiler. The Constructor.newInstance method avoids this problem by wrapping any exception thrown by the constructor in a (checked) InvocationTargetException.

} catch (ClassNotFoundException
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a point in catching specific exceptions? Doesn't any exception mean the provider failed to load? How would we deal with newer Java version throwing other exceptions?

| InstantiationException
| IllegalAccessException
| ClassCastException ex) {
providerLoadFailed = true;
exceptionCatched = cnex;
} catch (InstantiationException iex) {
providerLoadFailed = true;
exceptionCatched = iex;
} catch (IllegalAccessException iaex) {
providerLoadFailed = true;
exceptionCatched = iaex;
} catch (ClassCastException ccex) {
providerLoadFailed = true;
exceptionCatched = ccex;
exceptionCaught = ex;
}
}
if (provider == null) {
if (providerLoadFailed) {
ResourceBundle bundle = com.sun.identity.shared.locale.Locale.getInstallResourceBundle
("amUtilMsgs");
StdDebugFile.printError(Debug.class.getSimpleName(), bundle.getString("com.iplanet" + ".services" +
".debug.invalidprovider"), exceptionCatched);

}
if (provider == null) {
provider = new DebugProviderImpl();
}

setDebugProvider(provider);

if (providerLoadFailed) {
final ResourceBundle bundle = Locale.getInstallResourceBundle("amUtilMsgs");

StdDebugFile.printError(
Debug.class.getSimpleName(),
bundle.getString("com.iplanet.services.debug.invalidprovider"),
exceptionCaught
);
}

serviceInitialized = true;
}
}
Expand Down