Unresolved thread safety issue in AutowiredAnnotationBeanPostProcessor.AutowiredMethodElement [SPR-7635]

[spring-projects-issues]

Jon Seymour opened SPR-7635 and commented

There remains a thread safety issue in AutowiredAnnotationBeanPostProcessor.AutowiredMethodElement

This issue is unrelated to the issue reported in #10329 and represents a seperate exposure.

The issue was detected with Spring 2.5.6. Inspection reveals that the same issue exists in Spring 3.0.4.RELEASE

In the description that follows, Block A refers to the code starting at line 535 of org.springframework.beans.factory.annotation.AutowiredAnnotationBeanPostProcessor
and Block B refers to the code start at line 593 of the same class.

Both lines are within the inject(Object bean, String beanName, PropertyValues pvs) method of the AutowiredMethodElement inner class.

The blocks are quoted here for ease of reference:

// Block A - line 535

if (this.skip == null && this.pd != null && pvs != null && pvs.contains(this.pd.getName())) {
           // Explicit value provided as part of the bean definition.                                                                                          
           this.skip = Boolean.TRUE; Line 535
   }

// Block B - line 593
if (this.skip == null) {
if (this.pd != null && pvs instanceof MutablePropertyValues) {
((MutablePropertyValues) pvs).registerProcessedProperty(this.pd.getName()); // Line 595
}
this.skip = Boolean.FALSE; // Line 597
}

The issue arises because the statements execute in Block A and Block B are not executed atomically. In particular, if a Thread 1 is suspended after Line 595 by the Java Scheduler, then pvs.contains(this.pd.getName()) will return true for some other thread, Thread 2, executing block A causing Thread 2 to take the branch in that block.

If Thread 2 is then suspended before reaching Line 535 and Thread 1 executes line 597 then thread 2 is resumed, this.skip will be set to TRUE and remain TRUE.

As a result all further executions of the inject method will fail to execute the body of the inject method resulting in the failure of Spring to inject autowired dependencies into all subsequent instances of the bean until such time as the bean factory is eventually refreshed. On the otherhand, if thread 1 executes to completion without being interleaved with Thread 2, this.skip will be set to false and the intended behaviour will be observed.

The issues can occur even without the specified thread interleavings, given Java Memory Model implementation considerations.

---

Affects: 2.5.6, 3.0.4

Issue Links:

• NullPointerException in InjectionMetadata [SPR-7686] #12342 NullPointerException in InjectionMetadata
• Thread safety issue in InjectionMetaData affecting setter methods annotated with @Resource tags [SPR-7642] #12298 Thread safety issue in InjectionMetaData affecting setter methods annotated with @Resource tags

Referenced from: commits 5cb06f5, 27a10c7, 7893b3e, ac5b1bc

[spring-projects-issues]

Jon Seymour commented

Mmm..I think some of my line references were off.

The important interleaving is summarised by these expressions and statements.

Thread 1: Line 595 ((MutablePropertyValues) pvs).registerProcessedProperty(this.pd.getName());
Thread 2: Line 531 pvs.contains(this.pd.getName())
Thread 1: Line 597 this.skip = Boolean.FALSE;
Thread 2: Line 533 this.skip = Boolean.TRUE;

FWIW: this condition has been observed in the wild in circumstances where queued load is released on a newly initialized server which does a lot of autowiring in to Spring prototypes [ i.e., there is no protection from the singleton mutex because the instances being constructed are not singletons ]

[spring-projects-issues]

Jon Seymour commented

FWIW, my local fix for this is:

declare a boolean member variable:

private boolean locked = false;

add the following guard code at the top of the method:

synchronized (this)
{
while (locked)
{
this.wait();
}

    if (!cached)
    {
        //
        // If we have initialized once, we presume the remaining code is thread safe.
        // the intent is to minimize locking costs and the possibility of deadlocks
        // between threads that might acquire locks of this kind in a different order.
        //
        // We assume (without proof) that, after initialization, the codes paths are, in fact, thread safe.
        //
                locked = true;
    }
}

put the remaining code in try {} finally {} block

add the following code to the (new) finally block

synchronized (this)
{
    locked = false;
    this.notifyAll();
}

The net result is that a lock is obtained during initialization of the class, but is not otherwise held.

[spring-projects-issues]

Jon Seymour commented

Thinking about this some more, the possibility of deadlock could be reduced further if the release of the (proposed) lock occurred immediately before the following block:

if (arguments != null) { // Line 599
ReflectionUtils.makeAccessible(method);
method.invoke(bean, arguments);
}

as well as in the finally block (to ensure it is released even if an exception occurs).

[spring-projects-issues]

Jon Seymour commented

Hi,

It turns out that my issue is actually related to processing of @Resource tags, not @Autowired tags.

And, as it happens, processing of @Resource tags has a very similar issue.

Will raise another report for this issue.

[spring-projects-issues]

Jon Seymour commented

The other issue is #12298.

[spring-projects-issues]

Juergen Hoeller commented

This should be fixed in tomorrow's 3.0.5 snapshot, with the "contains" call replaced with a "getPropertyValue != null" call which doesn't react to processedProperties (and hence doesn't suffer from a potential side effect with registerProcessedProperty).

Juergen

[spring-projects-issues]

Jon Seymour commented

Ok, I'll review that when it is available.

BTW: the locking approach I suggested for this defect actually did cause a deadlock for us because the code within the locked region can, in theory, invoke arbtirary code which can result in the classic problem of obtaining arbtirary locks in arbitrary orders.

I have to say, I am somewhat surprised that you are able to solve this issue without a lock of some kind given the nature of the tests involved, so I await your fix with interest!

[spring-projects-issues]

Juergen Hoeller commented

Basically, the volatile 'skip' variable should be sufficient as long as the condition that it checks isn't affected by a race condition itself. 'registerProcessedProperty' affects 'contains' calls but not "getPropertyValue != null" calls... So by using the latter instead of 'contains', the condition should lead to a consistent result no matter whether another thread already managed to get to 'registerProcessedProperty' or not.

Juergen

[spring-projects-issues]

Jon Seymour commented

Doesn't this change the behaviour of the code in ways that might surprise existing users of it?

I don't pretend to understand this code very well, but I would have thought that (currently) when two bean post processors claim the same property, then the first one wins, and the second one skips. With the changes in 3.05, both bean post processors will get an opportunity to inject, both will inject, and the last one will normally win (since it was the last to inject).

I would have thought also that the map updated by registerProcessedProperty is still subject to concurrent access in this case, since it is not synchronized.

This is just my $0.02 worth...

jon.

[spring-projects-issues]

Juergen Hoeller commented

Good point, this was originally meant to help with the interaction between multiple post-processors... Even if that is not relied on with typical Spring configuration, we do need to restore that behavior. There is no way to avoid synchronization then, it seems.

Juergen

[spring-projects-issues]

Jon Seymour commented

Speaking of synchronization...

Due to the fix for #10329, 3.0.x now has a synchronized block that Spring 2.5.6 does not have. The thing is, when I patched 2.5.6 with a more or less equivalent synchronized block in order to workaround this issue (as described above), I ended up deadlocking the container.

The reason is that the lock that protects this.cached (in 3.0x) or the lock that I proposed above both enclose calls to arbitrary code via the nested call to beanFactory.resolveDependency(). Calls to arbitrary code raise the possibility of locks being acquired in arbitrary order and hence the possibility of deadlock.

I'll need to a little more work to produce a concrete example of how this occurs, but I have a feeling that even the existing synchronization in 3.0.4 due to the resolution of #10329 is broad enough to cause applications like ours deadlock issues.

[spring-projects-issues]

Jon Seymour commented

More correctly, the lock that protects - his.cachedMethodArguments

[spring-projects-issues]

Juergen Hoeller commented

I've committed another revision which will make it into the next 3.0.5 snapshot.

We are doing the actual skip check with synchronization now, whereas we're proceeding with a skip value if available outside of the synchronized block (similar to the handling of the cached flag, with both flags being volatile for immediate visibility). All such injection elements synchronize on the PropertyValues object of the containing BeanDefinition now, which in turn enforces visibility guarantees for the processedProperty part inbetween those independent injection elements.

As for deadlock potential, I don't see this for the skip check now since there are only well-controlled calls there, and that only once for each injection element. However, the cached check in the @Autowired elements might indeed be a problem; I'm still researching that part.

Juergen

[spring-projects-issues]

Jon Seymour commented

I think the deadlocking issue I observed would probably be solved if the true branch of this code...
{{
synchronized (this) {
if (!this.cached) {
// true branch
...
DependencyDescriptor descriptor = new DependencyDescriptor(methodParam, this.required);
this.cachedMethodArguments[i] = descriptor;
arguments[i] = beanFactory.resolveDependency(descriptor, beanName, autowiredBeanNames, typeConverter); // exposes the container to arbitrary code and this lock acquisitions
...

}
}
}}

did not attempt to both initialize cachedMethodArguments AND resolve the dependencies for the first instance at the same time.

If the loop simply limited itself to collecting cachedMethodArguments, then the need to invoke arbitrary code while holding a lock would be much reduced (if not completely eliminated - I don't know the code base well enough to say).

jon.

[spring-projects-issues]

Jon Seymour commented

Mmm...I see it is not quite that simple, because in some cases cachedMethodArguments[i] is initialized from the result of a beanFactory.resolveDependency call...

Anyway, I'll leave you to it :-)

[spring-projects-issues]

Jon Seymour commented

Synchronizing on pvs seems sound in checkPropertySkipping().

I wonder if it is really the best object to be synchronizing on in AutowiredMethodElement.inject().

Once skipping has been checked, neither pvs or this.skip are used again, so it seems a little bit odd to be using another object to guard a consistency condition between cached and cachedMethodArguments.

[spring-projects-issues]

Juergen Hoeller commented

Agreed, there is no need to synchronize on pvs for the actual @Autowired injection part. The local element is as good a monitor there, and a more fine-grained one.

Following your suggestion, we're resolving the dependency outside of the synchronized block now, before subsequently preparing the cached arguments. So we're only really locking for the actual cache preparation, not for the resolveDependency call which may possibly lead to calls into user code (i.e. other bean init code).

Juergen

[spring-projects-issues]

Jon Seymour commented

Ok, so that all looks mostly sane with respect to thread-safety and deadlocks.

One question though: if two or more methods hit the synchronized block that is protecting this.cached and this.cacheArguments, the loser will use its own arguments, rather than the winner's calculation of the cached arguments to initialize its own instance. All subsequent entrants will use the winner's arguments. This doesn't matter for the cachedMethodArgument elements that are RuntimeBeanReferences and DependencyDescriptors but it potentially might matter for the cachedMethodArgument elements that are resolved with

arguments[i] = beanFactory.resolveDependency(descriptors[i], beanName, autowiredBeanNames, typeConverter); // line 546

The reason it might matter is that under serial initialization, every instance will use the cached argument, but under certain multi-threaded cases the some instances will receoive a different
reference (in particular, the reference calculated by the losers).

I am not sure if this matters or not, but in case it does, an improvement might be to add an else branch to the if (!this.cached) {} statement that replaces elements from arguments that were resolved with beanFactory.resolveDependency() with the copies from cachedMethodArguments.

This way, all instances will be initialized with identical cachedMethodArguments references irrespective of whether calculation of cachedMethodArguments occurred under circumstances of high concurrency or not.

One other question: are the non DependencyDescriptor, non-RuntimeBeanReference elements of cachedMethodArguments always guaranteed to be singletons? If not, is there not a potential problem with a prototype unexpectedly shared between multiple points of use? [ If it always guaranteed to be a singleton, then the arguments above are probably moot, anyway. If not, then perhaps there is a different problem to solve ].

[spring-projects-issues]

Juergen Hoeller commented

Good point - there is no actual such guarantee: Those would typically "well-known dependency types" such as a BeanFactory reference itself, but since that mechanism is extensible (and supports an on-demand creaation mechanism), we can't rely on those being cacheable. Also, we don't actually cache that case for @Autowired fields either, so I've simply removed that clause from the @Autowired method case as well.

Juergen

[spring-projects-issues]

Jon Seymour commented

That looks pretty good.

A question about these lines 552-555:

if (arguments[i] == null) {
arguments = null;
break;
}

These lines would appear to imply that if a single argument resolves as null, then the injection method will not be invoked.

However, this logic only applies for instances that are initialized before this.cached is set true.

For instances initialized after this.cached is set true, the injection methods will be called with cachedMethodArguments irrespective of whether the dependency in position i evaluates as null.

This seems unusual - either the injection method should never be called with null parameters, or it should always be called that way.

[spring-projects-issues]

Juergen Hoeller commented

Good catch - even if this should actually be ok: If the initial arguments turn out to be not resolvable, they won't be resolvable later on either - and vice versa. In other words, we assume consistency in the resolvability of arguments (even if the actual argument values may differ per injected instance), and we're caching the need to inject or not inject a specific field or method that way.

Juergen

[spring-projects-issues]

Jon Seymour commented

Perhaps I have misunderstood your response, but I am not sure if you understood my point.

My point was that early instances will not have their injection methods invoked at all, whereas later instances will have their injection methods invoked, but with nulls in some parameters.

As a result, instances will experience different initialization life cycles depending on whether their arguments were calculated before or after this.cached became true.

[spring-projects-issues]

Jon Seymour commented

Apologies. I think I misread the code.

It will be consistently not called in both cases.

[spring-projects-issues]

Juergen Hoeller commented

Well, we're setting cachedMethodArguments = null if arguments == null within the synchronized block... resolveCachedArguments then returns null in that case for subsequent invocations, which makes the method invocation code skip the invocation since it is guarded by arguments != null there. Am I missing something?

Thanks for the thorough code review, BTW - this really helps!

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Ah ok, no problem. Was worth another look in any case.

Juergen

[spring-projects-issues]

Trond G. Ziarkowski commented

I think I have found an issue with the synchronization added for this issue.

We are using SpringBeanAutowiringInterceptor to use @Autowired on some setter methods in an EJB. This ends up with a call to AutowiredMethodElement.inject(Object bean, String beanName, PropertyValues pvs) with null values for beanName and pvs parameters. This causes a NullPointerException when trying to synchronize on pvs. I can avoid this by moving annotation from setter method to the field since AutowiredFieldElement.inject(Object bean, String beanName, PropertyValues pvs) tolerates null values for beanName and pvs.

Trond