Fix #74039 is_infinite(-INF) broken when isinf not defined by c960657 · Pull Request #2356 · php/php-src

c960657 · 2017-02-01T08:30:19Z

is_infinite() is broken in the PHP binary included in Alpine Linux. It returns TRUE for is_infinite(INF), but FALSE for is_infinite(-INF).

I believe to have fixed the problem, but I know virtually nothing about C or the PHP source, so bear with me :-)

krakjoe · 2017-02-02T16:28:19Z

This is targeting a security fix only branch.

c960657 · 2017-02-02T19:58:57Z

I have pushed a new version against the PHP-7.0 branch.

krakjoe · 2017-02-03T06:06:13Z

This should have a bug on bugs.php.net for reference, and you should not update the news file in the PR - the merger does that.

c960657 · 2017-02-03T07:59:36Z

I have now removed the NEWS entry, and also added some tests for is_finite().

krakjoe · 2017-02-03T08:01:53Z

@c960657 please open a bug on bugs.php.net

c960657 · 2017-02-03T08:02:55Z

Sorry, it's here: https://bugs.php.net/bug.php?id=74039

krakjoe · 2017-02-03T09:22:51Z

@c960657 Can you have a look at master, it needs a different patch because file names have changed (or maybe no patch) ?

nikic · 2017-02-03T09:31:27Z

The patch isn't wrong (and this line should be changed), but I think the actual problem is that we check for isinf and isnan as functions in configure, while IIRC they are spec'd as macros. musl probably only has them as macros, so HAVE_ISINF and HAVE_ISNAN end up not being set. Without mucking with configure, we can probably work around this with a simple defined(isinf) etc extra check.

krakjoe · 2017-02-03T09:42:33Z

That makes sense, I was a bit confused about the whole thing, especially the comment that says it's required by C99.

The isnan() and isinf() are C99 macros not functions.

c960657 · 2017-02-03T16:54:59Z

I have pushed a fix inspired by this. But I really have no clue about what I am doing and how to test it, so if it isn't quite right, I hope I can pass on the torch to someone else.

hikari-no-yume · 2017-02-05T01:00:01Z

zend_isnan, zend_isinf and zend_finite are defined in two places: /configure.ac and /Zend/configure.ac. Don't ask me why. You'd need to change both.

hikari-no-yume · 2017-02-05T01:40:38Z

See also: https://bugs.php.net/bug.php?id=73954

hikari-no-yume · 2017-02-05T01:47:46Z

Zend/configure.in

-	#define HAVE_ISNAN 1
-	#define HAVE_ISINF 1
-	#define HAVE_ISFINITE 1
+	#define HAVE_DECL_ISNAN 1


Is renaming these constants strictly necessary?

nikic · 2017-02-05T17:15:21Z

Merged via 9ea0949 with tests separated out in a separate file. Thanks!

nikic · 2017-02-06T00:50:14Z

Changes reverted by 8a8aa67 due to a failure in newly introduced test Zend/tests/bug73954.phpt on Travis. The diff is:

006+ #0 /home/travis/build/php/php-src/Zend/tests/bug73954.php(9): takes_int(-NAN)
006- #0 %s(9): takes_int(NAN)

I'm not seeing the failure locally though.

hikari-no-yume · 2017-02-06T00:53:27Z

…huh, -NAN. NaNs aren't supposed to be signed, right?

nikic · 2017-02-06T01:00:44Z

@TazeTSchnitzel NaNs do have a sign bit, though it's not usually considered to be relevant. What confuses me here is why -NAN only shows up in the argument list (but not in the the var_dump(NAN) a few lines up). They must be using different ways of printing doubles...

hikari-no-yume · 2017-02-06T01:02:28Z

@nikic Presumably. My first thought was maybe it's doing something like if (f > 0) { /* ... */ } else { putc("-"); }, but I'd need to check.

hikari-no-yume · 2017-02-06T01:08:24Z

Relevant code is this:

php-src/Zend/zend_exceptions.c

Lines 522 to 530 in 31332d0

    
           case IS_DOUBLE: { 
        
           	double dval = Z_DVAL_P(arg); 
        
           	char *s_tmp = emalloc(MAX_LENGTH_OF_DOUBLE + EG(precision) + 1); 
        
           	int l_tmp = zend_sprintf(s_tmp, "%.*G", (int) EG(precision), dval);  /* SAFE */ 
        
           	smart_str_appendl(str, s_tmp, l_tmp); 
        
           	smart_str_appends(str, ", "); 
        
           	efree(s_tmp); 
        
           	break; 
        
           }

Is there something wrong with zend_sprintf?

nikic · 2017-02-06T01:44:57Z

@TazeTSchnitzel Nice catch! Looks like zend_sprintf is essentially an alias for the system sprintf, which may indeed exhibit this behavior. What we should be using there is zend_spprintf or zend_strpprintf, or on master smart_str_append_printf.

That still leaves the question of why we're seeing this issue only after applying this patch.

weltling · 2017-02-06T23:44:03Z

I'm investigating on the same test fail recently still happening on AppVeyor. In this regard, i've found these two topics

https://blogs.msdn.microsoft.com/oldnewthing/20130221-00/?p=5183/
http://stackoverflow.com/questions/32195949/why-does-nan-nan-0-0-with-the-intel-c-compiler

Specifically AppVeyor, and what i get locally is -nan(ind), which is an indefinite NAN :) When debugging through, it is something that the argument contains right when it has been parsed. For the output however, it is different. In the smart string API, we check for zend_isnan() and place the hardcoded NAN, while a system routine would of course output the precise representation, Plus, representation nan(ind) as is somewhat different from what the older MSDN blog post refers to as 1#IND. So that's regarding all the possible types of NAN and INF :) In the Windows build, we seem to explicitly create this exact kind of NAN with php_get_nan().

Additionally, the other part is about the compiler flags and different compilers. Additionally, it comes to the concrete FPU which might create a different kind of nan or infinity. In PHP, we seem not to care about these differences, there it's simply a NAN. As for me - the conclusion is, that as long as we don't really care about finer NAN types and use the system routine for string conversion, and we cannot simply use is_nan() as in this case with the type hint conversion check, it is valid to simply care about the exception thrown and maybe check that the word NAN is present.

Thanks.

hikari-no-yume · 2017-02-06T23:50:20Z

in this case with the type hint conversion check, it is valid to simply care about the exception thrown and maybe check that the word NAN is present.

We could change the test, but I'm not sure the test is really the problem here. The PHP language is supposed to present only one kind of NAN. Internally it can be whatever we want, but these details shouldn't leak through into stacktraces and things.

weltling · 2017-02-06T23:59:48Z

Yeah, that would be clean of course, if the corresponding place could be moved to smart string API. I was more about illustrating, that the issue is not about type hints or math, how it looked like, but just the output routine. Were it possible to replace the output routine, so we'd be fine as well. Otherwise, fixing the test were sufficient, IMHO.

Thanks.

see #2356 (comment)

weltling · 2017-02-07T17:45:20Z

I think this PR can be given the next run, the conversion routine is replaced and everything seems OK.

Thanks.

nikic · 2017-02-07T18:05:32Z

We still need to understand how/why this PR changed the specific NAN value that the NAN constant returns -- it shouldn't have an impact on that. php_get_nan() is defined here: https://github.com/php/php-src/blob/PHP-7.0/ext/standard/basic_functions.c#L3489 My best guess it that after this PR we're hitting the second case (which sets a negative qNAN) and previously we hit the first case. This would mean that HAVE_HUGE_VAL_NAN is not being defined. Which would mean that zend_isnan is not working on the check for it.... Aha, this is the culprit: There is another check for isnan as a function here: https://github.com/php/php-src/blob/PHP-7.0/ext/standard/config.m4#L352 As this occurrence wasn't changed, we didn't check against the right macro.

hikari-no-yume · 2017-02-07T18:09:17Z

Aha. So, revert the macro name change, and all is well.

(You could keep that part, but it's probably needless effort. For one thing, you might get merge conflicts in master where the NaN code has been moved around.)

nikic · 2017-02-07T18:14:36Z

The macro name comes from autoconf.

The only change necessary here is to make the same change in ext/standard/config.m4 as was done in Zend/config.m4.

hikari-no-yume · 2017-02-07T18:15:39Z

What's the reason for changing it (the macro name, I mean) in the first place? Maybe I'm misunderstanding something.

nikic · 2017-02-07T23:56:29Z

Relanded as 714d825 with the extra change in ext/standard/config.m4.

* php/master: (149 commits) Fixed bug #61471 Add #ifndef restrict Fix detection of isnan and isinf ReflectionGenerator now sends ReflectionException as expected use some dynamically generated NAN as well rework fd521a2 to simplify for master, see github php#2356 switch to smart str conversion routine to hide exact NAN type Revert "Fix detection of isnan and isinf" Fix detection of isnan and isinf Fix bug #73954 Implement Parameter Type Widening RFC Add UPGRADING notes for deprecations Deprecate each() Deprecate assert() with string argument Deprecate mbstring.func_overload Deprecate track_errors / $php_errormsg Deprecate mb_parse_str() without second argument Deprecate parse_str() without second argument Deprecate (unset) Deprecate __autoload() ...

krakjoe added the Waiting on Author label Feb 2, 2017

c960657 changed the base branch from PHP-5.6 to PHP-7.0 February 2, 2017 17:52

krakjoe added the Bug label Feb 3, 2017

is_infinite(-INF) broken when isinf not defined

f868f70

krakjoe changed the title ~~is_infinite(-INF) broken when isinf not defined~~ Fix #74039 is_infinite(-INF) broken when isinf not defined Feb 3, 2017

Fix detection of isnan and isinf

2d278a3

The isnan() and isinf() are C99 macros not functions.

nikic self-assigned this Feb 3, 2017

nikic removed the Waiting on Author label Feb 3, 2017

hikari-no-yume reviewed Feb 5, 2017

View reviewed changes

nikic closed this Feb 5, 2017

nikic reopened this Feb 6, 2017

php-pulls pushed a commit that referenced this pull request Feb 7, 2017

switch to smart str conversion routine to hide exact NAN type

fd521a2

see #2356 (comment)

php-pulls pushed a commit that referenced this pull request Feb 7, 2017

rework fd521a2 to simplify for master, see github #2356

7e440f2

nikic closed this Feb 7, 2017

Conversation

c960657 commented Feb 1, 2017

Uh oh!

krakjoe commented Feb 2, 2017

Uh oh!

c960657 commented Feb 2, 2017

Uh oh!

krakjoe commented Feb 3, 2017

Uh oh!

c960657 commented Feb 3, 2017

Uh oh!

krakjoe commented Feb 3, 2017

Uh oh!

c960657 commented Feb 3, 2017

Uh oh!

krakjoe commented Feb 3, 2017

Uh oh!

nikic commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krakjoe commented Feb 3, 2017

Uh oh!

c960657 commented Feb 3, 2017

Uh oh!

hikari-no-yume commented Feb 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hikari-no-yume commented Feb 5, 2017

Uh oh!

hikari-no-yume Feb 5, 2017

Choose a reason for hiding this comment

Uh oh!

nikic commented Feb 5, 2017

Uh oh!

nikic commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hikari-no-yume commented Feb 6, 2017

Uh oh!

nikic commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hikari-no-yume commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hikari-no-yume commented Feb 6, 2017

Uh oh!

nikic commented Feb 6, 2017

Uh oh!

weltling commented Feb 6, 2017

Uh oh!

hikari-no-yume commented Feb 6, 2017

Uh oh!

weltling commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weltling commented Feb 7, 2017

Uh oh!

nikic commented Feb 7, 2017

Uh oh!

hikari-no-yume commented Feb 7, 2017

Uh oh!

nikic commented Feb 7, 2017

Uh oh!

hikari-no-yume commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Feb 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nikic commented Feb 3, 2017 •

edited

Loading

hikari-no-yume commented Feb 5, 2017 •

edited

Loading

nikic commented Feb 6, 2017 •

edited

Loading

nikic commented Feb 6, 2017 •

edited

Loading

hikari-no-yume commented Feb 6, 2017 •

edited

Loading

weltling commented Feb 6, 2017 •

edited

Loading

hikari-no-yume commented Feb 7, 2017 •

edited

Loading