Add StreamWriteConstraints with a nesting depth check by pjfanning · Pull Request #1055 · FasterXML/jackson-core (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- relates to Add configurable processing limits for JSON generator (StreamWriteConstraints) #1048
- open for discussion - still lots of work to do
- equivalent of StreamReadConstraints
- initially, only max nesting depth is tracked
- the changes in this PR to get the Filtering Delegate Generator to check the depth are probably overkill - the underlying JsonGenerator (the one being delegated to) is probably tracking already
- and tests, of course
@cowtowncoder this code does not work yet because of a quirk in how JsonWriteContext works.
The nestingDepth is tracked if you create a JsonWriteContext with a parent context (inside the null check below).
But JsonWriteContext has the reset method too. Is it ok if I hack this to update the nestingDepth too? It is a pain because nestingDepth is final and that final marker will need to be removed. The reset will mutate the nestingDepth and not in a nice way - makes the code so much harder to test.
Is there any hope that this reset behaviour could be removed? So, we would instead create a new JsonWriteContext with a parent instead.
public JsonWriteContext createChildArrayContext() {
JsonWriteContext ctxt = _child;
if (ctxt == null) {
_child = ctxt = new JsonWriteContext(TYPE_ARRAY, this,
(_dups == null) ? null : _dups.child());
return ctxt;
}
return ctxt.reset(TYPE_ARRAY);
}
This was referenced
Jun 18, 2023
+1 for working on this. I will not have time to engage on this for next 2 weeks or so (will try to send an email notification about my vacation), except for today and tomorrow.
But maybe we can get things started.
@cowtowncoder I have the some tests working now and the issue I was hitting with JsonWriteContext was a bug in my code.
If I remove the changes in the Filtering Delegate Generator (because they are overkill), would it be possible to get this merged today/tomorrow? That would make it easier for me to test out the changes in jackson-databind and other jackson modules. Whatever way it falls out, we can complete the extra bits after your vacation.
[DRAFT] add StreamWriteConstraints add StreamWriteConstraints with a nesting depth check
@cowtowncoder this code does not work yet because of a quirk in how JsonWriteContext works.
The nestingDepth is tracked if you create a JsonWriteContext with a parent context (inside the null check below).
But JsonWriteContext has the
resetmethod too.
reset() is only called to reuse an instance at same nesting level, so shouldn't it be safe to in that case actually ignore check call altogether? (since that nesting level must have been validated once before).
Or if not, just copy nesting depth.
Reset is NOT used for general purpose reuse but specifically case of avoiding allocation for a sibling of earlier nesting level. This is noted in Javadoc comments.
I may be missing something here so want to make sure I understand the problem.
@cowtowncoder this code does not work yet because of a quirk in how JsonWriteContext works.
The nestingDepth is tracked if you create a JsonWriteContext with a parent context (inside the null check below).
But JsonWriteContext has theresetmethod too.
reset()is only called to reuse an instance at same nesting level, so shouldn't it be safe to in that case actually ignore check call altogether? (since that nesting level must have been validated once before). Or if not, just copy nesting depth.Reset is NOT used for general purpose reuse but specifically case of avoiding allocation for a sibling of earlier nesting level. This is noted in Javadoc comments.
I may be missing something here so want to make sure I understand the problem.
I was debugging an issue and I misunderstood what was going on. The context reset was not the issue and it does not need to change.
I have the PR working with a few basic tests now.
@pjfanning Ok perfect. I just wanted to make sure you weren't blocked here. Plus role of reset() is not necessarily that clear without knowing the context (no pun intended)
@pjfanning Oh, one other thing -- I know it's a pain, but once you have something ready, is there any chance of merging things incrementally, to help merge to master (3.0)? Starting with plumbing/scaffolding, then implementation, then tests?
I know it's a hassle so you can decide if it's worth it.
Update TokenFilterContext.java
make stream write constraints accessible
Update FilteringGeneratorDelegate.java
police nesting depth in json generators
Update JsonGeneratorImpl.java
add test that should fail
fix test
test issue
more tests
revert changes in FilteringGeneratorDelegate
Update FilteringGeneratorDelegate.java
@cowtowncoder could you review this PR when you have time? There would be need of follow up PRs in jackson-databind and some of the other modules (XML, text formats, binary formats). I can do those after this is merged.
@pjfanning Yes, trying to go through backlog now that I am back; will try to get this (and 1056) reviewed ASAP.
Sorry, didn't get this done today; aiming for tomorrow.
add StreamWriteConstraints with a nesting depth check Add StreamWriteConstraints with a nesting depth check
This was referenced
Jul 8, 2023
Issue planned (at earliest) for 2.16
Issues related to limiting aspects of input/output that can be processed without exception
labels
8 tasks
Labels
Issue planned (at earliest) for 2.16
Issues related to limiting aspects of input/output that can be processed without exception