Upgrade to Jackson 2.9.2 #27032

tlrx · 2017-10-17T12:02:10Z

This PR updates Jackson to version 2.9.2.

event when it's closed

tlrx · 2017-10-19T11:35:47Z

core/src/main/java/org/elasticsearch/common/io/FastStringReader.java

@@ -49,8 +47,9 @@ public FastStringReader(String s) {
     * Check to make sure that the stream has not been closed
     */
    private void ensureOpen() throws IOException {
-        if (length == -1)
+        if (closed) {


FastStringReader uses a length equal to -1 to indicate that the reader has been closed. But once closed, the CharSequence's length() method reports an invalid value making use of subSequence(int,int) method fail. I fixed that by adding a closed flag instead of relying on length value.

It has been caught by tests, because Jackson 2.9.2 is more descriptive than 2.8.6 when reporting JSON location in exception messages and uses CharSequence's length() method here

Makes sense to me.

tlrx · 2017-10-19T11:36:31Z

@nik9000 Thanks for review, but the PR was in WIP. I changed few things, would you like to have another look?

jasontedor

LGTM. Will this also be ported to 6.x?

nik9000 · 2017-10-19T13:02:36Z

core/src/main/java/org/elasticsearch/common/io/FastStringReader.java

@@ -49,8 +47,9 @@ public FastStringReader(String s) {
     * Check to make sure that the stream has not been closed
     */
    private void ensureOpen() throws IOException {
-        if (length == -1)
+        if (closed) {


Makes sense to me.

Upgrade to Jackson 2.9.2 and also use a boolean `closed` flag to indicate that a FastStringReader instance is closed, so that length is still correctly reported after the reader is closed.

tlrx · 2017-10-19T13:59:09Z

Will this also be ported to 6.x?

Yes

tlrx · 2017-10-19T14:03:35Z

Thanks @nik9000 and @jasontedor.

This reverts commit 0b9acc5.

tlrx · 2017-10-20T06:30:06Z

This pull request passed on CI here but now it is merged it is causing different types of issues on CI, so I reverted it (463e7e6 and 72b9859) until I find the cause of the errors.

tlrx · 2017-10-31T14:41:10Z

After some investigation, this upgrade caused various failures because of two changes in Jackson 2.9.2:

Smile

In Jackson 2.9.2 the SMILE format is now part of the jackson-dataformats-binary project. The code has been factorized multiple times but the consequence is that the SmileParser now extends SmileParserBase (instead of ParserBase in 2.8) and correctly parses back float values.

Some elasticsearch tests like GetResultTests or GetResponseTests rely on a special treatment for float values (see RandomObjects) and they fail. This is good since the float values are now correctly parsed back but these tests have to be adapted for Jackson 2.9.

Yaml

Jackson 2.9.2 now depends on snakeyaml 1.18 (instead of 1.15) and I suspect a performance degradation in the YAML parser when reading large text values.

The entry point is in the org.yaml.snakeyaml.scanner.ScannerImpl#scanFlowScalarNonSpaces(boolean, Mark) method:

while (Constant.NULL_BL_T_LINEBR.hasNo(reader.peek(length), "\'\"\\")) {
  length++;
}

This method uses the peek() method of a org.yaml.snakeyaml.reader.StreamReader instance and this method has been changed in 1.18 to return a int instead of a char, but the new implementation is less efficient for large values.

The consequence is that parsing some values take a lot of time, it slows down all REST integration tests and some tests just time out during execution (like ingest-attachement).

I'm currently testing Jackson 2.8.10 in #27230

While it's not possible to upgrade the Jackson dependencies to their latest versions yet (see #27032 (comment) for more) it's still possible to upgrade to the latest 2.8.x version.

msymons · 2017-11-20T11:24:09Z

Will there be a backport for v5.6.x?

This would address deserialization vulnerability CVE-2017-7525 affecting transitive dependency jackson-databind v2.8.6

tlrx · 2017-11-20T11:38:14Z

@msymons This pull request has been reverted. What you're looking for is #27361 which has been backported to the current 5.6.x maintenance branch but not released.

msymons · 2017-11-20T14:59:58Z

@tlrx. My apologies… somehow, I had ended up making my comment above in the wrong browser tab! I had seen the reversion that you refer to and had meant to add my comment to #27230 (which does not (yet) reference v5.6.5 ). Is not #27361 related just to AWS SDK?

jasontedor · 2017-11-20T15:05:47Z

Is not #27361 related just to AWS SDK?

@msymons The databind artifact is only used as transitive dependencies in discovery-ec2, repository-s3, and ingest-geoip. From #27361 we have upgraded that dependency to a patched version for the plugins that use the AWS SDK. For ingest-geoip, we have to wait until core has its core Jackson dependency upgraded (at this PR was doing). The usage of jackson-databind in that plugin is tightly scoped. Otherwise, there are no other uses for databind within Elasticsearch.

tlrx · 2018-06-14T06:49:24Z

I tested Jackson 2.9.6 and I reported this issue in jackson-dataformats-text. Binary values can be encoded in YAML but not decoded when it exceeds a given size.

tlrx · 2018-10-23T13:35:40Z

I tested Jackson 2.9.7 and still the same issue.

nkomissar · 2019-03-18T13:55:53Z

hi tlrx, I can see the issue you've found is fixed in 2.9.9/2.10.0, does it make possible upgrading Jackson?
In my company, we would like to use elasticsearch but it's blocked by Security due to vulnerabilities in 2.8.11.

Thanks!

jasontedor · 2019-03-18T14:10:21Z

I want to make explicit here that Elasticsearch is not exposed to the vulnerabilities in 2.8.11.

nkomissar · 2019-03-18T14:13:19Z

Can you please elaborate or provide some link where I can find more info to satisfy our Security? This will be greatly appreciated!

jasontedor · 2019-03-18T14:22:16Z

Elasticsearch does not use any of the functionality related to the exploits, we are not exposed to any of the issues related to polymorphic deserialization at all. Does that help?

nkomissar · 2019-03-18T14:50:10Z

Thank you. I will talk to the AppSec folks and will see :)

msymons · 2019-06-17T15:26:15Z

@jasontedor

Elasticsearch does not use any of the functionality related to the exploits, we are not exposed to any of the issues related to polymorphic deserialization at all. Does that help?

I totally understand that Elasticsearch has no exposure to any of the jackson-databind issues. However, projects using Elasticsearch and other dependencies that ARE vulnerable to the jackson-databind issues (meaning that the other dependencies must use jackson 2.9.9) does make for problems with dependency management.

Additionally, using out of date components does lead to operational-risk warnings from analysers such as Nexus-IQ

However, I would hope that updating to 2.9.9 would provide its own benefits in terms of functionality, etc. But I am not a developer and so cannot opine too much on that aspect.

tlrx added :Core/Infra/Core Core issues without another label review v7.0.0 labels Oct 17, 2017

Upgrade to Jackson 2.9.2

8548340

tlrx force-pushed the upgrade-jackson branch from 9084f8d to 8548340 Compare October 17, 2017 12:42

tlrx added WIP and removed review labels Oct 17, 2017

Fix unit test

4235499

nik9000 approved these changes Oct 17, 2017

View reviewed changes

Adapt unit tests, change FastStringReader to return the right length

f32a581

event when it's closed

tlrx commented Oct 19, 2017

View reviewed changes

tlrx added review and removed WIP labels Oct 19, 2017

jasontedor approved these changes Oct 19, 2017

View reviewed changes

nik9000 approved these changes Oct 19, 2017

View reviewed changes

tlrx merged commit 0b9acc5 into elastic:master Oct 19, 2017

tlrx added the v6.1.0 label Oct 19, 2017

tlrx deleted the upgrade-jackson branch October 19, 2017 13:59

tlrx added a commit that referenced this pull request Oct 20, 2017

Revert "Upgrade to Jackson 2.9.2 (#27032)"

463e7e6

This reverts commit 0b9acc5.

tlrx added a commit that referenced this pull request Oct 20, 2017

Revert "Upgrade to Jackson 2.9.2 (#27032)"

72b9859

This reverts commit 0b9acc5.

clintongormley removed v6.1.0 v7.0.0 labels Oct 30, 2017

javanna mentioned this pull request Oct 31, 2017

Fix rare failure in docs tests #27183

Closed

tlrx mentioned this pull request Nov 2, 2017

Upgrade to Jackson 2.8.10 #27230

Merged

tlrx added a commit that referenced this pull request Nov 6, 2017

Upgrade to Jackson 2.8.10 (#27230)

43e7a4a

While it's not possible to upgrade the Jackson dependencies to their latest versions yet (see #27032 (comment) for more) it's still possible to upgrade to the latest 2.8.x version.

tlrx added a commit that referenced this pull request Nov 6, 2017

Upgrade to Jackson 2.8.10 (#27230)

202c67c

While it's not possible to upgrade the Jackson dependencies to their latest versions yet (see #27032 (comment) for more) it's still possible to upgrade to the latest 2.8.x version.

martijnvg mentioned this pull request Jan 4, 2018

Enable ASN support for Ingest GeoIP plugin. #27958

Merged

msymons mentioned this pull request May 2, 2018

Update Jackson version to 2.8.11.2 #30352

Closed

tlrx mentioned this pull request Aug 8, 2018

CORE: Upgrade to Jackson 2.8.11 #32670

Merged

tlrx mentioned this pull request Sep 5, 2019

Update jackson #46367

Closed

jasontedor mentioned this pull request Mar 12, 2020

Upgrade to Jackson 2.10.3 #53523

Merged

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket elastic/elasticsearch-net#4525

Closed

38 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Jackson 2.9.2 #27032

Upgrade to Jackson 2.9.2 #27032

tlrx commented Oct 17, 2017

tlrx Oct 19, 2017

tlrx Oct 19, 2017

nik9000 Oct 19, 2017

tlrx commented Oct 19, 2017

jasontedor left a comment

nik9000 Oct 19, 2017

tlrx commented Oct 19, 2017

tlrx commented Oct 19, 2017

tlrx commented Oct 20, 2017

tlrx commented Oct 31, 2017 •

edited

Loading

msymons commented Nov 20, 2017

tlrx commented Nov 20, 2017

msymons commented Nov 20, 2017

jasontedor commented Nov 20, 2017

tlrx commented Jun 14, 2018

tlrx commented Oct 23, 2018

nkomissar commented Mar 18, 2019

jasontedor commented Mar 18, 2019

nkomissar commented Mar 18, 2019

jasontedor commented Mar 18, 2019

nkomissar commented Mar 18, 2019

msymons commented Jun 17, 2019

Upgrade to Jackson 2.9.2 #27032

Upgrade to Jackson 2.9.2 #27032

Conversation

tlrx commented Oct 17, 2017

tlrx Oct 19, 2017

Choose a reason for hiding this comment

tlrx Oct 19, 2017

Choose a reason for hiding this comment

nik9000 Oct 19, 2017

Choose a reason for hiding this comment

tlrx commented Oct 19, 2017

jasontedor left a comment

Choose a reason for hiding this comment

nik9000 Oct 19, 2017

Choose a reason for hiding this comment

tlrx commented Oct 19, 2017

tlrx commented Oct 19, 2017

tlrx commented Oct 20, 2017

tlrx commented Oct 31, 2017 • edited Loading

Smile

Yaml

msymons commented Nov 20, 2017

tlrx commented Nov 20, 2017

msymons commented Nov 20, 2017

jasontedor commented Nov 20, 2017

tlrx commented Jun 14, 2018

tlrx commented Oct 23, 2018

nkomissar commented Mar 18, 2019

jasontedor commented Mar 18, 2019

nkomissar commented Mar 18, 2019

jasontedor commented Mar 18, 2019

nkomissar commented Mar 18, 2019

msymons commented Jun 17, 2019

tlrx commented Oct 31, 2017 •

edited

Loading