From 45e7e24736eeb4a157ac89bd16a374dbf917ae26 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Wed, 11 Apr 2018 15:09:44 +0200 Subject: [PATCH] Restrict Document list access in ParseContext (#29463) Today we expose a mutable list of documents in ParseContext via ParseContext#docs(). This, on the one hand places knowledge how to access nested documnts in multiple places and on the other allows for potential illegal access to nested only docs after the docs are reversed. This change restricts the access and streamlines nested / non-root doc access. --- .../index/mapper/DocumentParser.java | 8 +-- .../index/mapper/FieldNamesFieldMapper.java | 2 +- .../index/mapper/IdFieldMapper.java | 3 -- .../index/mapper/IndexFieldMapper.java | 3 -- .../index/mapper/MetadataFieldMapper.java | 4 +- .../index/mapper/ParseContext.java | 53 +++++++++++++++---- .../index/mapper/RoutingFieldMapper.java | 4 -- .../index/mapper/SeqNoFieldMapper.java | 4 +- .../index/mapper/SourceFieldMapper.java | 43 ++++++--------- .../index/mapper/TypeFieldMapper.java | 4 -- .../index/mapper/VersionFieldMapper.java | 3 +- 11 files changed, 64 insertions(+), 67 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index a667af21700..61ff4a4ff3d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -76,7 +76,7 @@ final class DocumentParser { throw new IllegalStateException("found leftover path elements: " + remainingPath); } - reverseOrder(context); + context.postParse(); return parsedDocument(source, context, createDynamicUpdate(mapping, docMapper, context.getDynamicMappers())); } @@ -141,12 +141,6 @@ final class DocumentParser { return false; } - private static void reverseOrder(ParseContext.InternalParseContext context) { - // reverse the order of docs for nested docs support, parent should be last - if (context.docs().size() > 1) { - Collections.reverse(context.docs()); - } - } private static ParsedDocument parsedDocument(SourceToParse source, ParseContext.InternalParseContext context, Mapping update) { return new ParsedDocument( diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java index 95971c9385f..606777392de 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java @@ -263,7 +263,7 @@ public class FieldNamesFieldMapper extends MetadataFieldMapper { if (fieldType().isEnabled() == false) { return; } - for (ParseContext.Document document : context.docs()) { + for (ParseContext.Document document : context) { final List paths = new ArrayList<>(document.getFields().size()); String previousPath = ""; // used as a sentinel - field names can't be empty for (IndexableField field : document.getFields()) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java index 9d9c2b5cd63..32cb99e3f91 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java @@ -267,9 +267,6 @@ public class IdFieldMapper extends MetadataFieldMapper { super.parse(context); } - @Override - public void postParse(ParseContext context) throws IOException {} - @Override protected void parseCreateField(ParseContext context, List fields) throws IOException { if (fieldType.indexOptions() != IndexOptions.NONE || fieldType.stored()) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IndexFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IndexFieldMapper.java index 8e92ecc8bf6..4061303416b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IndexFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IndexFieldMapper.java @@ -172,9 +172,6 @@ public class IndexFieldMapper extends MetadataFieldMapper { @Override public void preParse(ParseContext context) throws IOException {} - @Override - public void postParse(ParseContext context) throws IOException {} - @Override protected void parseCreateField(ParseContext context, List fields) throws IOException {} diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java index 1240250a747..c5c90992241 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java @@ -64,7 +64,9 @@ public abstract class MetadataFieldMapper extends FieldMapper { /** * Called after {@link FieldMapper#parse(ParseContext)} on the {@link RootObjectMapper}. */ - public abstract void postParse(ParseContext context) throws IOException; + public void postParse(ParseContext context) throws IOException { + // do nothing + } @Override public MetadataFieldMapper merge(Mapper mergeWith) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParseContext.java b/server/src/main/java/org/elasticsearch/index/mapper/ParseContext.java index 4a7af6cba47..8c2eda31ca1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParseContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParseContext.java @@ -29,10 +29,11 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentParser; import java.util.ArrayList; +import java.util.Collections; import java.util.Iterator; import java.util.List; -public abstract class ParseContext { +public abstract class ParseContext implements Iterable{ /** Fork of {@link org.apache.lucene.document.Document} with additional functionality. */ public static class Document implements Iterable { @@ -171,6 +172,11 @@ public abstract class ParseContext { this.in = in; } + @Override + public Iterable nonRootDocuments() { + return in.nonRootDocuments(); + } + @Override public DocumentMapperParser docMapperParser() { return in.docMapperParser(); @@ -211,11 +217,6 @@ public abstract class ParseContext { return in.rootDoc(); } - @Override - public List docs() { - return in.docs(); - } - @Override public Document doc() { return in.doc(); @@ -280,6 +281,11 @@ public abstract class ParseContext { public List getDynamicMappers() { return in.getDynamicMappers(); } + + @Override + public Iterator iterator() { + return in.iterator(); + } } public static class InternalParseContext extends ParseContext { @@ -309,9 +315,10 @@ public abstract class ParseContext { private long numNestedDocs; - private final List dynamicMappers; + private boolean docsReversed = false; + public InternalParseContext(@Nullable Settings indexSettings, DocumentMapperParser docMapperParser, DocumentMapper docMapper, SourceToParse source, XContentParser parser) { this.indexSettings = indexSettings; @@ -360,8 +367,7 @@ public abstract class ParseContext { return documents.get(0); } - @Override - public List docs() { + List docs() { return this.documents; } @@ -426,8 +432,35 @@ public abstract class ParseContext { public List getDynamicMappers() { return dynamicMappers; } + + @Override + public Iterable nonRootDocuments() { + if (docsReversed) { + throw new IllegalStateException("documents are already reversed"); + } + return documents.subList(1, documents.size()); + } + + void postParse() { + // reverse the order of docs for nested docs support, parent should be last + if (documents.size() > 1) { + docsReversed = true; + Collections.reverse(documents); + } + } + + @Override + public Iterator iterator() { + return documents.iterator(); + } } + /** + * Returns an Iterable over all non-root documents. If there are no non-root documents + * the iterable will return an empty iterator. + */ + public abstract Iterable nonRootDocuments(); + public abstract DocumentMapperParser docMapperParser(); /** @@ -506,8 +539,6 @@ public abstract class ParseContext { public abstract Document rootDoc(); - public abstract List docs(); - public abstract Document doc(); protected abstract void addDoc(Document doc); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java index a22b9da7da3..76753496f46 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java @@ -157,10 +157,6 @@ public class RoutingFieldMapper extends MetadataFieldMapper { super.parse(context); } - @Override - public void postParse(ParseContext context) throws IOException { - } - @Override public Mapper parse(ParseContext context) throws IOException { // no need ot parse here, we either get the routing in the sourceToParse diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java index 197d5557363..ac3ffe46272 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java @@ -253,11 +253,9 @@ public class SeqNoFieldMapper extends MetadataFieldMapper { // we share the parent docs fields to ensure good compression SequenceIDFields seqID = context.seqID(); assert seqID != null; - int numDocs = context.docs().size(); final Version versionCreated = context.mapperService().getIndexSettings().getIndexVersionCreated(); final boolean includePrimaryTerm = versionCreated.before(Version.V_6_1_0); - for (int i = 1; i < numDocs; i++) { - final Document doc = context.docs().get(i); + for (Document doc : context.nonRootDocuments()) { doc.add(seqID.seqNo); doc.add(seqID.seqNoDocValue); if (includePrimaryTerm) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java index dafbb2e82ac..f2090613c09 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java @@ -216,10 +216,6 @@ public class SourceFieldMapper extends MetadataFieldMapper { super.parse(context); } - @Override - public void postParse(ParseContext context) throws IOException { - } - @Override public Mapper parse(ParseContext context) throws IOException { // nothing to do here, we will call it in pre parse @@ -228,32 +224,23 @@ public class SourceFieldMapper extends MetadataFieldMapper { @Override protected void parseCreateField(ParseContext context, List fields) throws IOException { - if (!enabled) { - return; - } - if (!fieldType().stored()) { - return; - } BytesReference source = context.sourceToParse().source(); - // Percolate and tv APIs may not set the source and that is ok, because these APIs will not index any data - if (source == null) { - return; + if (enabled && fieldType().stored() && source != null) { + // Percolate and tv APIs may not set the source and that is ok, because these APIs will not index any data + if (filter != null) { + // we don't update the context source if we filter, we want to keep it as is... + Tuple> mapTuple = + XContentHelper.convertToMap(source, true, context.sourceToParse().getXContentType()); + Map filteredSource = filter.apply(mapTuple.v2()); + BytesStreamOutput bStream = new BytesStreamOutput(); + XContentType contentType = mapTuple.v1(); + XContentBuilder builder = XContentFactory.contentBuilder(contentType, bStream).map(filteredSource); + builder.close(); + source = bStream.bytes(); + } + BytesRef ref = source.toBytesRef(); + fields.add(new StoredField(fieldType().name(), ref.bytes, ref.offset, ref.length)); } - - if (filter != null) { - // we don't update the context source if we filter, we want to keep it as is... - Tuple> mapTuple = - XContentHelper.convertToMap(source, true, context.sourceToParse().getXContentType()); - Map filteredSource = filter.apply(mapTuple.v2()); - BytesStreamOutput bStream = new BytesStreamOutput(); - XContentType contentType = mapTuple.v1(); - XContentBuilder builder = XContentFactory.contentBuilder(contentType, bStream).map(filteredSource); - builder.close(); - - source = bStream.bytes(); - } - BytesRef ref = source.toBytesRef(); - fields.add(new StoredField(fieldType().name(), ref.bytes, ref.offset, ref.length)); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java index ccc49e9b9ea..36bd4b137cf 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java @@ -269,10 +269,6 @@ public class TypeFieldMapper extends MetadataFieldMapper { super.parse(context); } - @Override - public void postParse(ParseContext context) throws IOException { - } - @Override public Mapper parse(ParseContext context) throws IOException { // we parse in pre parse diff --git a/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java index bedb98e2126..ef3c63f4889 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java @@ -128,8 +128,7 @@ public class VersionFieldMapper extends MetadataFieldMapper { // that don't have the field. This is consistent with the default value for efficiency. Field version = context.version(); assert version != null; - for (int i = 1; i < context.docs().size(); i++) { - final Document doc = context.docs().get(i); + for (Document doc : context.nonRootDocuments()) { doc.add(version); } }