> 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.
>
