Skip to content

Commit

Permalink
Disable Jackson default JSON processing limits
Browse files Browse the repository at this point in the history
Jackson 2.15 introduced read constraints that are meant to protect deserialization path
from deeply nested/long string JSON documents. Introduced limits are too small for Trino
to work properly. Airlift is already disabling those limits when ObjectMapperProvider is used.

Additionaly ban direct usage of JsonFactory and JsonFactoryBuilder.
Instead JsonUtils.jsonFactory() or JsonUtils.jsonFactoryBuilder() should be used.
  • Loading branch information
wendigo authored and findepi committed Jun 21, 2023
1 parent dd9a53a commit 89c61de
Show file tree
Hide file tree
Showing 20 changed files with 120 additions and 23 deletions.
12 changes: 12 additions & 0 deletions .mvn/modernizer/violations.xml
Original file line number Diff line number Diff line change
Expand Up @@ -313,4 +313,16 @@
<version>1.8</version>
<comment>Prefer org.testng.annotations.AfterClass</comment>
</violation>

<violation>
<name>com/fasterxml/jackson/core/JsonFactory."&lt;init&gt;":()V</name>
<version>1.8</version>
<comment>Use io.trino.plugin.base.util.JsonUtils.jsonFactory()</comment>
</violation>

<violation>
<name>com/fasterxml/jackson/core/JsonFactoryBuilder."&lt;init&gt;":()V</name>
<version>1.8</version>
<comment>Use io.trino.plugin.base.util.JsonUtils.jsonFactoryBuilder() instead</comment>
</violation>
</modernizer>
19 changes: 18 additions & 1 deletion client/trino-cli/src/main/java/io/trino/cli/JsonPrinter.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,11 @@
package io.trino.cli;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonFactoryBuilder;
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.StreamReadConstraints;
import com.google.common.collect.ImmutableList;
import org.gaul.modernizer_maven_annotations.SuppressModernizer;

import java.io.IOException;
import java.io.Writer;
Expand All @@ -40,7 +43,7 @@ public JsonPrinter(List<String> fieldNames, Writer writer)
public void printRows(List<List<?>> rows, boolean complete)
throws IOException
{
JsonFactory jsonFactory = new JsonFactory().configure(JsonGenerator.Feature.AUTO_CLOSE_TARGET, false);
JsonFactory jsonFactory = jsonFactory();
try (JsonGenerator jsonGenerator = jsonFactory.createGenerator(writer)) {
jsonGenerator.setRootValueSeparator(null);
for (List<?> row : rows) {
Expand Down Expand Up @@ -70,4 +73,18 @@ private static Object formatValue(Object o)
}
return o;
}

@SuppressModernizer
// JsonFactoryBuilder usage is intentional as we don't want to bring additional dependency on plugin-toolkit module
private static JsonFactory jsonFactory()
{
return new JsonFactoryBuilder()
.streamReadConstraints(StreamReadConstraints.builder()
.maxNumberLength(Integer.MAX_VALUE)
.maxNestingDepth(Integer.MAX_VALUE)
.maxStringLength(Integer.MAX_VALUE)
.build())
.build()
.configure(JsonGenerator.Feature.AUTO_CLOSE_TARGET, false);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package io.trino.operator.scalar;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonFactoryBuilder;
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonParseException;
import com.fasterxml.jackson.core.JsonParser;
Expand All @@ -36,6 +35,7 @@
import static com.fasterxml.jackson.core.JsonToken.START_OBJECT;
import static com.fasterxml.jackson.core.JsonToken.VALUE_NULL;
import static io.airlift.slice.Slices.utf8Slice;
import static io.trino.plugin.base.util.JsonUtils.jsonFactoryBuilder;
import static io.trino.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT;
import static io.trino.util.JsonUtil.createJsonGenerator;
import static io.trino.util.JsonUtil.createJsonParser;
Expand Down Expand Up @@ -118,7 +118,7 @@ public final class JsonExtract
{
private static final int ESTIMATED_JSON_OUTPUT_SIZE = 512;

private static final JsonFactory JSON_FACTORY = new JsonFactoryBuilder()
private static final JsonFactory JSON_FACTORY = jsonFactoryBuilder()
.disable(CANONICALIZE_FIELD_NAMES)
.build();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package io.trino.operator.scalar;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonFactoryBuilder;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.databind.MappingJsonFactory;
Expand Down Expand Up @@ -47,14 +46,15 @@
import static com.fasterxml.jackson.core.JsonToken.VALUE_STRING;
import static com.fasterxml.jackson.core.JsonToken.VALUE_TRUE;
import static io.airlift.slice.Slices.utf8Slice;
import static io.trino.plugin.base.util.JsonUtils.jsonFactoryBuilder;
import static io.trino.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT;
import static io.trino.spi.type.Chars.padSpaces;
import static io.trino.util.JsonUtil.createJsonParser;
import static io.trino.util.JsonUtil.truncateIfNecessaryForErrorMessage;

public final class JsonFunctions
{
private static final JsonFactory JSON_FACTORY = new JsonFactoryBuilder()
private static final JsonFactory JSON_FACTORY = jsonFactoryBuilder()
.disable(CANONICALIZE_FIELD_NAMES)
.build();

Expand Down
4 changes: 2 additions & 2 deletions core/trino-main/src/main/java/io/trino/util/JsonUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package io.trino.util;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonFactoryBuilder;
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
Expand Down Expand Up @@ -77,6 +76,7 @@
import static com.fasterxml.jackson.core.JsonToken.START_ARRAY;
import static com.fasterxml.jackson.core.JsonToken.START_OBJECT;
import static com.google.common.base.Verify.verify;
import static io.trino.plugin.base.util.JsonUtils.jsonFactoryBuilder;
import static io.trino.spi.StandardErrorCode.INVALID_CAST_ARGUMENT;
import static io.trino.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT;
import static io.trino.spi.type.BigintType.BIGINT;
Expand Down Expand Up @@ -114,7 +114,7 @@ private JsonUtil() {}
// Note: JsonFactory is mutable, instances cannot be shared openly.
public static JsonFactory createJsonFactory()
{
return new JsonFactoryBuilder().disable(CANONICALIZE_FIELD_NAMES).build();
return jsonFactoryBuilder().disable(CANONICALIZE_FIELD_NAMES).build();
}

public static JsonParser createJsonParser(JsonFactory factory, Slice json)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import static io.trino.operator.scalar.JsonExtract.ObjectFieldJsonExtractor;
import static io.trino.operator.scalar.JsonExtract.ScalarValueJsonExtractor;
import static io.trino.operator.scalar.JsonExtract.generateExtractor;
import static io.trino.plugin.base.util.JsonUtils.jsonFactory;
import static io.trino.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT;
import static io.trino.spi.type.VarcharType.VARCHAR;
import static io.trino.testing.assertions.TrinoExceptionAssert.assertTrinoExceptionThrownBy;
Expand Down Expand Up @@ -341,7 +342,7 @@ public void testNoAutomaticEncodingDetection()
private static String doExtract(JsonExtractor<Slice> jsonExtractor, String json)
throws IOException
{
JsonFactory jsonFactory = new JsonFactory();
JsonFactory jsonFactory = jsonFactory();
JsonParser jsonParser = jsonFactory.createParser(json);
jsonParser.nextToken(); // Advance to the first token
Slice extract = jsonExtractor.extract(jsonParser);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package io.trino.hive.formats.line.json;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonFactoryBuilder;
import com.fasterxml.jackson.core.JsonLocation;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
Expand Down Expand Up @@ -68,6 +67,7 @@
import static io.trino.hive.formats.HiveFormatUtils.parseHiveDate;
import static io.trino.hive.formats.HiveFormatUtils.writeDecimal;
import static io.trino.plugin.base.type.TrinoTimestampEncoderFactory.createTimestampEncoder;
import static io.trino.plugin.base.util.JsonUtils.jsonFactoryBuilder;
import static io.trino.spi.type.BigintType.BIGINT;
import static io.trino.spi.type.BooleanType.BOOLEAN;
import static io.trino.spi.type.Chars.truncateToLengthAndTrimSpaces;
Expand Down Expand Up @@ -107,7 +107,7 @@
public class JsonDeserializer
implements LineDeserializer
{
private static final JsonFactory JSON_FACTORY = new JsonFactoryBuilder()
private static final JsonFactory JSON_FACTORY = jsonFactoryBuilder()
.disable(INTERN_FIELD_NAMES)
.build();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static io.trino.plugin.base.util.JsonUtils.jsonFactory;
import static io.trino.spi.type.BigintType.BIGINT;
import static io.trino.spi.type.BooleanType.BOOLEAN;
import static io.trino.spi.type.DateType.DATE;
Expand All @@ -67,7 +68,7 @@ public JsonSerializer(List<Column> columns)
.map(column -> field(column.name().toLowerCase(Locale.ROOT), column.type()))
.collect(toImmutableList()));

jsonFactory = new JsonFactory();
jsonFactory = jsonFactory();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package io.trino.plugin.base.util;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonFactoryBuilder;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.airlift.json.ObjectMapperProvider;
Expand All @@ -31,13 +30,14 @@
import static com.fasterxml.jackson.core.JsonFactory.Feature.CANONICALIZE_FIELD_NAMES;
import static com.fasterxml.jackson.databind.SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS;
import static com.google.common.base.Preconditions.checkState;
import static io.trino.plugin.base.util.JsonUtils.jsonFactoryBuilder;
import static io.trino.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT;
import static java.lang.String.format;
import static java.nio.charset.StandardCharsets.UTF_8;

public final class JsonTypeUtil
{
private static final JsonFactory JSON_FACTORY = new JsonFactoryBuilder().disable(CANONICALIZE_FIELD_NAMES).build();
private static final JsonFactory JSON_FACTORY = jsonFactoryBuilder().disable(CANONICALIZE_FIELD_NAMES).build();
private static final ObjectMapper SORTED_MAPPER = new ObjectMapperProvider().get().configure(ORDER_MAP_ENTRIES_BY_KEYS, true);

private JsonTypeUtil() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,17 @@
*/
package io.trino.plugin.base.util;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonFactoryBuilder;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.StreamReadConstraints;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.airlift.json.ObjectMapperProvider;
import org.gaul.modernizer_maven_annotations.SuppressModernizer;

import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -144,6 +148,24 @@ private static <T> T parseJson(JsonNode node, String jsonPointer, Class<T> javaT
return jsonTreeToValue(mappingsNode, javaType);
}

public static JsonFactory jsonFactory()
{
return jsonFactoryBuilder().build();
}

@SuppressModernizer
// JsonFactoryBuilder usage is intentional as we need to disable read constraints
// due to the limits introduced by Jackson 2.15
public static JsonFactoryBuilder jsonFactoryBuilder()
{
return new JsonFactoryBuilder()
.streamReadConstraints(StreamReadConstraints.builder()
.maxStringLength(Integer.MAX_VALUE)
.maxNestingDepth(Integer.MAX_VALUE)
.maxNumberLength(Integer.MAX_VALUE)
.build());
}

private interface ParserConstructor<I>
{
JsonParser createParser(ObjectMapper mapper, I input)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,15 @@
package io.trino.plugin.base.util;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.StreamReadConstraints;
import com.fasterxml.jackson.databind.JsonNode;
import org.testng.annotations.Test;

import java.io.IOException;
import java.io.UncheckedIOException;

import static io.trino.plugin.base.util.JsonUtils.jsonFactory;
import static io.trino.plugin.base.util.JsonUtils.jsonFactoryBuilder;
import static io.trino.plugin.base.util.JsonUtils.parseJson;
import static io.trino.plugin.base.util.TestJsonUtils.TestEnum.OPTION_A;
import static java.nio.charset.StandardCharsets.US_ASCII;
Expand Down Expand Up @@ -70,4 +73,28 @@ public void testTrailingContent()
.hasMessage("Could not parse JSON")
.hasStackTraceContaining("Unrecognized token 'not': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')");
}

@Test
public void testFactoryHasNoReadContraints()
{
assertReadConstraints(jsonFactory().streamReadConstraints());
assertReadConstraints(jsonFactoryBuilder().build().streamReadConstraints());
}

@Test
public void testBuilderHasNoReadConstraints()
{
assertReadConstraints(jsonFactoryBuilder().build().streamReadConstraints());
}

private static void assertReadConstraints(StreamReadConstraints constraints)
{
// Jackson 2.15 introduced read constraints limit that are too strict for
// Trino use-cases. Ensure that those limits are no longer present for JsonFactories.
//
// https://github.com/trinodb/trino/issues/17843
assertThat(constraints.getMaxStringLength()).isEqualTo(Integer.MAX_VALUE);
assertThat(constraints.getMaxNestingDepth()).isEqualTo(Integer.MAX_VALUE);
assertThat(constraints.getMaxNumberLength()).isEqualTo(Integer.MAX_VALUE);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.nio.ByteBuffer;
import java.util.UUID;

import static io.trino.plugin.base.util.JsonUtils.jsonFactory;
import static io.trino.spi.type.DecimalType.createDecimalType;
import static io.trino.spi.type.Decimals.rescale;
import static java.lang.String.format;
Expand All @@ -37,7 +38,7 @@ public class PartitionData
implements StructLike
{
private static final String PARTITION_VALUES_FIELD = "partitionValues";
private static final JsonFactory FACTORY = new JsonFactory();
private static final JsonFactory FACTORY = jsonFactory();
private static final ObjectMapper MAPPER = new ObjectMapper(FACTORY)
.configure(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS, true);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
*/
package io.trino.plugin.prometheus;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.core.type.TypeReference;
Expand All @@ -28,6 +27,7 @@
import java.util.List;
import java.util.Map;

import static io.trino.plugin.base.util.JsonUtils.jsonFactory;
import static io.trino.plugin.prometheus.PrometheusErrorCode.PROMETHEUS_PARSE_ERROR;
import static java.util.Collections.singletonList;

Expand All @@ -46,7 +46,7 @@ public PrometheusQueryResponseParse(InputStream response)
{
ObjectMapper mapper = new ObjectMapper();
mapper.registerModule(new JavaTimeModule());
JsonParser parser = new JsonFactory().createParser(response);
JsonParser parser = jsonFactory().createParser(response);
while (!parser.isClosed()) {
JsonToken jsonToken = parser.nextToken();
if (JsonToken.FIELD_NAME.equals(jsonToken)) {
Expand Down
5 changes: 5 additions & 0 deletions service/trino-proxy/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@
</properties>

<dependencies>
<dependency>
<groupId>io.trino</groupId>
<artifactId>trino-plugin-toolkit</artifactId>
</dependency>

<dependency>
<groupId>io.airlift</groupId>
<artifactId>bootstrap</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package io.trino.proxy;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonFactoryBuilder;
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
Expand Down Expand Up @@ -72,6 +71,7 @@
import static io.airlift.http.client.Request.Builder.preparePost;
import static io.airlift.http.client.StaticBodyGenerator.createStaticBodyGenerator;
import static io.airlift.jaxrs.AsyncResponseHandler.bindAsyncResponse;
import static io.trino.plugin.base.util.JsonUtils.jsonFactoryBuilder;
import static jakarta.ws.rs.core.MediaType.APPLICATION_JSON;
import static jakarta.ws.rs.core.MediaType.TEXT_PLAIN_TYPE;
import static jakarta.ws.rs.core.Response.Status.BAD_GATEWAY;
Expand All @@ -93,7 +93,7 @@ public class ProxyResource

private static final String X509_ATTRIBUTE = "jakarta.servlet.request.X509Certificate";
private static final Duration ASYNC_TIMEOUT = new Duration(2, MINUTES);
private static final JsonFactory JSON_FACTORY = new JsonFactoryBuilder().disable(CANONICALIZE_FIELD_NAMES).build();
private static final JsonFactory JSON_FACTORY = jsonFactoryBuilder().disable(CANONICALIZE_FIELD_NAMES).build();

private final ExecutorService executor = newCachedThreadPool(daemonThreadsNamed("proxy-%s"));
private final HttpClient httpClient;
Expand Down
5 changes: 5 additions & 0 deletions service/trino-verifier/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@
<artifactId>trino-parser</artifactId>
</dependency>

<dependency>
<groupId>io.trino</groupId>
<artifactId>trino-plugin-toolkit</artifactId>
</dependency>

<dependency>
<groupId>io.trino</groupId>
<artifactId>trino-spi</artifactId>
Expand Down
Loading

0 comments on commit 89c61de

Please sign in to comment.