You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@santuario.apache.org by Eric Johnson <er...@tibco.com> on 2012/07/25 23:38:52 UTC
Thread safety and ResourceResolverSpi
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.
Re: Thread safety and ResourceResolverSpi
Posted by Eric Johnson <er...@tibco.com>.
Hi Colm,
Done.
https://issues.apache.org/jira/browse/SANTUARIO-337
-Eric.
On 7/31/12 3:24 AM, Colm O hEigeartaigh wrote:
> 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 <eric@tibco.com
> <ma...@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
>
Re: Thread safety and ResourceResolverSpi
Posted by Colm O hEigeartaigh <co...@apache.org>.
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 <er...@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