Skip to content

Commit 08d1749

Browse files
qwerty4030qwerty4030
committed
Fixed NPEs caused by requests without content.
REST handlers that require a body will throw an an ElasticsearchParseException "request body required". REST handlers that require a body OR source param will throw an ElasticsearchParseException "request body or source param required". Replaced asserts in BulkRequest parsing code with a more descriptive IllegalArgumentException if the line contains an empty object. Updated bulk REST test to verify an empty action line is rejected properly. Updated BulkRequestTests with randomized testing for an empty action line. Used try-with-resouces for XContentParser in AbstractBulkByQueryRestHandler.
1 parent 160a049 commit 08d1749

29 files changed

Lines changed: 306 additions & 93 deletions

File tree

‎client/client-benchmark-noop-api-plugin/src/main/java/org/elasticsearch/plugin/noop/action/bulk/RestNoopBulkAction.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,8 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
7373
}
7474
bulkRequest.timeout(request.paramAsTime("timeout", BulkShardRequest.DEFAULT_TIMEOUT));
7575
bulkRequest.setRefreshPolicy(request.param("refresh"));
76-
bulkRequest.add(request.content(), defaultIndex, defaultType, defaultRouting, defaultFields, null, defaultPipeline, null, true,
77-
request.getXContentType());
76+
bulkRequest.add(request.requiredContent(), defaultIndex, defaultType, defaultRouting, defaultFields,
77+
null, defaultPipeline, null, true, request.getXContentType());
7878

7979
// short circuit the call to the transport layer
8080
return channel -> {

‎core/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
import org.elasticsearch.common.unit.TimeValue;
4242
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
4343
import org.elasticsearch.common.xcontent.XContent;
44-
import org.elasticsearch.common.xcontent.XContentFactory;
4544
import org.elasticsearch.common.xcontent.XContentParser;
4645
import org.elasticsearch.common.xcontent.XContentType;
4746
import org.elasticsearch.index.VersionType;
@@ -300,10 +299,16 @@ public BulkRequest add(BytesReference data, @Nullable String defaultIndex, @Null
300299
if (token == null) {
301300
continue;
302301
}
303-
assert token == XContentParser.Token.START_OBJECT;
302+
if (token != XContentParser.Token.START_OBJECT) {
303+
throw new IllegalArgumentException("Malformed action/metadata line [" + line + "], expected "
304+
+ XContentParser.Token.START_OBJECT + " but found [" + token + "]");
305+
}
304306
// Move to FIELD_NAME, that's the action
305307
token = parser.nextToken();
306-
assert token == XContentParser.Token.FIELD_NAME;
308+
if (token != XContentParser.Token.FIELD_NAME) {
309+
throw new IllegalArgumentException("Malformed action/metadata line [" + line + "], expected "
310+
+ XContentParser.Token.FIELD_NAME + " but found [" + token + "]");
311+
}
307312
String action = parser.currentName();
308313

309314
String index = defaultIndex;

‎core/src/main/java/org/elasticsearch/rest/RestRequest.java‎

Lines changed: 32 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,18 @@ public final String path() {
136136

137137
public abstract BytesReference content();
138138

139+
/**
140+
* @return content of the request body or throw an exception if the body or content type is missing
141+
*/
142+
public final BytesReference requiredContent() {
143+
if (hasContent() == false) {
144+
throw new ElasticsearchParseException("request body is required");
145+
} else if (xContentType.get() == null) {
146+
throw new IllegalStateException("unknown content type");
147+
}
148+
return content();
149+
}
150+
139151
/**
140152
* Get the value of the header or {@code null} if not found. This method only retrieves the first header value if multiple values are
141153
* sent. Use of {@link #getAllHeaderValues(String)} should be preferred
@@ -329,12 +341,7 @@ public NamedXContentRegistry getXContentRegistry() {
329341
* {@link #contentOrSourceParamParser()} for requests that support specifying the request body in the {@code source} param.
330342
*/
331343
public final XContentParser contentParser() throws IOException {
332-
BytesReference content = content();
333-
if (content.length() == 0) {
334-
throw new ElasticsearchParseException("Body required");
335-
} else if (xContentType.get() == null) {
336-
throw new IllegalStateException("unknown content type");
337-
}
344+
BytesReference content = requiredContent(); // will throw exception if body or content type missing
338345
return xContentType.get().xContent().createParser(xContentRegistry, content);
339346
}
340347

@@ -364,11 +371,7 @@ public final boolean hasContentOrSourceParam() {
364371
*/
365372
public final XContentParser contentOrSourceParamParser() throws IOException {
366373
Tuple<XContentType, BytesReference> tuple = contentOrSourceParam();
367-
BytesReference content = tuple.v2();
368-
if (content.length() == 0) {
369-
throw new ElasticsearchParseException("Body required");
370-
}
371-
return tuple.v1().xContent().createParser(xContentRegistry, content);
374+
return tuple.v1().xContent().createParser(xContentRegistry, tuple.v2());
372375
}
373376

374377
/**
@@ -377,10 +380,10 @@ public final XContentParser contentOrSourceParamParser() throws IOException {
377380
* back to the user when there isn't request content.
378381
*/
379382
public final void withContentOrSourceParamParserOrNull(CheckedConsumer<XContentParser, IOException> withParser) throws IOException {
380-
Tuple<XContentType, BytesReference> tuple = contentOrSourceParam();
381-
BytesReference content = tuple.v2();
382-
XContentType xContentType = tuple.v1();
383-
if (content.length() > 0) {
383+
if (hasContentOrSourceParam()) {
384+
Tuple<XContentType, BytesReference> tuple = contentOrSourceParam();
385+
BytesReference content = tuple.v2();
386+
XContentType xContentType = tuple.v1();
384387
try (XContentParser parser = xContentType.xContent().createParser(xContentRegistry, content)) {
385388
withParser.accept(parser);
386389
}
@@ -390,28 +393,26 @@ public final void withContentOrSourceParamParserOrNull(CheckedConsumer<XContentP
390393
}
391394

392395
/**
393-
* Get the content of the request or the contents of the {@code source} param. Prefer {@link #contentOrSourceParamParser()} or
394-
* {@link #withContentOrSourceParamParserOrNull(CheckedConsumer)} if you need a parser.
396+
* Get the content of the request or the contents of the {@code source} param or throw an exception if both are missing.
397+
* Prefer {@link #contentOrSourceParamParser()} or {@link #withContentOrSourceParamParserOrNull(CheckedConsumer)} if you need a parser.
395398
*/
396399
public final Tuple<XContentType, BytesReference> contentOrSourceParam() {
397-
if (hasContent()) {
398-
if (xContentType.get() == null) {
399-
throw new IllegalStateException("unknown content type");
400-
}
401-
return new Tuple<>(xContentType.get(), content());
400+
if (hasContentOrSourceParam() == false) {
401+
throw new ElasticsearchParseException("request body or source parameter is required");
402+
} else if (hasContent()) {
403+
return new Tuple<>(xContentType.get(), requiredContent());
402404
}
403-
404405
String source = param("source");
405406
String typeParam = param("source_content_type");
406-
if (source != null && typeParam != null) {
407-
BytesArray bytes = new BytesArray(source);
408-
final XContentType xContentType = parseContentType(Collections.singletonList(typeParam));
409-
if (xContentType == null) {
410-
throw new IllegalStateException("Unknown value for source_content_type [" + typeParam + "]");
411-
}
412-
return new Tuple<>(xContentType, bytes);
407+
if (source == null || typeParam == null) {
408+
throw new IllegalStateException("source and source_content_type parameters are required");
409+
}
410+
BytesArray bytes = new BytesArray(source);
411+
final XContentType xContentType = parseContentType(Collections.singletonList(typeParam));
412+
if (xContentType == null) {
413+
throw new IllegalStateException("Unknown value for source_content_type [" + typeParam + "]");
413414
}
414-
return new Tuple<>(XContentType.JSON, BytesArray.EMPTY);
415+
return new Tuple<>(xContentType, bytes);
415416
}
416417

417418
/**

‎core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestPutStoredScriptAction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client
5858
lang = null;
5959
}
6060

61-
BytesReference content = request.content();
61+
BytesReference content = request.requiredContent();
6262

6363
if (lang != null) {
6464
deprecationLogger.deprecated(

‎core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestPutIndexTemplateAction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
5757
putRequest.masterNodeTimeout(request.paramAsTime("master_timeout", putRequest.masterNodeTimeout()));
5858
putRequest.create(request.paramAsBoolean("create", false));
5959
putRequest.cause(request.param("cause", ""));
60-
putRequest.source(request.content(), request.getXContentType());
60+
putRequest.source(request.requiredContent(), request.getXContentType());
6161
return channel -> client.admin().indices().putTemplate(putRequest, new AcknowledgedRestListener<>(channel));
6262
}
6363

‎core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestPutMappingAction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public RestPutMappingAction(Settings settings, RestController controller) {
6464
public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
6565
PutMappingRequest putMappingRequest = putMappingRequest(Strings.splitStringByCommaToArray(request.param("index")));
6666
putMappingRequest.type(request.param("type"));
67-
putMappingRequest.source(request.content(), request.getXContentType());
67+
putMappingRequest.source(request.requiredContent(), request.getXContentType());
6868
putMappingRequest.updateAllTypes(request.paramAsBoolean("update_all_types", false));
6969
putMappingRequest.timeout(request.paramAsTime("timeout", putMappingRequest.timeout()));
7070
putMappingRequest.masterNodeTimeout(request.paramAsTime("master_timeout", putMappingRequest.masterNodeTimeout()));

‎core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestUpdateSettingsAction.java‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -54,18 +54,16 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
5454
updateSettingsRequest.indicesOptions(IndicesOptions.fromRequest(request, updateSettingsRequest.indicesOptions()));
5555

5656
Map<String, Object> settings = new HashMap<>();
57-
if (request.hasContent()) {
58-
try (XContentParser parser = request.contentParser()) {
59-
Map<String, Object> bodySettings = parser.map();
60-
Object innerBodySettings = bodySettings.get("settings");
61-
// clean up in case the body is wrapped with "settings" : { ... }
62-
if (innerBodySettings instanceof Map) {
63-
@SuppressWarnings("unchecked")
64-
Map<String, Object> innerBodySettingsMap = (Map<String, Object>) innerBodySettings;
65-
settings.putAll(innerBodySettingsMap);
66-
} else {
67-
settings.putAll(bodySettings);
68-
}
57+
try (XContentParser parser = request.contentParser()) {
58+
Map<String, Object> bodySettings = parser.map();
59+
Object innerBodySettings = bodySettings.get("settings");
60+
// clean up in case the body is wrapped with "settings" : { ... }
61+
if (innerBodySettings instanceof Map) {
62+
@SuppressWarnings("unchecked")
63+
Map<String, Object> innerBodySettingsMap = (Map<String, Object>) innerBodySettings;
64+
settings.putAll(innerBodySettingsMap);
65+
} else {
66+
settings.putAll(bodySettings);
6967
}
7068
}
7169
updateSettingsRequest.settings(settings);

‎core/src/main/java/org/elasticsearch/rest/action/document/RestBulkAction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
8686
}
8787
bulkRequest.timeout(request.paramAsTime("timeout", BulkShardRequest.DEFAULT_TIMEOUT));
8888
bulkRequest.setRefreshPolicy(request.param("refresh"));
89-
bulkRequest.add(request.content(), defaultIndex, defaultType, defaultRouting, defaultFields,
89+
bulkRequest.add(request.requiredContent(), defaultIndex, defaultType, defaultRouting, defaultFields,
9090
defaultFetchSourceContext, defaultPipeline, null, allowExplicitIndex, request.getXContentType());
9191

9292
return channel -> client.bulk(bulkRequest, new RestStatusToXContentListener<>(channel));

‎core/src/main/java/org/elasticsearch/rest/action/document/RestIndexAction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
6464
indexRequest.routing(request.param("routing"));
6565
indexRequest.parent(request.param("parent"));
6666
indexRequest.setPipeline(request.param("pipeline"));
67-
indexRequest.source(request.content(), request.getXContentType());
67+
indexRequest.source(request.requiredContent(), request.getXContentType());
6868
indexRequest.timeout(request.paramAsTime("timeout", IndexRequest.DEFAULT_TIMEOUT));
6969
indexRequest.setRefreshPolicy(request.param("refresh"));
7070
indexRequest.version(RestActions.parseVersion(request));

‎core/src/test/java/org/elasticsearch/action/bulk/BulkRequestTests.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,31 @@ public void testSimpleBulk10() throws Exception {
177177
assertThat(bulkRequest.numberOfActions(), equalTo(9));
178178
}
179179

180+
public void testBulkEmptyObject() throws Exception {
181+
String bulkIndexAction = "{ \"index\":{\"_index\":\"test\",\"_type\":\"type1\",\"_id\":\"1\"} }\r\n";
182+
String bulkIndexSource = "{ \"field1\" : \"value1\" }\r\n";
183+
String emptyObject = "{}\r\n";
184+
StringBuilder bulk = new StringBuilder();
185+
int emptyLine;
186+
if (randomBoolean()) {
187+
bulk.append(emptyObject);
188+
emptyLine = 1;
189+
} else {
190+
int actions = randomIntBetween(1, 10);
191+
int emptyAction = randomIntBetween(1, actions);
192+
emptyLine = emptyAction * 2 - 1;
193+
for (int i = 1; i <= actions; i++) {
194+
bulk.append(i == emptyAction ? emptyObject : bulkIndexAction);
195+
bulk.append(randomBoolean() ? emptyObject : bulkIndexSource);
196+
}
197+
}
198+
String bulkAction = bulk.toString();
199+
BulkRequest bulkRequest = new BulkRequest();
200+
IllegalArgumentException exc = expectThrows(IllegalArgumentException.class,
201+
() -> bulkRequest.add(bulkAction.getBytes(StandardCharsets.UTF_8), 0, bulkAction.length(), null, null, XContentType.JSON));
202+
assertThat(exc.getMessage(), containsString("Malformed action/metadata line [" + emptyLine + "], expected FIELD_NAME but found [END_OBJECT]"));
203+
}
204+
180205
// issue 7361
181206
public void testBulkRequestWithRefresh() throws Exception {
182207
BulkRequest bulkRequest = new BulkRequest();

0 commit comments

Comments
 (0)