Hi Eric,
I agree that there is a potential issue here, although as you point out
it's unlikely to occur in practise. Could you open a JIRA?
Thanks,
Colm.
On Wed, Jul 25, 2012 at 10:38 PM, Eric Johnson <e...@tibco.com> wrote:
> I've been looking over the Santuario Java code and I noticed what appears
> to be a bug.
>
> Specifically this code (with details not needed elided) from
> ResourceResolver.getInstance()**:
>
> for (ResourceResolver resolver : resolverList) {
> ResourceResolver resolverTmp = resolver;
> if (!resolver.resolverSpi.**engineIsThreadSafe()) {
> // generate a new instances of resolverSpi ...
> }
>
> // ...
>
> resolverTmp.resolverSpi.**secureValidation =
> secureValidation;
> if ((resolverTmp != null) && resolverTmp.canResolve(uri,
> baseURI)) {
> // Check to see whether the Resolver is allowed
> // check for certain types ....
> return resolverTmp;
> }
> }
>
> In case you didn't see it, the trouble is the juxtaposition of
> "resolverSpi.**engineIsThreadSafe()" followed by code that sets
> "secureValidation" on the very same instance of the spi, whether or not it
> is thread safe.
>
> Meaning, if two threads resolve at the same time, and one thread is
> attempting secure resolution while the other is not, all the "thread safe"
> resolvers risk a race condition where they will now be in an uncertain
> state. Of course, it turns out that all the built-in resolvers declare
> themselves thread-safe, so this magnifies the problem.
>
> In practice, this is not likely ever to occur, because any given
> application will likely share the same notion of "secureValidation".
>
> Am I mis-reading this code?
>
> Three observations
>
> #1 - the secureValidation flag needs only be set for the resolver that is
> actually chosen
>
> #2 - the secureValidation flag should instead be passed as a parameter
> ResourceResolverSpi.**engineResolve() method, not stored as data in the
> SPI instance.
>
> #3 - if there were ever a resolver to show up that isn't thread safe, and
> it also has properties, the logic above which creates a new instance of the
> SPI would obliterate the properties of the spi. It turns out only the HTTP
> resolver uses these properties for anything, so this may not be problematic
> in the real world.
>
> -Eric.
>
--
Colm O hEigeartaigh
Talend Community Coder
http://coders.talend.com
