Skip to content

Too big float to int cast results nonsense #17081

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mvorisek opened this issue Dec 7, 2024 · 13 comments
Open

Too big float to int cast results nonsense #17081

mvorisek opened this issue Dec 7, 2024 · 13 comments

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Dec 7, 2024

Description

repro: https://3v4l.org/0FJkO

I discovered this in library where big timeout was converted to milliseconds like $timeoutFloat * 1000 and later casted into integer which resulted very dangerous 0 without any warning.

I would expect some warning like https://3v4l.org/4C3ru when too big float is impossible to be casted meaningfully into int.

@TimWolla
Copy link
Member

TimWolla commented Dec 9, 2024

I would expect some warning like https://3v4l.org/4C3ru when too big float is impossible to be casted meaningfully into int.

That's an implicit conversion. An explicit cast does not warn: https://3v4l.org/F9Ir7.

Similarly implicitly converting this large float to an int will already throw an error: https://3v4l.org/6NnZ6

In any case, given that this affects the core language functionality, this requires an RFC.

@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 9, 2024

As the first step of this/my feature request, I would expect PHP_INT_MAX for float larger than the max. integer value.

The warning is second priority.

@iluuu1994
Copy link
Member

Casting huge ints to floats also loses precision. Deciding what should warn and what shouldn't is not trivial. I also think this should be discussed first.

@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 9, 2024

In this issue I want to focus solely on https://3v4l.org/4gVYb behaviour which is very dangerous and not much expected from user POV. I would expect the resulting integer to be capped to PHP_INT_MAX/PHP_INT_MIN with the correct sign.

@mvorisek mvorisek changed the title Too big float to int cast should emit warning Too big float to int cast results nonsense Dec 9, 2024
Copy link

There has not been any recent activity in this feature request. It will automatically be closed in 14 days if no further action is taken. Please see https://github.com/probot/stale#is-closing-stale-issues-really-a-good-idea to understand why we auto-close stale feature requests.

@github-actions github-actions bot added the Stale label Mar 10, 2025
@mvorisek
Copy link
Contributor Author

I belive this is manageable, where in the code the float to int is casted? I belive we should and can cap it to int max/min.

@github-actions github-actions bot removed the Stale label Mar 11, 2025
Copy link

github-actions bot commented Jun 9, 2025

There has not been any recent activity in this feature request. It will automatically be closed in 14 days if no further action is taken. Please see https://github.com/probot/stale#is-closing-stale-issues-really-a-good-idea to understand why we auto-close stale feature requests.

@github-actions github-actions bot added the Stale label Jun 9, 2025
@mvorisek
Copy link
Contributor Author

mvorisek commented Jun 9, 2025

Can someone please point me to the php-src file/line where the cast from float to int is done?

@nielsdos
Copy link
Member

nielsdos commented Jun 9, 2025

Can someone please point me to the php-src file/line where the cast from float to int is done?

/* Used to convert a string float to integer during an (int) cast */
static zend_always_inline zend_long zend_dval_to_lval_cap(double d)
{
if (UNEXPECTED(!zend_finite(d)) || UNEXPECTED(zend_isnan(d))) {
return 0;
} else if (!ZEND_DOUBLE_FITS_LONG(d)) {
return (d > 0 ? ZEND_LONG_MAX : ZEND_LONG_MIN);
}
return (zend_long)d;
}
/* }}} */

@mvorisek
Copy link
Contributor Author

mvorisek commented Jun 9, 2025

So the case should be handled, but it isn't. Any idea why?

Here is simpler repro: https://3v4l.org/JauAu

-actual
+expected
 9.2233720368548E+21
 9223372036854775807
-0
+9223372036854775807

@nielsdos
Copy link
Member

nielsdos commented Jun 9, 2025

Ah because I pointed to the wrong location.
The location I pointed at is for string floats, not floats.
zend_dval_to_lval is used for floats, which will call zend_dval_to_lval_slow which doesn't cap.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jun 9, 2025

Initially I opened this as a feature to get a warning emit. Let this issue be not about a warning, but about the result.

Can this issue be please changed to a bug /wo RFC?

I belive it is a bug because of these reasons:

  1. the result is not always 0 - repro https://3v4l.org/lYgYh
  2. the result is inconsistent on x86 vs. x64 (x86 can be checked by editing the 3v4l.org testcase in live-local-preview)

@nielsdos
Copy link
Member

nielsdos commented Jun 9, 2025

It needs an RFC regardless because it changes behaviour that so long standing that people may (unknowingly) rely on it. I'm also quite certain that people will argue a lot about the desired behaviour.
If an RFC comes from this, it should also make the string float casting behaviour consistent with the normal float casting behaviour. I was surprised this is different.

@github-actions github-actions bot removed the Stale label Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants