users@jersey.java.net

Re: [Jersey] Thread safety of XML-based factories <was> Re: [Jersey] Fixed <was> Re: [Jersey] Jersey vulnerable to XXE attack?

From: Tatu Saloranta <tsaloranta_at_gmail.com>
Date: Mon, 13 Jul 2009 13:44:43 -0700

On Mon, Jul 13, 2009 at 11:08 AM, Paul Sandoz<Paul.Sandoz_at_sun.com> wrote:
>
> On Jul 13, 2009, at 7:37 PM, Tatu Saloranta wrote:
...
>> That is: what I do is that iff I can configure factory instance up
>> front, instance will be thread-safe after this. What this means, then,
>> is that either
>>
>> (a) factory must be created eagerly when app/service starts, OR
>> (b) accessor must be synchronized, if lazy instantiation is needed.
...
>
> The trouble is i do not know how the JAXP/StAX implementations of the
> factories behave, so i had to take a very conservative position. I have to
> say it: it completely sucks to have to do this!

Right: as a developer of apps, I can trust my judgment on factories
not messing things; but if I was to develop Jersey, I would be more
conservative as well. :-)

As a sidenote: wouldn't being able to inject these factories (which
probably is already working? just define provider for these factories)
would at least allow users to prevent such lookups?

> Possibly one other solution is to wrap the factory and:
>
> 1) synchronize the wrapper methods to create parser instances; and
> 2) throw unsupported operation exceptions for methods that modify state.
>
> but even then i do not know if, when parsing, the parser may modify some
> stored state on the factory.

Worse than this: above would only protect against cases where user
does stupid/unsafe things; but more dangerous thing is that factory
itself may modify its state by trying to, say, reuse parsers (etc).
I think some version of Sjsxp (stax parser) tried reuse of parser, but
without proper synchronization. I think that has since been fixed, but
would be an example of non-thread-safe factory instance.

>> But if this is done, ThreadLocals should use soft references to refer
>> to factory. That allows GC to dump them if and as needed. That's an ok
>> approach.
>
> Hmm...  so the likelihood is if a thread goes idle for a bit the factories
> could be GC'ed. Then if the thread starts getting used for multiple
> requests, new factory instances will be created and reused, because the time
> between requests is smaller than the time to check if those objects would be
> GC'ed ?

Yes.

> So you mean support ThreadLocal<SoftReference<T>> and if the value of the
> SoftReference returns null the value in the thread local is replaced with a
> new instance ?

Exactly.

One more possibility: just figure out implementation class once (via
introspection), like:

Class<XMLInputFactory> staxInClass = XMLInputFactory.newInstance().getClass();

and afterwards instantiate using

XMLInputFactory factory = staxInClass.newInstance();

(and same for other similar factories)

which would avoid lookup costs.
I have never seen a case where the implementation would need to change
dynamically -- thereotically it could, if system property is changed;
I just have never seen any either needing it, or trying to make use of
it.

-+ Tatu +-