Skip to content
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

Prevent "interval" overflow in ExtUvLoop #196

Merged
merged 2 commits into from Aug 12, 2019

Conversation

PabloKowalczyk
Copy link
Contributor

@PabloKowalczyk PabloKowalczyk commented May 20, 2019

This PR fixes #194.

@WyriHaximus WyriHaximus requested review from WyriHaximus, clue and jsor May 20, 2019
@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented May 20, 2019

@PabloKowalczyk looking into why Travis lost track of the required UV dependencies

@PabloKowalczyk
Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk commented May 20, 2019

@WyriHaximus command sudo apt-get install libuv1-dev || true silently failed with message E: Unable to locate package libuv1-dev, maybe this is a reason?

@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented May 20, 2019

@PabloKowalczyk yeah most likely, but ensure why that is an issue all of the sudden

@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented May 23, 2019

@PabloKowalczyk just filed #197 to fix this issue.

@PabloKowalczyk
Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk commented May 23, 2019

@WyriHaximus good job, after your merge #197 i will rebase my branch and push changes again.

@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented May 23, 2019

@PabloKowalczyk FYI #197 has been merged

@PabloKowalczyk
Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk commented May 24, 2019

Rebase done, but there is other issue:

React\Tests\EventLoop\Timer\ExtUvTimerTest::testAddPeriodicTimerCancelsItself
Failed asserting that 0.004443168640136719 is equal to 0.005 or is greater than 0.005.

IMO it is not caused by my change, i can't reproduce it locally and seems "random" to me. What do you think @WyriHaximus?

@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented May 24, 2019

@PabloKowalczyk sometimes we have to kick the build to fix that error ;)

@PabloKowalczyk
Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk commented May 24, 2019

Yep, now better :)

jsor
jsor approved these changes May 24, 2019
Copy link
Member

@clue clue left a comment

@PabloKowalczyk Thank you for looking into this and keeping this updated!

I've added a couple of remarks below, what do you think about this? 👍

src/ExtUvLoop.php Outdated Show resolved Hide resolved
}

// Subtract 2 because 1 doesn't seems to work and may give wrong results
$maxValue = ((int) (\PHP_INT_MAX / 1000)) - 2;
Copy link
Member

@clue clue May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calculation looks really odd, i.e. it doesn't explain why or what "wrong results" could be. As an alternative, what do you think about something like this:

$result = $interval * 1000;
if ($result > PHP_INT_MAX) {
    // nope
}
return (int)$result;

Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code will work only if $result will be about ~1025 more than PHP_INT_MAX,
check this example: https://3v4l.org/OH3T7

Copy link
Member

@clue clue May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. PHP will automatically convert an int on overflow to a float, but PHP_INT_MAX + 1 > PHP_INT_MAX === false. If you cast this float value back to int, you'll get an integer overflow and a negative value in return. Here's the gist:

$ms = (int)($interval * 1000);
if ($ms < 0 || $interval > \PHP_INT_MAX) {
    // nope
}
return $ms;

Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at this code: https://3v4l.org/WcQBR - should it output "Ok"?

Copy link
Member

@clue clue May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we talking about the boundary value? I don't really mind tbqh. We're discussing integer overflows that happen for timer intervals of round ~25 days on 32 bit platforms and millions of years on 64 bit platforms, so I don't think it's really relevant if it's +/-1 ms. What I do care about is that the outcome is consistent and reliable.

I'm not suggesting this solution is perfect, I was mostly approaching it from a different perspective because the original implementation contained an apparently arbitrary -2 offset.

Either solution is fine for me as long as it works reliably 👍

Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you point of view, we could move -2-responsibility to user and do not bother with it in implementation - https://3v4l.org/AADvB, but we still need to do (PHP_INT_MAX / 1000) - 2 in tests (\React\Tests\EventLoop\AbstractLoopTest::testTimerIntervalCanBeFarInFuture) - maybe with extensive comment about -2-issue.
On the other hand i'm completely fine with leaving current implementation as is, so it is up to you to choose the best for you/project/users/etc.

Copy link

@ghost ghost Jun 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maximum working interval is (int) (\PHP_INT_MAX / 1000) - 1. If the value is higher, you can simply reject it before doing any conversion.

Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk Jun 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

private function convertFloatSecondsToMilliseconds($interval, $allowZero = false)
{
if ($interval < 0) {
$interval = 0;
Copy link

@ghost ghost Jun 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well do a return here instead of going through all these calculations?

Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk Jun 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could return early only if i duplicate $allowZero-related logic from last line:

return $result === 0 && !$allowZero
    ? 1
    : $result
;

So new code will look like:

if ($interval < 0) {
    return $allowZero
        ? 0
        : 1
    ;
}

IMO it is better to leave it as is.

Copy link

@ghost ghost Jun 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure for what it even is? Libuv allows 0, so differentiating between 0 and 1 is imo unnecessary.

Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk Jun 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must admit i don't know, but my guess is addPeriodicTimer with 0 will run code only once, also adding 1 was part of old code: https://github.com/reactphp/event-loop/blob/v1.1.0/src/ExtUvLoop.php#L130 and https://github.com/reactphp/event-loop/blob/v1.1.0/src/ExtUvLoop.php#L153-L154

Copy link

@ghost ghost Jun 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need a non-zero repeat (periodic timer). And then it'd be better to do an inline check there, so it only affects the repeat value and not the timeout value.

Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk Jun 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, actually it makes conversion easier. Please check new code.

$maxValue = (int) (\PHP_INT_MAX / 1000);
$intInterval = (int) $interval;

if ($intInterval >= $maxValue) {
Copy link

@ghost ghost Jun 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just tested this and unfortunately there's an edge case, which makes the comparison fail, even though the interval is significantly larger (as float) than the PHP integer max value.

Based on https://3v4l.org/TIkG3, the suggestion should work around that. You should probably add test cases for these.

Suggested change
if ($intInterval >= $maxValue) {
if ($interval > \PHP_INT_MAX || ($intInterval <= 0 && $interval > 0)) {

Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, fixed and tested.

@ghost
Copy link

@ghost ghost commented Jul 10, 2019

@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Jul 10, 2019

@CharlotteDunois done, thanks for the heads up 👍

@ghost
Copy link

@ghost ghost commented Jul 10, 2019

@clue Could you please review the PR again? From my side it looks from a quick glance ok.

Copy link
Member

@clue clue left a comment

Thank you for the update, the changes LGTM! Can you squash this to a reasonable number of commits before we can merge this? :shipit:

@PabloKowalczyk
Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk commented Jul 11, 2019

@clue should i do the squash? You can also do squash and merge by GitHub UI.

@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Jul 26, 2019

@PabloKowalczyk yes please.

@PabloKowalczyk
Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk commented Jul 27, 2019

Done.

$this->loop->addTimer(-1, function () {});

$this->assertRunFasterThan(0.002);
}
Copy link
Member

@clue clue Jul 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently have this odd split in our tests suite, can you move this to the similar tests in AbstractTimerTests?

Other than that, this test looks good to me, but we've seen similar tests fail sporadically due to timer inaccuracies, see also

public function testAddTimerWillBeInvokedOnceAndBlocksLoopWhenRunning()
{
// Make no strict assumptions about actual time interval. Common
// environments usually provide millisecond accuracy (or better), but
// Travis and other CI systems are slow.
// We try to compensate for this by skipping accurate tests when the
// current platform is known to be inaccurate. We test this by sleeping
// 3x1ms and then measure the time for each iteration before running the
// actual test.
for ($i = 0; $i < 3; ++$i) {
$start = microtime(true);
usleep(1000);
$time = microtime(true) - $start;
if ($time < 0.001 || $time > 0.002) {
$this->markTestSkipped('Platform provides insufficient accuracy (' . $time . ' s)');
}
}

Perhaps you can update this to use a similar logic to make sure an inaccurate platform does not cause this test to fail?

Copy link
Contributor Author

@PabloKowalczyk PabloKowalczyk Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved.

clue
clue approved these changes Aug 12, 2019
Copy link
Member

@clue clue left a comment

Changes LGTM, thanks for keeping up with this! :shipit:

@WyriHaximus WyriHaximus merged commit 85a0b7c into reactphp:master Aug 12, 2019
1 check passed
@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Aug 12, 2019

Thanks 👍 !

@PabloKowalczyk PabloKowalczyk deleted the interval-overflow-fix branch Aug 12, 2019
@clue clue added this to the v1.1.1 milestone Jan 1, 2020
@clue clue added the bug label Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants