From 317b51f2ad0894b4e7eea33e41938806551c3892 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 7 Mar 2018 09:57:16 +0000 Subject: [PATCH] Make ApplicationContextRequestMatcher and subclasses thread-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, when performing lazy initialisation of the context, ApplicationContextRequestMatcher assigned the context field before it called initialized. The context being non-null is used as the signal that it’s ok to call a subclass’s matches method. If one thread checks for a non-null context in between the field being assigned and initialized being called on another thread, matches will be called before the subclass is ready. This commit closes the window for the race condition by only assigning the context field once the subclass’s initialized method has been called. There is a secondary problem in each of the subclasses. Due to the use of double-checked locking in ApplicationContextRequestMatcher, it’s possible for a subclass’s matches method to be called by a thread that has not synchronised on the context lock that’s held when initialized is called and the delegate field is assigned. This means that the value assigned to the field may not be visible to that thread. This commit declares the delegate field of each ApplicationContextRequestMatcher subclass as volatile to ensure that, following initialisation, its value is guaranteed to be visible to all threads. Closes gh-12380 --- .../autoconfigure/security/servlet/EndpointRequest.java | 2 +- .../boot/autoconfigure/security/servlet/PathRequest.java | 2 +- .../security/servlet/StaticResourceRequest.java | 2 +- .../security/servlet/ApplicationContextRequestMatcher.java | 5 +++-- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/security/servlet/EndpointRequest.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/security/servlet/EndpointRequest.java index 9c87b916f0d..6e6b285304d 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/security/servlet/EndpointRequest.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/security/servlet/EndpointRequest.java @@ -100,7 +100,7 @@ public final class EndpointRequest { private final List excludes; - private RequestMatcher delegate; + private volatile RequestMatcher delegate; private EndpointRequestMatcher() { this(Collections.emptyList(), Collections.emptyList()); diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/servlet/PathRequest.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/servlet/PathRequest.java index 023c9f84215..562aef52094 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/servlet/PathRequest.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/servlet/PathRequest.java @@ -64,7 +64,7 @@ public final class PathRequest { public static final class H2ConsoleRequestMatcher extends ApplicationContextRequestMatcher { - private RequestMatcher delegate; + private volatile RequestMatcher delegate; private H2ConsoleRequestMatcher() { super(H2ConsoleProperties.class); diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/servlet/StaticResourceRequest.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/servlet/StaticResourceRequest.java index eafc839db6a..7e3d3d3f2d6 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/servlet/StaticResourceRequest.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/security/servlet/StaticResourceRequest.java @@ -100,7 +100,7 @@ public final class StaticResourceRequest { private final Set locations; - private RequestMatcher delegate; + private volatile RequestMatcher delegate; private StaticResourceRequestMatcher(Set locations) { super(ServerProperties.class); diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/security/servlet/ApplicationContextRequestMatcher.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/security/servlet/ApplicationContextRequestMatcher.java index 84441b3eb4d..11d29b1d25a 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/security/servlet/ApplicationContextRequestMatcher.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/security/servlet/ApplicationContextRequestMatcher.java @@ -69,8 +69,9 @@ public abstract class ApplicationContextRequestMatcher implements RequestMatc if (this.context == null) { synchronized (this.contextLock) { if (this.context == null) { - this.context = createContext(request); - initialized(this.context); + Supplier createdContext = createContext(request); + initialized(createdContext); + this.context = createdContext; } } }