Explanations satisfactory. +1 with initialization change.
-Sundar
On 10/20/2015 1:09 PM, Attila Szegedi wrote:
On Oct 20, 2015, at 5:19 AM, Sundararajan Athijegannathan
<sundararajan.athijegannat...@oracle.com> wrote:
* File: DynamicLinkerFactory:
+ String autoLoaderClassName = "unknown";
+ final GuardingDynamicLinkerExporter autoLoader = it.next();
+ try {
+ autoLoaderClassName = autoLoader.getClass().getName();
+ final String knownAutoLoaderClassName = autoLoaderClassName;
Do we need 2 variables to store auto loaded class name? autoLoaderClassName
seems to be used only to initialize second (final) variable. Am I missing
something?
autoLoaderClassName is also used in the catch(Throwable t) block, so if e.g.
it.next() throws an exception, we’ll report it as “with unknown” (FWIW, it’ll
mostly be a ServiceConfigurationError thrown from it.next() and that’ll put the
class name in it). Actually, thinking a bit more about it, it.next() will
*only* throw a SCE, so this initial setting to “unknown” is indeed not needed.
I’ll change that.
* If a particular linker exporter jar does not have necessary permission to export, it appears that it
can still do "deniel of service". i.e., SecurityException will force for loop in
"discoverAutoLoadLinkers"method to exit early and prevent other legitimate 'linker
exporters" from working.
ServiceLoader catches all instantiation errors and creates a
ServiceConfigurationError in response to them, see
java.util.ServiceLoader$LazyIterator.nextService() code around c.newInstance().
We have a try/catch for ServiceConfigurationError within the loop, so it will
continue with the next linker. There is another try/catch outside the loop, and
it will only catch non-recoverable errors, namely any ServiceConfigurationError
happening in ServiceLoader.load(), .iterator(), or .hasNext(), but those don’t
involve custom code and are to do with IO errors, misconfigured
META-INF/services files etc.
Mind you, I explicitly exempted any subclass of VirtualMachineError from the
recovery process, but that actually doesn’t allow a malicious linker to abort
the discovery process by throwing, say, an OutOfMemoryError as ServiceLoader
just catches all throwables; so this only guards against a VM error in our code.
Attila.
-Sundar
On 10/19/2015 9:51 PM, Attila Szegedi wrote:
Please review JDK-8139895 "Introduce GuardingDynamicLinkerExporter" at
<http://cr.openjdk.java.net/~attila/8139895/webrev.jdk9> for
<https://bugs.openjdk.java.net/browse/JDK-8139895>
Remarks:
- DynamicLinkerFactory now loads the linkers in a doPrivileged block so that
the lack of “dynalink.exportLinkersAutomatically” permission in the caller
doesn’t preclude it from loading trusted linkers. This is consistent with how
e.g. ScriptEngineManager uses ServiceLoader.
- DynamicLinkerFactory now has a new method "List<ServiceConfigurationError>
getAutoLoadingErrors()” so that the user can inspect if some automatically loaded linkers
failed to load. This is especially significant as the GuardingDynamicLinkerExporter can
have some failure modes: it can lack the dynalink.exportLinkersAutomatically permission,
it can return null from get() or any element of the returned element might be null
Thanks,
Attila.
