From 3bb69ec39ca8493c5856897551ea19e035f2f27d Mon Sep 17 00:00:00 2001 From: qnnn <65326092+qnnn@users.noreply.github.com> Date: Sat, 20 Jul 2024 02:02:16 +0800 Subject: [PATCH 1/3] Fix potential memory leak when using ReadBodyRoutePredicateFactory Fixes gh-3465 --- .../filter/RemoveCachedBodyFilter.java | 24 ++----------------- .../handler/RoutePredicateHandlerMapping.java | 2 ++ .../support/ServerWebExchangeUtils.java | 22 +++++++++++++++++ 3 files changed, 26 insertions(+), 22 deletions(-) diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/RemoveCachedBodyFilter.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/RemoveCachedBodyFilter.java index 5307cb98..e3b15fe8 100644 --- a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/RemoveCachedBodyFilter.java +++ b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/filter/RemoveCachedBodyFilter.java @@ -16,37 +16,17 @@ package org.springframework.cloud.gateway.filter; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; import reactor.core.publisher.Mono; +import org.springframework.cloud.gateway.support.ServerWebExchangeUtils; import org.springframework.core.Ordered; -import org.springframework.core.io.buffer.PooledDataBuffer; import org.springframework.web.server.ServerWebExchange; -import static org.springframework.cloud.gateway.support.ServerWebExchangeUtils.CACHED_REQUEST_BODY_ATTR; - public class RemoveCachedBodyFilter implements GlobalFilter, Ordered { - private static final Log log = LogFactory.getLog(RemoveCachedBodyFilter.class); - @Override public Mono filter(ServerWebExchange exchange, GatewayFilterChain chain) { - return chain.filter(exchange).doFinally(s -> { - Object attribute = exchange.getAttributes().remove(CACHED_REQUEST_BODY_ATTR); - if (attribute != null && attribute instanceof PooledDataBuffer) { - PooledDataBuffer dataBuffer = (PooledDataBuffer) attribute; - if (dataBuffer.isAllocated()) { - if (log.isTraceEnabled()) { - log.trace("releasing cached body in exchange attribute"); - } - // ensure proper release - while (!dataBuffer.release()) { - // release() counts down until zero, will never be infinite loop - } - } - } - }); + return chain.filter(exchange).doFinally(s -> ServerWebExchangeUtils.clearCachedRequestBody(exchange)); } @Override diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/handler/RoutePredicateHandlerMapping.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/handler/RoutePredicateHandlerMapping.java index fb12f36b..eb0006a8 100644 --- a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/handler/RoutePredicateHandlerMapping.java +++ b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/handler/RoutePredicateHandlerMapping.java @@ -24,6 +24,7 @@ import org.springframework.cloud.gateway.config.GatewayProperties; import org.springframework.cloud.gateway.config.GlobalCorsProperties; import org.springframework.cloud.gateway.route.Route; import org.springframework.cloud.gateway.route.RouteLocator; +import org.springframework.cloud.gateway.support.ServerWebExchangeUtils; import org.springframework.core.env.Environment; import org.springframework.web.cors.CorsConfiguration; import org.springframework.web.reactive.handler.AbstractHandlerMapping; @@ -99,6 +100,7 @@ public class RoutePredicateHandlerMapping extends AbstractHandlerMapping { }) .switchIfEmpty(Mono.empty().then(Mono.fromRunnable(() -> { exchange.getAttributes().remove(GATEWAY_PREDICATE_ROUTE_ATTR); + ServerWebExchangeUtils.clearCachedRequestBody(exchange); if (logger.isTraceEnabled()) { logger.trace("No RouteDefinition found for [" + getExchangeDesc(exchange) + "]"); } diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/support/ServerWebExchangeUtils.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/support/ServerWebExchangeUtils.java index 0ce96e64..36ee17d9 100644 --- a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/support/ServerWebExchangeUtils.java +++ b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/support/ServerWebExchangeUtils.java @@ -40,6 +40,7 @@ import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.core.io.buffer.DefaultDataBuffer; import org.springframework.core.io.buffer.NettyDataBuffer; +import org.springframework.core.io.buffer.PooledDataBuffer; import org.springframework.http.HttpStatus; import org.springframework.http.server.reactive.AbstractServerHttpResponse; import org.springframework.http.server.reactive.ServerHttpRequest; @@ -379,6 +380,27 @@ public final class ServerWebExchangeUtils { .flatMap(function); } + /** + * clear the request body in a ServerWebExchange attribute. The attribute is + * {@link #CACHED_REQUEST_BODY_ATTR}. + * @param exchange the available ServerWebExchange. + */ + public static void clearCachedRequestBody(ServerWebExchange exchange) { + Object attribute = exchange.getAttributes().remove(CACHED_REQUEST_BODY_ATTR); + if (attribute != null && attribute instanceof PooledDataBuffer) { + PooledDataBuffer dataBuffer = (PooledDataBuffer) attribute; + if (dataBuffer.isAllocated()) { + if (log.isTraceEnabled()) { + log.trace("releasing cached body in exchange attribute"); + } + // ensure proper release + while (!dataBuffer.release()) { + // release() counts down until zero, will never be infinite loop + } + } + } + } + private static ServerHttpRequest decorate(ServerWebExchange exchange, DataBuffer dataBuffer, boolean cacheDecoratedRequest) { if (dataBuffer.readableByteCount() > 0) { From da77ec780af4576a6a4e823c4c4621d909a749f4 Mon Sep 17 00:00:00 2001 From: ohprettyhak Date: Mon, 1 Jul 2024 23:04:58 +0900 Subject: [PATCH 2/3] Updates HeaderRoutePredicateFactory to use getValuesAsList Fixes gh-3447 --- .../predicate/HeaderRoutePredicateFactory.java | 5 +---- .../HeaderRoutePredicateFactoryTests.java | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactory.java b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactory.java index e96e9bc6..97d8c43d 100644 --- a/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactory.java +++ b/spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactory.java @@ -17,7 +17,6 @@ package org.springframework.cloud.gateway.handler.predicate; import java.util.Arrays; -import java.util.Collections; import java.util.List; import java.util.function.Predicate; import java.util.regex.Pattern; @@ -59,9 +58,7 @@ public class HeaderRoutePredicateFactory extends AbstractRoutePredicateFactory values = exchange.getRequest() - .getHeaders() - .getOrDefault(config.header, Collections.emptyList()); + List values = exchange.getRequest().getHeaders().getValuesAsList(config.header); if (values.isEmpty()) { return false; } diff --git a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactoryTests.java b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactoryTests.java index 92be2a3a..e4a51451 100644 --- a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactoryTests.java +++ b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactoryTests.java @@ -92,6 +92,14 @@ public class HeaderRoutePredicateFactoryTests extends BaseWebClientTests { assertThat(predicate.toString()).contains("Header: myheader regexp=myregexp"); } + @Test + public void headerRouteHandlesCommaSeparatedValues() { + testClient.get().uri("/get").header("X-Example-Header", "value1, value2 ,exact_match,value3").exchange() + .expectStatus().isOk().expectHeader() + .valueEquals(HANDLER_MAPPER_HEADER, RoutePredicateHandlerMapping.class.getSimpleName()).expectHeader() + .valueEquals(ROUTE_ID_HEADER, "header_test_comma_separated"); + } + @EnableAutoConfiguration @SpringBootConfiguration @Import(DefaultTestConfig.class) @@ -103,8 +111,10 @@ public class HeaderRoutePredicateFactoryTests extends BaseWebClientTests { @Bean RouteLocator queryRouteLocator(RouteLocatorBuilder builder) { return builder.routes() - .route("header_exists_dsl", r -> r.header("X-Foo").filters(f -> f.prefixPath("/httpbin")).uri(uri)) - .build(); + .route("header_exists_dsl", r -> r.header("X-Foo").filters(f -> f.prefixPath("/httpbin")).uri(uri)) + .route("header_test_comma_separated", r -> r.header("X-Example-Header", "exact_match") + .filters(f -> f.prefixPath("/httpbin")).uri(uri)) + .build(); } } From f444ad5cf226c2663564daa1b5ac4225dec7f4c4 Mon Sep 17 00:00:00 2001 From: spencergibb Date: Thu, 26 Sep 2024 16:24:35 -0400 Subject: [PATCH 3/3] formatting --- .../HeaderRoutePredicateFactoryTests.java | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactoryTests.java b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactoryTests.java index e4a51451..6792ed97 100644 --- a/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactoryTests.java +++ b/spring-cloud-gateway-server/src/test/java/org/springframework/cloud/gateway/handler/predicate/HeaderRoutePredicateFactoryTests.java @@ -94,10 +94,16 @@ public class HeaderRoutePredicateFactoryTests extends BaseWebClientTests { @Test public void headerRouteHandlesCommaSeparatedValues() { - testClient.get().uri("/get").header("X-Example-Header", "value1, value2 ,exact_match,value3").exchange() - .expectStatus().isOk().expectHeader() - .valueEquals(HANDLER_MAPPER_HEADER, RoutePredicateHandlerMapping.class.getSimpleName()).expectHeader() - .valueEquals(ROUTE_ID_HEADER, "header_test_comma_separated"); + testClient.get() + .uri("/get") + .header("X-Example-Header", "value1, value2 ,exact_match,value3") + .exchange() + .expectStatus() + .isOk() + .expectHeader() + .valueEquals(HANDLER_MAPPER_HEADER, RoutePredicateHandlerMapping.class.getSimpleName()) + .expectHeader() + .valueEquals(ROUTE_ID_HEADER, "header_test_comma_separated"); } @EnableAutoConfiguration @@ -111,10 +117,12 @@ public class HeaderRoutePredicateFactoryTests extends BaseWebClientTests { @Bean RouteLocator queryRouteLocator(RouteLocatorBuilder builder) { return builder.routes() - .route("header_exists_dsl", r -> r.header("X-Foo").filters(f -> f.prefixPath("/httpbin")).uri(uri)) - .route("header_test_comma_separated", r -> r.header("X-Example-Header", "exact_match") - .filters(f -> f.prefixPath("/httpbin")).uri(uri)) - .build(); + .route("header_exists_dsl", r -> r.header("X-Foo").filters(f -> f.prefixPath("/httpbin")).uri(uri)) + .route("header_test_comma_separated", + r -> r.header("X-Example-Header", "exact_match") + .filters(f -> f.prefixPath("/httpbin")) + .uri(uri)) + .build(); } }