From 3bde3962a383ac941ef5b007ead188a6a910696c Mon Sep 17 00:00:00 2001 From: Rishabh Maurya Date: Tue, 18 Jun 2024 17:25:08 -0700 Subject: [PATCH] Fix a race condition in Derived Field parsing from search request Signed-off-by: Rishabh Maurya --- CHANGELOG.md | 1 + .../ingest/common/CopyProcessor.java | 3 +- .../org/opensearch/common/util/CopyUtil.java | 64 +++++++++++++++++++ .../mapper/DefaultDerivedFieldResolver.java | 4 +- .../org/opensearch/ingest/IngestDocument.java | 42 +----------- 5 files changed, 72 insertions(+), 42 deletions(-) create mode 100644 server/src/main/java/org/opensearch/common/util/CopyUtil.java diff --git a/CHANGELOG.md b/CHANGELOG.md index c0266d2a6dd4e..2b30332a445eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Remote Store] Rate limiter for remote store low priority uploads ([#14374](https://github.com/opensearch-project/OpenSearch/pull/14374/)) - Apply the date histogram rewrite optimization to range aggregation ([#13865](https://github.com/opensearch-project/OpenSearch/pull/13865)) - [Writable Warm] Add composite directory implementation and integrate it with FileCache ([12782](https://github.com/opensearch-project/OpenSearch/pull/12782)) +- Fix a race condition in derived field defined in search request ([14445](https://github.com/opensearch-project/OpenSearch/pull/14445)) ### Dependencies - Bump `org.gradle.test-retry` from 1.5.8 to 1.5.9 ([#13442](https://github.com/opensearch-project/OpenSearch/pull/13442)) diff --git a/modules/ingest-common/src/main/java/org/opensearch/ingest/common/CopyProcessor.java b/modules/ingest-common/src/main/java/org/opensearch/ingest/common/CopyProcessor.java index dec69df275130..816ca1f2a7e09 100644 --- a/modules/ingest-common/src/main/java/org/opensearch/ingest/common/CopyProcessor.java +++ b/modules/ingest-common/src/main/java/org/opensearch/ingest/common/CopyProcessor.java @@ -8,6 +8,7 @@ package org.opensearch.ingest.common; +import org.opensearch.common.util.CopyUtil; import org.opensearch.core.common.Strings; import org.opensearch.ingest.AbstractProcessor; import org.opensearch.ingest.ConfigurationUtils; @@ -95,7 +96,7 @@ public IngestDocument execute(IngestDocument document) { if (overrideTarget || document.hasField(target, true) == false || document.getFieldValue(target, Object.class) == null) { Object sourceValue = document.getFieldValue(source, Object.class); - document.setFieldValue(target, IngestDocument.deepCopy(sourceValue)); + document.setFieldValue(target, CopyUtil.deepCopy(sourceValue)); } else { throw new IllegalArgumentException("target field [" + target + "] already exists"); } diff --git a/server/src/main/java/org/opensearch/common/util/CopyUtil.java b/server/src/main/java/org/opensearch/common/util/CopyUtil.java new file mode 100644 index 0000000000000..b64ec887993ef --- /dev/null +++ b/server/src/main/java/org/opensearch/common/util/CopyUtil.java @@ -0,0 +1,64 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.util; + +import java.time.ZonedDateTime; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Date; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +/** + * Utility class for deep copying Object of different types. + */ +public class CopyUtil { + + /** + * Deep copies the provided object + * @return cloned object after deep copy. Throws {@link IllegalArgumentException} for unsupported object type. + */ + public static Object deepCopy(Object value) { + if (value instanceof Map) { + Map mapValue = (Map) value; + Map copy = new HashMap<>(mapValue.size()); + for (Map.Entry entry : mapValue.entrySet()) { + copy.put(entry.getKey(), deepCopy(entry.getValue())); + } + return copy; + } else if (value instanceof List) { + List listValue = (List) value; + List copy = new ArrayList<>(listValue.size()); + for (Object itemValue : listValue) { + copy.add(deepCopy(itemValue)); + } + return copy; + } else if (value instanceof byte[]) { + byte[] bytes = (byte[]) value; + return Arrays.copyOf(bytes, bytes.length); + } else if (value == null + || value instanceof Byte + || value instanceof Character + || value instanceof Short + || value instanceof String + || value instanceof Integer + || value instanceof Long + || value instanceof Float + || value instanceof Double + || value instanceof Boolean + || value instanceof ZonedDateTime) { + return value; + } else if (value instanceof Date) { + return ((Date) value).clone(); + } else { + throw new IllegalArgumentException("unexpected value type [" + value.getClass() + "]"); + } + } +} diff --git a/server/src/main/java/org/opensearch/index/mapper/DefaultDerivedFieldResolver.java b/server/src/main/java/org/opensearch/index/mapper/DefaultDerivedFieldResolver.java index c577a4117247b..531eb9d954512 100644 --- a/server/src/main/java/org/opensearch/index/mapper/DefaultDerivedFieldResolver.java +++ b/server/src/main/java/org/opensearch/index/mapper/DefaultDerivedFieldResolver.java @@ -12,6 +12,7 @@ import org.apache.logging.log4j.Logger; import org.opensearch.common.regex.Regex; import org.opensearch.index.query.QueryShardContext; +import org.opensearch.ingest.IngestDocument; import org.opensearch.script.Script; import java.io.IOException; @@ -189,9 +190,10 @@ private void initDerivedFieldTypes(Map derivedFieldsObject, List private Map getAllDerivedFieldTypeFromObject(Map derivedFieldObject) { Map derivedFieldTypes = new HashMap<>(); + // deep copy of derivedFieldObject is required as DocumentMapperParser modifies the map DocumentMapper documentMapper = queryShardContext.getMapperService() .documentMapperParser() - .parse(DerivedFieldMapper.CONTENT_TYPE, derivedFieldObject); + .parse(DerivedFieldMapper.CONTENT_TYPE, IngestDocument.deepCopyMap(derivedFieldObject)); if (documentMapper != null && documentMapper.mappers() != null) { for (Mapper mapper : documentMapper.mappers()) { if (mapper instanceof DerivedFieldMapper) { diff --git a/server/src/main/java/org/opensearch/ingest/IngestDocument.java b/server/src/main/java/org/opensearch/ingest/IngestDocument.java index 9ec59e4c275a8..e4b97948e4482 100644 --- a/server/src/main/java/org/opensearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/opensearch/ingest/IngestDocument.java @@ -32,6 +32,7 @@ package org.opensearch.ingest; +import org.opensearch.common.util.CopyUtil; import org.opensearch.core.common.Strings; import org.opensearch.core.common.util.CollectionUtils; import org.opensearch.index.VersionType; @@ -45,10 +46,8 @@ import java.time.ZoneOffset; import java.time.ZonedDateTime; import java.util.ArrayList; -import java.util.Arrays; import java.util.Base64; import java.util.Collections; -import java.util.Date; import java.util.EnumMap; import java.util.HashMap; import java.util.LinkedHashSet; @@ -754,44 +753,7 @@ public Map getSourceAndMetadata() { @SuppressWarnings("unchecked") public static Map deepCopyMap(Map source) { CollectionUtils.ensureNoSelfReferences(source, "IngestDocument: Self reference present in object."); - return (Map) deepCopy(source); - } - - public static Object deepCopy(Object value) { - if (value instanceof Map) { - Map mapValue = (Map) value; - Map copy = new HashMap<>(mapValue.size()); - for (Map.Entry entry : mapValue.entrySet()) { - copy.put(entry.getKey(), deepCopy(entry.getValue())); - } - return copy; - } else if (value instanceof List) { - List listValue = (List) value; - List copy = new ArrayList<>(listValue.size()); - for (Object itemValue : listValue) { - copy.add(deepCopy(itemValue)); - } - return copy; - } else if (value instanceof byte[]) { - byte[] bytes = (byte[]) value; - return Arrays.copyOf(bytes, bytes.length); - } else if (value == null - || value instanceof Byte - || value instanceof Character - || value instanceof Short - || value instanceof String - || value instanceof Integer - || value instanceof Long - || value instanceof Float - || value instanceof Double - || value instanceof Boolean - || value instanceof ZonedDateTime) { - return value; - } else if (value instanceof Date) { - return ((Date) value).clone(); - } else { - throw new IllegalArgumentException("unexpected value type [" + value.getClass() + "]"); - } + return (Map) CopyUtil.deepCopy(source); } /**