Skip to content

Conversation

juan-morales
Copy link
Contributor

No description provided.

@juan-morales
Copy link
Contributor Author

@cmb69 , while working on my RFC (#9355) , I found out this.

Can you check the azure pipeline also? Is this a bug?

@juan-morales juan-morales marked this pull request as ready for review August 17, 2022 19:49
@cmb69
Copy link
Member

cmb69 commented Aug 18, 2022

That is mostly expected behavior. $depth must be smaller than or equal to 2**31-1; otherwise a ValueError is thrown, and the function is not executed at all. On 32bit systems, PHP_INT_MAX is equal to 2**31-1, so no error is thrown, and the function succeeds.

Testing this ValueError makes sense, but we should skip that test on 32bit systems.

Anyhow, the error message is off by one (should be "less than or equal to"), and there is no need for this check on 32bit systems at all; zend_range_check.h has some macros which support such range checks; in this case ZEND_LONG_INT_OVFL(depth) could be used.

And we need to document the range limit (0 < depth <= INT_MAX) in the manual.

PS: docs fixed

cmb69 added a commit to php/doc-en that referenced this pull request Aug 18, 2022
mumumu added a commit to php/doc-ja that referenced this pull request Aug 18, 2022
@juan-morales
Copy link
Contributor Author

Should we close this PR? Should I fix something in the core? (like depth >= INT_MAX) ?

@cmb69
Copy link
Member

cmb69 commented Aug 19, 2022

I think this PR is good (unless that code path is already tested), but the test needs to be skipped on 32bit architectures.

If you like, you can provide a PR regarding the implementation of the function.

@juan-morales
Copy link
Contributor Author

lets see how can I improve all this.

@juan-morales
Copy link
Contributor Author

Just found this:

ext/json/tests/bug72787.phpt

Luckly, there is already a test for that in json_decode() and I will use the same SKIP Rule in my test for is_json() RFC

tiffany-taylor pushed a commit to tiffany-taylor/doc-en that referenced this pull request Jan 16, 2023
claudepache pushed a commit to claudepache/php-doc-en that referenced this pull request Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants