search

apache / drill

Apache Drill is a distributed MPP query layer for self describing data
https://drill.apache.org/
Apache License 2.0
1.95k stars 979 forks source link

DRILL-8415: Upgrade Jackson 2.14.3 → 2.16.1 #2866

Closed jnturton closed 10 months ago

jnturton commented 10 months ago

DRILL-8415: Upgrade Jackson 2.14.3 → 2.16.1

Description

The following should be investigated before merging.

There are some security focused enhancements including a new class called StreamReadConstraints. The defaults on StreamReadConstraints are pretty high but it is not inconceivable that some Drill users might need to relax them. Parsing large strings as numbers is sub-quadratic, thus the default limit of 1000 chars or bytes (depending on input context).

When the Drill team consider upgrading to Jackson 2.15 or above, you might also want to consider adding some way for users to configure the StreamReadConstraints.

Documentation

N/A

Testing

Unit tests pass.

Lceeba commented 10 months ago

Unsubscribe

On Wed, 3 Jan, 2024, 13:41 James Turton, @.***> wrote:

DRILL-8415 https://issues.apache.org/jira/browse/DRILL-8415: Upgrade Jackson 2.14.3 → 2.16.1 Description

The following should be investigated before merging.

There are some security focused enhancements including a new class called StreamReadConstraints. The defaults on StreamReadConstraints https://javadoc.io/static/com.fasterxml.jackson.core/jackson-core/2.15.0-rc1/com/fasterxml/jackson/core/StreamReadConstraints.html are pretty high but it is not inconceivable that some Drill users might need to relax them. Parsing large strings as numbers is sub-quadratic, thus the default limit of 1000 chars or bytes (depending on input context).

When the Drill team consider upgrading to Jackson 2.15 or above, you might also want to consider adding some way for users to configure the StreamReadConstraints.

Documentation

N/A Testing

Unit tests pass.

You can view, comment on, or merge this pull request online at:

https://github.com/apache/drill/pull/2866 Commit Summary

File Changes

(1 file https://github.com/apache/drill/pull/2866/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/apache/drill/pull/2866, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACZFXPXLDZDST7EC5F5TWEDYMUHDDAVCNFSM6AAAAABBLB6LK2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGA3DGNBWGIZTCOA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

jnturton commented 10 months ago

I starting adding congifuration support for the new StreamReadConstraints, first globally and then just in the JSON reader, but I got stopped by a sense of YAGNI. It's hard to imagine someone who will need something beyond the default values in Jackson and more configuration is more complexity that users must contend with. So my opinion at this point is that we should only add that configurability if someone asks for it...

cgivre commented 10 months ago

@jnturton This looks good however there is a merge conflict. Can you please resolve so that we can run the CI?

jnturton commented 10 months ago

I haven't rebased this yet in case we decide to squash the WIP commits that were merged into master. Once a decision is made either way this can be rebased and a CI run obtained.

cgivre commented 10 months ago

I haven't rebased this yet in case we decide to squash the WIP commits that were merged into master. Once a decision is made either way this can be rebased and a CI run obtained.

I'm fine with leaving the WIP commits as long as we don't make a habit out of it. It's probably more of a hassle to undo the PR, squash the commits and re-merge them.