Fix infinite recursion on some schemas when setting defaults (#359) #365

erayd · 2017-03-01T11:23:24Z

Made this PR to collaborate with @mathroc on #359. The root cause has been located and is not a bug, but the recursion needs to be mitigated, as infinite recursion of a validator is in violation of the spec.

Will backport the fix to 5.2.0 once this PR is merged.

Changes Relating to #359

Add CHECK_MODE_ONLY_REQUIRED_DEFAULTS
Don't expand nested defaults (fixes infinite loop with Constraint::CHECK_MODE_APPLY_DEFAULTS #359)

Other Changes

Seeing as I'm doing this defaults-related PR anyway, I figured I might as well do a little tidying-up of the defaults code, plus fix any other bugs I run into while tracking this one down.

Fix another bug with trying to fetch an empty URI
Refactor defaults to use LooseTypeCheck where appropriate
Move default-setting into its own method
Rename one of the defaults variables for clarity
Add tests:
- Ensure non-container values aren't treated like containers;
- Infinite recursion of default objects via $ref
- Default values do not override defined null
- Default values of null are correctly applied
Fix second test case for DefaultPropertiesTest::testLeaveBasicTypesAlone
Fix applying default values of null / not overwriting null (fixes Handling of nulls when applying defaults #377)

shmax · 2017-03-06T00:43:22Z

LGTM

mathroc · 2017-03-06T09:30:01Z

hmm in fact there's still an issue when trying to validate a schema, this fails :

<?php

include_once __DIR__."/../vendor/autoload.php";

$validator = new \JsonSchema\Validator();

$data = (object)["propertyOne" => (object)[]];
$schema = (object)['$ref' => 'http://json-schema.org/draft-04/schema'];

echo __LINE__, PHP_EOL;

$validator->validate($data, $schema);
var_dump($validator->isValid());
echo __LINE__, PHP_EOL;

$validator->validate(
    $data,
    $schema,
    \JsonSchema\Constraints\Constraint::CHECK_MODE_APPLY_DEFAULTS | \JsonSchema\Constraints\Constraint::CHECK_MODE_REQUIRE_DEFAULTS
);

var_dump($validator->isValid());
echo __LINE__, PHP_EOL;

$ php src/test.php
21
bool(true)
25

Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 32 bytes) in /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/Constraint.php on line 205

mathroc · 2017-03-06T09:49:37Z

I think && !$this->factory->getConfig(self::CHECK_MODE_REQUIRE_DEFAULTS) should be added to the ifs for array and leaf

erayd · 2017-03-06T09:50:10Z

Thanks - I'll get onto this in the morning; it's 11pm here :-).

Note that I have not yet implemented a recursion fix in this PR - that is coming.

erayd · 2017-03-06T19:57:58Z

I think && !$this->factory->getConfig(self::CHECK_MODE_REQUIRE_DEFAULTS) should be added to the ifs for array and leaf

Bit more complex than that - need to check whether they are actually required - but god knows what I was thinking when I committed the required-defaults-only feature it in its current state; it's more than a little bit incomplete!

erayd · 2017-03-06T21:24:25Z

@mathroc Sorted. I also renamed the option to CHECK_MODE_ONLY_REQUIRED_DEFAULTS, in the interests of being obvious to the end user.

Still need to address the recursion thing.

mathroc · 2017-03-06T21:41:01Z

src/JsonSchema/Constraints/UndefinedConstraint.php

+                return true;
+            }
+            // draft-03 required is set
+            if (isset($definition->required) && !is_array($definition->required) && $definition->required) {


$definition->required can be something else than a boolean ?

Yup - in draft-04, it's an array of strings.

mathroc · 2017-03-06T21:51:46Z

looks neat, I'll test it in the morning; it's 11pm here ;)

Still need to address the recursion thing.

if I got this right, I don't think there is a recursion thing when CHECK_MODE_ONLY_REQUIRED_DEFAULTS is set. for the other case, it does not seems that easy, good luck!

erayd · 2017-03-06T21:57:09Z

@mathroc Sounds like we are in timezones exactly 12 hours apart! You're in the UK?

if I got this right, I don't think there is a recursion thing when CHECK_MODE_ONLY_REQUIRED_DEFAULTS is set. for the other case, it does not seems that easy, good luck!

There could be if you wrote a schema that required the looping defaults. Plus, it turns out that the spec says that validators aren't allowed to infinitely recurse, so I still have to deal with it. Thanks for your good wishes! :-)

mathroc · 2017-03-06T22:18:49Z

@mathroc Sounds like we are in timezones exactly 12 hours apart! You're in the UK?

France, you're in NZ ?

There could be if you wrote a schema that required the looping defaults

indeed. I wonder: is there a "finite" json document that would be valid according to such a schema ? (even without applying defaults)
maybe if there is an exit option with a $anyOf. so now I'm wondering what would happen if there is default values in $anyOf'd schemas, would they all be applied ? wouldn't that potentially create invalid documents as a result ? eg:

{
	"$anyOf": [{
		"type" : "object",
		"properties": {
			"a": {
				"type": "string",
				"default": "A"
			}
		},
		"required" : ["a"],
		"additionalProperties": false
	},{
		"type" : "object",
		"properties": {
			"b": {
				"type": "string",
				"default": "B"
			}
		},
		"required" : ["b"],
		"additionalProperties": false
	}]
}

erayd · 2017-03-06T22:34:20Z

@mathroc

France, you're in NZ ?

Yep, I'm a Kiwi :-).

Re your schema question - as I understand it, the schema you have quoted above would be satisfied by either an object containing only a string b, or an object containing only a string a.

mathroc · 2017-03-06T22:57:35Z

yes but what would be the json document if called with en empty object and the CHECK_MODE_APPLY_DEFAULTS flag ?

it should be either an object containing only a string b, or an object containing only a string a, yes. but which one? I've not tested it, but it might also return an object with both a and b set and (maybe) mark it as invalid. I guess the question is: once a $anyOf schema is validated, does the lib tries the second schema anyway ? and the answer seems to be here : https://github.com/justinrainbow/json-schema/blob/ef3ee8356a2ea9bb44135787dab2f1491dcbb9f7/src/JsonSchema/Constraints/UndefinedConstraint.php#L249 which means it will return an object with a set (and I just realized I forgot the default in the previous exemple - fixed)

so now, there is valid json documents for this schema :

{
	"$anyOf": [{
		"type" : "object",
		"properties": {
			"a": {
				"$ref": "#",
				"default": {}
			}
		},
		"required" : ["a"],
		"additionalProperties": false
	},{
		"type" : "object",
		"additionalProperties": false
	}]
}

but the current version of lib would recurse infinitely. what should the fixed version return ? {"a": {}} or {} ? I would prefer {} but I can't think of an easy solution, looks like we would have to first try each schema and if none is valid, retry with applied defaults…

and I think that's a schema would have the issue even with CHECK_MODE_ONLY_REQUIRED_DEFAULTS at the moment

ps: sorry if I'm being too talkative

erayd · 2017-03-06T23:03:03Z

I'll have to think on it - let me fix the recursion first!

erayd · 2017-03-06T23:03:33Z

And no, you're not being too talkative - you're making me think, which is a good thing here.

erayd · 2017-03-06T23:58:00Z

@mathroc Think I've got the recursion sorted :-). Your example above will return {"a":{}}.

@everyone Can anyone else think of an example that will infinitely recurse? Object recursion should be impossible, but if there's a way to do it with arrays...

erayd · 2017-03-07T00:33:59Z

Never mind, my brain cooperated and threw up an array recursion:

{
    "items": [
        {
            "$ref": "#",
            "default": []
        }
    ]
}

Time to deal with the array cases now...

erayd · 2017-03-07T00:42:47Z

Array recursion is now also fixed.

@mathroc @shmax Please review.

erayd · 2017-03-07T18:36:21Z

Rebased.

erayd · 2017-03-08T02:15:02Z

Travis fail is Travis' fault; the build for PHP-7.1 never ran. Tests should all be passing.

shmax · 2017-03-08T04:13:29Z

I don't entirely follow all this, but I see no obvious black holes. LGTM

erayd · 2017-03-08T04:15:29Z

@shmax Which part(s) are you finding confusing? I tried to make it obvious what I was doing, but if it's problematic for you to follow, maybe I wasn't clear enough - it might be worth my while to add some more comments in the code.

shmax · 2017-03-08T04:47:43Z

Naw, I meant the meta discussion regarding recursion and the finer points of the spec and such; had my attention elsewhere while it was going on and I didn't follow along as closely as I should have. I trust that you guys have it properly sorted, and the code looks fine. LGTM

erayd · 2017-03-08T04:48:48Z

Gotcha - thanks :-)

mathroc · 2017-03-10T14:51:00Z

I finally took the time to look at it and test it, everything looks fine to me, LGTM!

mathroc · 2017-03-10T15:01:19Z

not that it matters that much, but why is this pr for 6.0.0-dev instead of master ? it does not seems to contains BC break

erayd · 2017-03-10T16:19:57Z

@mathroc Because it needs to be in 6.0.0 anyway, and adding it to master first means a merge mess later. I'm backporting non-bc-breaking changes from 6.0.0 as they are merged (see #368).

erayd · 2017-03-10T17:23:25Z

@shmax @mathroc
Thanks for reviewing :-).

I have just added one final commit after your LGTMs to fix #377 (c92dfae), if you also wish to review that. I would have done it as a separate PR, except that the default code has significantly changed in this one, so it was more logical to put it here.

@bighappyface Anything else that needs doing to this PR before merging?

Throws an exception when the ref can't be resolved to a useful file URI, rather than waiting for something further down the line to fail after the fact.

If CHECK_MODE_ONLY_REQUIRED_DEFAULTS is set, then only apply defaults if they are marked as required.

erayd · 2017-03-17T04:26:52Z

Rebased.

bighappyface

+1

…nbow#359) (jsonrainbow#365) * Don't try to fetch files that don't exist Throws an exception when the ref can't be resolved to a useful file URI, rather than waiting for something further down the line to fail after the fact. * Refactor defaults code to use LooseTypeCheck where appropriate * Test for not treating non-containers like arrays * Update comments * Rename variable for clarity * Add CHECK_MODE_ONLY_REQUIRED_DEFAULTS If CHECK_MODE_ONLY_REQUIRED_DEFAULTS is set, then only apply defaults if they are marked as required. * Workaround for $this scope issue on PHP-5.3 * Fix infinite recursion via $ref when applying defaults * Add missing second test for array case * Add test for setting a default value for null * Also fix infinite recursion via $ref for array defaults * Move nested closure into separate method * $parentSchema will always be set when $name is, so don't check it * Handle nulls properly - fixes issue jsonrainbow#377

* Add URI translation for retrieval & add local copies of spec schema * Add use line for InvalidArgumentException & adjust scope (#372) Fixes issue #371 * add quiet option (#382) * add quiet option * use verbose instead of quiet * add quiet option * always output dump-schema * always output dump-schema-url * fix typo and ws * [BUGFIX] Add provided schema under a dummy / internal URI (fixes #376) (#378) * Add provided schema under a dummy / internal URI (fixes #376) In order to resolve internal $ref references within a user-provided schema, SchemaStorage needs to know about the schema. As user-supplied schemas do not have an associated URI, use a dummy / internal one instead. * Remove dangling use * Change URI to class constant on SchemaStorage * Add option to disable validation of "format" constraint (#383) * Add more unit tests (#366) * Add test coverage for coercion API * Complete test coverage for SchemaStorage * Add test coverage for ObjectIterator * Add exception test for JsonPointer * MabeEnum\Enum appears to use singletons - add testing const * Don't check this line for coverage mbstring is on all test platforms, so this line will never be reached. * Add test for TypeConstraint::validateTypeNameWording() * Add test for exception on TypeConstraint::validateType() * PHPunit doesn't like an explanation with its @codeCoverageIgnore... * Add various tests for UriRetriever * Add tests for FileGetContents * Add tests for JsonSchema\Uri\Retrievers\Curl * Add missing bad-syntax test file * Restrict ignore to the exception line only * Fix exception scope * Allow the schema to be an associative array (#389) * Allow the schema to be an associative array Implements #388. * Use json_decode(json_encode()) for array -> object cast * Skip exception check on PHP versions < 5.5.0 * Skip test on HHVM, as it's happy to encode resources * Enable FILTER_FLAG_EMAIL_UNICODE for email format if present (#398) * Don't throw exceptions until after checking anyOf / oneOf (#394) Fixes #393 * Fix infinite recursion on some schemas when setting defaults (#359) (#365) * Don't try to fetch files that don't exist Throws an exception when the ref can't be resolved to a useful file URI, rather than waiting for something further down the line to fail after the fact. * Refactor defaults code to use LooseTypeCheck where appropriate * Test for not treating non-containers like arrays * Update comments * Rename variable for clarity * Add CHECK_MODE_ONLY_REQUIRED_DEFAULTS If CHECK_MODE_ONLY_REQUIRED_DEFAULTS is set, then only apply defaults if they are marked as required. * Workaround for $this scope issue on PHP-5.3 * Fix infinite recursion via $ref when applying defaults * Add missing second test for array case * Add test for setting a default value for null * Also fix infinite recursion via $ref for array defaults * Move nested closure into separate method * $parentSchema will always be set when $name is, so don't check it * Handle nulls properly - fixes issue #377 * Add option to also validate the schema (#357) * Remove stale files from #357 (obviated by #362) (#400) * Stop #386 sneaking in alongside another PR backport

…nbow#359) (jsonrainbow#365) * Don't try to fetch files that don't exist Throws an exception when the ref can't be resolved to a useful file URI, rather than waiting for something further down the line to fail after the fact. * Refactor defaults code to use LooseTypeCheck where appropriate * Test for not treating non-containers like arrays * Update comments * Rename variable for clarity * Add CHECK_MODE_ONLY_REQUIRED_DEFAULTS If CHECK_MODE_ONLY_REQUIRED_DEFAULTS is set, then only apply defaults if they are marked as required. * Workaround for $this scope issue on PHP-5.3 * Fix infinite recursion via $ref when applying defaults * Add missing second test for array case * Add test for setting a default value for null * Also fix infinite recursion via $ref for array defaults * Move nested closure into separate method * $parentSchema will always be set when $name is, so don't check it * Handle nulls properly - fixes issue jsonrainbow#377

erayd mentioned this pull request Mar 1, 2017

infinite loop with Constraint::CHECK_MODE_APPLY_DEFAULTS #359

Closed

erayd changed the base branch from master to 6.0.0-dev March 2, 2017 21:49

erayd force-pushed the bugfix-359-recursion branch from 9e30166 to bb014e4 Compare March 5, 2017 22:52

erayd force-pushed the bugfix-359-recursion branch from bb014e4 to 16a28ff Compare March 6, 2017 21:21

mathroc reviewed Mar 6, 2017

View reviewed changes

erayd changed the title ~~[WIP] Fix for #359~~ Fix for #359 Mar 7, 2017

erayd force-pushed the bugfix-359-recursion branch from 11de406 to 8af9398 Compare March 7, 2017 18:36

erayd changed the title ~~Fix for #359~~ Fix infinite recursion on some schemas when setting defaults (#359) Mar 8, 2017

erayd added 14 commits March 17, 2017 17:23

Don't try to fetch files that don't exist

93c5a64

Throws an exception when the ref can't be resolved to a useful file URI, rather than waiting for something further down the line to fail after the fact.

Refactor defaults code to use LooseTypeCheck where appropriate

976cfb0

Test for not treating non-containers like arrays

bdf8d06

Update comments

8ecbca2

Rename variable for clarity

7b869c0

Add CHECK_MODE_ONLY_REQUIRED_DEFAULTS

4ed0591

If CHECK_MODE_ONLY_REQUIRED_DEFAULTS is set, then only apply defaults if they are marked as required.

Workaround for $this scope issue on PHP-5.3

35c03bb

Fix infinite recursion via $ref when applying defaults

50c26d0

Add missing second test for array case

852ad07

Add test for setting a default value for null

37aa991

Also fix infinite recursion via $ref for array defaults

e0baac8

Move nested closure into separate method

e098ee7

$parentSchema will always be set when $name is, so don't check it

c4031e1

Handle nulls properly - fixes issue jsonrainbow#377

f63d938

erayd force-pushed the bugfix-359-recursion branch from c92dfae to f63d938 Compare March 17, 2017 04:26

bighappyface approved these changes Mar 21, 2017

View reviewed changes

bighappyface merged commit 46d10ac into jsonrainbow:6.0.0-dev Mar 21, 2017

erayd mentioned this pull request Mar 21, 2017

Backports for 5.2.0 #368

Merged

erayd deleted the bugfix-359-recursion branch March 22, 2017 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix infinite recursion on some schemas when setting defaults (#359) #365

Fix infinite recursion on some schemas when setting defaults (#359) #365

erayd commented Mar 1, 2017 •

edited

Loading

shmax commented Mar 6, 2017

mathroc commented Mar 6, 2017

mathroc commented Mar 6, 2017

erayd commented Mar 6, 2017

erayd commented Mar 6, 2017

erayd commented Mar 6, 2017

mathroc Mar 6, 2017

erayd Mar 6, 2017

mathroc commented Mar 6, 2017

erayd commented Mar 6, 2017 •

edited

Loading

mathroc commented Mar 6, 2017 •

edited

Loading

erayd commented Mar 6, 2017

mathroc commented Mar 6, 2017

erayd commented Mar 6, 2017

erayd commented Mar 6, 2017

erayd commented Mar 6, 2017

erayd commented Mar 7, 2017

erayd commented Mar 7, 2017

erayd commented Mar 7, 2017

erayd commented Mar 8, 2017

shmax commented Mar 8, 2017

erayd commented Mar 8, 2017

shmax commented Mar 8, 2017

erayd commented Mar 8, 2017

mathroc commented Mar 10, 2017

mathroc commented Mar 10, 2017

erayd commented Mar 10, 2017

erayd commented Mar 10, 2017

erayd commented Mar 17, 2017

bighappyface left a comment

Fix infinite recursion on some schemas when setting defaults (#359) #365

Fix infinite recursion on some schemas when setting defaults (#359) #365

Conversation

erayd commented Mar 1, 2017 • edited Loading

Changes Relating to #359

Other Changes

shmax commented Mar 6, 2017

mathroc commented Mar 6, 2017

mathroc commented Mar 6, 2017

erayd commented Mar 6, 2017

erayd commented Mar 6, 2017

erayd commented Mar 6, 2017

mathroc Mar 6, 2017

Choose a reason for hiding this comment

erayd Mar 6, 2017

Choose a reason for hiding this comment

mathroc commented Mar 6, 2017

erayd commented Mar 6, 2017 • edited Loading

mathroc commented Mar 6, 2017 • edited Loading

erayd commented Mar 6, 2017

mathroc commented Mar 6, 2017

erayd commented Mar 6, 2017

erayd commented Mar 6, 2017

erayd commented Mar 6, 2017

erayd commented Mar 7, 2017

erayd commented Mar 7, 2017

erayd commented Mar 7, 2017

erayd commented Mar 8, 2017

shmax commented Mar 8, 2017

erayd commented Mar 8, 2017

shmax commented Mar 8, 2017

erayd commented Mar 8, 2017

mathroc commented Mar 10, 2017

mathroc commented Mar 10, 2017

erayd commented Mar 10, 2017

erayd commented Mar 10, 2017

erayd commented Mar 17, 2017

bighappyface left a comment

Choose a reason for hiding this comment

erayd commented Mar 1, 2017 •

edited

Loading

erayd commented Mar 6, 2017 •

edited

Loading

mathroc commented Mar 6, 2017 •

edited

Loading