Review request for JDK-8139895: Introduce GuardingDynamicLinkerExporter

[Sundararajan Athijegannathan]

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