I would go for option (b). Colm.
On Thu, Aug 2, 2012 at 8:48 PM, Eric Johnson <[email protected]> wrote: > Hmmm. > > I need a little bit of direction. > > In the ResourceResolver class, the "getInstance" method is the one that > takes the flag for "secureValidation". Callers then take the returned > "resolver" instance, and call "resolve", which /doesn't/ take a > "secureValidation" parameter. > > There are two ways to solve this, I think: > > a) always make new copies of the resolver & spi instances - this means > that we can safely set the "secureValidation" flag on the instance of the > ResourceResolverSpi, and it will just work. This moots the > "engineIsThreadSafe" method, since every resolution will be done in a > thread safe manner (although the code still needs to be tweaked so that it > only creates a new resolver and spi instance once an Spi instance has been > chosen). > > b) change the parameters to the "resolve" method, so that it takes the > "secureValidation" parameter. Note that this removes the need for the > "secureValidation" field of "ResourceResolverSpi", as it will now always be > passed as state. Older Spi implementations might respect this flag. > However, since it was introduced in 1.5.0, this list of processors is > likely to be short, as it will have to be a processor written in the range > of [1.5.0,1.5.3) > > Thoughts? > > -Eric. > > > On 8/1/12 2:59 PM, Eric Johnson wrote: > >> Hi Colm, >> >> I think I can come up with a patch. It will take me a few days. >> >> I'm inclined to change the resolving code to pass around a >> "ResolveContext" instance which will take all of the existing parameters. >> This way, additional parameters added in the future will not re-break the >> API. >> >> Do you think that can work? Or do I need to submit a patch before you >> understand what I mean? >> >> I admit, I have reason for such an approach - my port of Santuario to use >> GenXDM has to change the resolver APIs to add a parameter. Combined with >> the "secureValidation" flag, that's two additional pieces of metadata, and >> that suggests the possibility of yet more future changes. >> >> -Eric. >> >> On 8/1/12 3:43 AM, Colm O hEigeartaigh (JIRA) wrote: >> >>> [ https://issues.apache.org/**jira/browse/SANTUARIO-337?** >>> page=com.atlassian.jira.**plugin.system.issuetabpanels:** >>> comment-tabpanel&**focusedCommentId=13426502#**comment-13426502<https://issues.apache.org/jira/browse/SANTUARIO-337?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13426502#comment-13426502>] >>> >>> Colm O hEigeartaigh commented on SANTUARIO-337: >>> ------------------------------**----------------- >>> >>> Hi Eric, >>> >>> Could you submit a patch for this issue? One downside to adding the >>> secureValidation flag to the ResourceResolverSpi.**engineResolve() >>> method is that it breaks backwards compatibility for any custom >>> ResourceResolverSpi implementations. I don't see a way around this though >>> unfortunately. >>> >>> Colm. >>> >>>> ResourceResolver does thread-unsafe handling of the "secureValidation" >>>> flag >>>> ------------------------------**------------------------------**--------------- >>>> >>>> >>>> Key: SANTUARIO-337 >>>> URL: https://issues.apache.org/** >>>> jira/browse/SANTUARIO-337<https://issues.apache.org/jira/browse/SANTUARIO-337> >>>> Project: Santuario >>>> Issue Type: Bug >>>> Components: Java >>>> Affects Versions: Java 1.5.2 >>>> Reporter: Eric Johnson >>>> Assignee: Colm O hEigeartaigh >>>> Priority: Minor >>>> >>>> From ResourceResolver.getInstance()**, with parts elided for brevity >>>> & clarity: >>>> 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 included resolvers declare >>>> themselves thread-safe, so this potentially magnifies the problem. >>>> In practice, this is not likely ever to occur, because any given >>>> application will likely share the same notion of "secureValidation". >>>> 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 discard the properties set on the registered spi. It turns >>>> out only the HTTP resolver uses these properties for anything, so this may >>>> not be problematic in the real world. >>>> Originally reported via email: >>>> http://article.gmane.org/**gmane.text.xml.security.devel/**7647<http://article.gmane.org/gmane.text.xml.security.devel/7647> >>>> >>> -- >>> This message is automatically generated by JIRA. >>> If you think it was sent incorrectly, please contact your JIRA >>> administrators: https://issues.apache.org/**jira/secure/** >>> ContactAdministrators!default.**jspa<https://issues.apache.org/jira/secure/ContactAdministrators%21default.jspa> >>> For more information on JIRA, see: http://www.atlassian.com/** >>> software/jira <http://www.atlassian.com/software/jira> >>> >>> >> > -- Colm O hEigeartaigh Talend Community Coder http://coders.talend.com
