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

High resolution monotonic timer #2976

Closed
wants to merge 13 commits into
base: master
from

Conversation

9 participants
@weltling
Contributor

weltling commented Dec 16, 2017

The implementation is based on the work started in #2368, with some reworks. This PR gets more focus on portability and solves issues in the predecessor as seems it's abandoned now. Besides benchmarking, the implmented function is suitable for other situations where monotonic timer is needed.

The delivered time starts at some uncertain point in the past. The time source is monotonic and isn't in any way adjustable or related to time of day. Several platform specific details are implemented. The signature is mixed hrtime([bool as_num]). By default, it delivers an array of the form [seconds, nanoseconds]. If the optional argument was passed, the return value is the number of nanoseconds as int on 64-bit or float on 32-bit. Some additional APIs can be implemented later, like hrtime_cmp and hrtime_diff.

Thanks.

@weltling weltling referenced this pull request Dec 16, 2017

Closed

Implement hrtime() #2368

@kelunik

This comment has been minimized.

Contributor

kelunik commented Dec 16, 2017

Just had a quick look API-wise. I'd prefer different functions for different formats, bool flags are evil. But I don't have good name suggestions right now.

@weltling

This comment has been minimized.

Contributor

weltling commented Dec 16, 2017

@kelunik thanks for checking. That's microtime() API consistency. There's otherwise no much impact. And naming can get weird, yep, like putting datatype into signature.

Thanks.

@kelunik

This comment has been minimized.

Contributor

kelunik commented Dec 16, 2017

@weltling I know, but it's not really consistent. microtime() returns a string by default which must be split.

@weltling

This comment has been minimized.

Contributor

weltling commented Dec 16, 2017

@kelunik the signature it is and the way it returns some number with true. The stirng return - well, not really sure it should mimic that, too. In terms of functionality - perhaps array is the only thing that makes sense for portability. The number return is good on 64-bit and thus was tempting. Anyway, please suggest another API to discuss. Otherwise could simply only leave the array return and remove the alternative variant. Something for diff/compare would be then unambiguous, too.

@nikic nikic added the Enhancement label Dec 16, 2017

} while (0)
#endif

/* {{{ proto mixed hrtime([bool get_as_numu = false])

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

Should probably mention it is monotonic. It's in the patch comment, but this won't be available for people reading the code.

} while (0)
#endif

/* {{{ proto mixed hrtime([bool get_as_numu = false])

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

I guess "numu" should be "num"? Or, even better, "number".

This comment has been minimized.

@weltling

weltling Jan 3, 2018

Contributor

Yeah, typo here. Spelling completely as suggested.

PHP_RETURN_HRTIME(t);
}
#else
RETURN_LONG(0);

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

Maybe it would be better to return false? Otherwise there's no reliable non-probabilistic way of seeing whether the current platform supports hrtime.

This comment has been minimized.

@weltling

weltling Jan 3, 2018

Contributor

Good point, either NULL or false, so lets take false.

}
/* }}} */

PHPAPI php_hrtime_t php_hrtime_current(void)

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

This is not used anywhere. Is it supposed to be?

This comment has been minimized.

@weltling

weltling Jan 3, 2018

Contributor

Done specifically to export the useful piece of the new API, so i'd say yes. It can be used by non core exts, etc.

#ifndef HRTIME_H
#define HRTIME_H

#define PHP_HRTIME_PLATFORM_POSIX 0

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

Shouldn't these things be handled by autoconf? FPM has some autoconf checks for clock_gettime already, maybe others can be added.

This comment has been minimized.

@weltling

weltling Jan 3, 2018

Contributor

Can migrate this, yep. In general, a system where sysconf is available could do a compile time check. I've omited it, because a single compile time failure is less to say against the runtime check. The binary can be compiled with a runtime check at the PHP startup, the submodule will fail otherwise.

#if PHP_HRTIME_PLATFORM_WINDOWS
uint64_t cur;
QueryPerformanceCounter((LARGE_INTEGER*) &cur);
return (php_hrtime_t)((double)cur * _timer_int * (double)NANO_IN_SEC);

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

double has only 53 bits of precision for value, so there could be a loss of precision here. Couldn't we use timer frequency directly instead of floats?

{/*{{{*/
#if PHP_HRTIME_PLATFORM_WINDOWS
uint64_t cur;
QueryPerformanceCounter((LARGE_INTEGER*) &cur);

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

This looks kinda dangerous. Couldn't we use LARGE_INTEGER properly instead of relying on uint64_t having the same bits (which it probably would, but it still is unclean)?

#ifdef _WIN32
# define HRTIME_U64A(i, s, len) _ui64toa_s(i, s, len, 10)
#else
# define HRTIME_U64A(i, s, len) \

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

Don't we already have ZEND_LTOA? Why duplicate it?

This comment has been minimized.

@weltling

weltling Jan 3, 2018

Contributor

ZEND_LTOA depends on the platform and matches zend_long. The timer is saved in uint64_t, thus the macro is not suitable on 32-bit.

Z_PARAM_BOOL(get_as_num)
ZEND_PARSE_PARAMETERS_END();

if (!get_as_num) {

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

could we reverse the condition? It's cognitively easier to read something as "if this then do A else do B" then "if not this then do A else do B".

This comment has been minimized.

@weltling

weltling Jan 3, 2018

Contributor

Of course, that doesn't matter. Also should wrap with UNEXPECTED. For the processor it doesn't matter anyway, only a single instruction in both cases.

char _a[ZEND_LTOA_BUF_LEN]; \
double _d; \
HRTIME_U64A(t, _a, ZEND_LTOA_BUF_LEN); \
_d = zend_strtod(_a, NULL); \

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

I'm not sure why we're converting to string and back here - can't we just do (double)? Yes, there's a potential for precision loss, but wouldn't it happen in string case too?

This comment has been minimized.

@weltling

weltling Jan 3, 2018

Contributor

The integral part of double can be overflown quite fast. With an up time of a couple of months, as soon as timestamp exceeds (2^52)/1000000000. The string conversion is the only way i see so far to do the correct conversion. Still it's 32-bit only. On 64-bit, even a signed 64-bit integral is fine for some hundreds of years of up time.

@smalyshev

This comment has been minimized.

Contributor

smalyshev commented Jan 2, 2018

Please also document new function in UPGRADING.

/* $Id$ */

#include "php.h"
#include "zend_exceptions.h"

This comment has been minimized.

@smalyshev

smalyshev Jan 2, 2018

Contributor

Is this actually used?

This comment has been minimized.

@weltling

weltling Jan 3, 2018

Contributor

Nope, removed.

@weltling weltling force-pushed the weltling:hrtime branch 2 times, most recently from 6857b27 to 3cab0c3 Jan 3, 2018

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 3, 2018

Stas, i've addressed the comments where appropriate or left an explanation. The 32-bit part regarding floats is of course not pretty, but the string conversion is most reliable. Indeed, one could simply abandon the get_as_number part, but it would be a pity for 64-bit. It is anyway to see whether 32-bit dies or we'll have to implement 64-bit integer support there :)

Regarding the m4 migration - yep, can be done, then AC_FPM_CLOCK function should be moved into the core m4. Either header or m4 seems ok, so i'd leave it with header for now and see to do this with FPM refactoring. And of course keeping in mind UPGRADING, will document once merged.

Thanks.

@smalyshev

This comment has been minimized.

Contributor

smalyshev commented Jan 3, 2018

You can add UPGRADING part to this very patch, no need to make two separate ones I think?

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 3, 2018

Of course, just that's often a conflicting part for the merge. I'll add it then, nevertheless.

kelunik and others added some commits Feb 5, 2017

Implement hrtime()
This adds hrtime() to provide access to the system's high-resolution timer.

Fix hrtime() proto

Merge used parts of timer.h library into hrtime.c

Change hrtime return to double, improve feature checking

Add hrtime support for HP-UX

Add UNEXPECTED wrapping for hrtime failures

Fix bogus elif

Fix other bogus elif
Return nanoseconds as zend_long on 64-bit, float on 32-bit, implement…
… return array [s, ns]

Adjust tests

Move and rename test

Fix proto comment

Return array by default

Fix 32-bit and some more

Still provide symbols even if functionality is unavailable

Change typename

Add AIX specific pieces, untested

Improve 32-bit side once more

Rework macros

Few renames

Add comment

Remove unused code, a few other small things

Add comment

ws

Reword
Use LARGE_INTEGER also for the frequency and care
about QueryPerformanceCounter usage as per doc

@weltling weltling force-pushed the weltling:hrtime branch from 40931a8 to 1d1062a Jan 4, 2018

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 4, 2018

After some tests and research, i've turned the QPC part back to double arithmetics. It turns out, that QPC is still not 100% reliable when it comes to virtualization. As a reference, here's among others the link like https://www.virtualbox.org/ticket/11951. I was also able to find a machine where QPC was resetting about every 2 hours, that's anything but monotonic. The double arithmetic mitigates it by using the compiler magic with type conversions int64_t -> uint64_t -> double and due to the fact 32-bit VS still has a native 64-bit integer type. I need to have some more tests on machines with uptime over two months yet. I think, the Windows implementation in this patch is the most PITA, tests on Linux and FreeBSD show good results.

Thanks.

if (UNEXPECTED(get_as_num)) {
PHP_RETURN_HRTIME(t);
} else {
array_init(return_value);

This comment has been minimized.

@laruence

laruence Jan 6, 2018

Member

you may use array_init_size and zend_hash_real_init to make it a packed array

This comment has been minimized.

@weltling

weltling Jan 6, 2018

Contributor

Done. Thanks for the tip!

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 7, 2018

Merged as 8349732.

Thanks everyone!

@weltling weltling closed this Jan 7, 2018

@mappu

This comment has been minimized.

mappu commented Jan 7, 2018

Firefox/Chrome recently removed high-performance timers because they can be abused for Spectre exploits in multitenant environments. It's a very strange time to be introducing a new high-performance timer API.

Is there any potential security impact of having a high-performance timer API available? e.g. shared hosting environments?

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 7, 2018

@mappu could you give more info on that, please?

Thanks.

@mappu

This comment has been minimized.

mappu commented Jan 8, 2018

@weltling "Spectre" attacks can read information from other processes on the machine, by biasing the CPU branch predictor and then timing how long some operations take.

It's a major vulnerability for web applications because the vulnerability can be exploited from Javascript. In that case, a website running malicious javascript could read sensitive data from other tabs, or from other OS processes.

Firefox [1], Edge [2], and Chrome [3] all issued emergency security patches this week, to reduce the accuracy of their high-resolution timer API (performance.now()).

It's not clear whether a better mitigation will be available in the future, or if this is going to be permanent on current CPUs.

I suspect the same attack, or a similar attack, would also apply to PHP in shared hosting environments.

Maybe it's not possible, but in a post-Spectre world, high-resolution timers must be treated as a security-sensitive issue.

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 8, 2018

@mappu thanks for more info. Basically, anytime one comes from the holiday, something horrible has happened :) Talking seriously - the real mitigation seems unavoidable in first place by hardware and OSes, that's where the focus is. The mitigation in browsers is of course the necessary immediate step to solve the issue right now in the released software. This patch is master only and has time to see what happens further.

Today, 5 day after the disclosure, solutions are being worked on, in kernels and compilers. Some distros already provide kernel patches, see for example SUSE, also gcc and clang already have patches. So it is clear a mitigation stands already. But in general, this kind of vulnerability is likely only to solve at the very low level, not by the applications. In the end, there is no reason for panic, but of course a good reason to stay tuned as there are other time sources based on gettimeofday(), etc. and modules in many other scripting languages, etc.

Thanks.

@nikic

This comment has been minimized.

Member

nikic commented Jan 8, 2018

Shared hosting providers may still disable this function, just like they currently disable functions that they consider dangerous for shared hosting use.

That said, the reason why this functionality was originally requested, was because AMP needed a monotonic (but not high-resolution) clock source. Maybe it would be better to separate this into two features, so one will not be affected if the other has to be disabled for security reasons.

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 8, 2018

I thought about that, too. At the start it was also about high resolution and no Spectre issue known ofc. Basically it doesn't hurt to revert this patch and reintroduce it in another form. Fun enough - same APIs would be used for just monotonic+milliseconds. Probably should introduce just monotonic_time, same as it is, only delivering milliseconds. I would suggest to do so and see the further happenings with the HW and OS mitigations. What do you think?

In worst case - yes, hosters can disable these functions. Note that microtime() is already exposed, if disabled - it would likely break some apps. But it really stands in the dark right now - if a program can deliver high resolution times, it's not automatically suspicious. Also it'd still require proof it's possible with PHP, or say another language like Python, etc. Node's hrtime() function uses similar APIs. Without a global fix to OSes and hardware, not only shared hostings but also containers and cloud are vulnerable. There are tons of software, that could potentially be called suspicious otherwise.

Thanks.

@beberlei

This comment has been minimized.

beberlei commented Jan 8, 2018

One solution on linux could be to fall back down to clock_monotonic_coarse if you compile php in a different way. This is of course a bit dangerous depending on what people rely on.

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 8, 2018

Probably that doesn't help much. As far i understood, the concern of @mappu is, that PHP could potentially provide an attack possibility. PHP itself, as any other program, would be vulnerable to an attack by a third party software. The poc in Javascript is based firstly on the fact, that the code is compiled to JIT, and secondly - that it relies on a certain browser feature. PHP doesn't compile to JIT yet, for one.

From what i also could read today in many sources, a fix to this issue is only possible on the system and HW level, fe https://de.wikipedia.org/wiki/Spectre_(Sicherheitsl%C3%BCcke)#Gegenma%C3%9Fnahmen. Otherwise no process can be sure. Also, while containers are important today, there are still not only them. The functions can be disabled by admins, if needed. To compare, as the mitigations can introduce performance impacts, RedHat tells it'll be possible to enable them selectively. Clear, it's probably the worst issue the industry ever seen, but the head should still be cold.

I'm nevertheless working on monotonictime() now, reusing the existing code but shrinking the return value to milliseconds, as suggested. If it later turns out the world wants get rid of high resolution timers for some reason, so the existing hrtime() can be deleted before 7.3.

Thanks.

@smalyshev

This comment has been minimized.

Contributor

smalyshev commented Jan 9, 2018

We could also introduce INI_SYSTEM setting with maximum accuracy and have the code here that limits the accuracy of the timer. This way people running single-user but needing good accuracy can set it to high accuracy, and shared system admins could limit the potential for abusing timing oracles with restricting the accuracy while still keeping the monotonity. What do you think?

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 9, 2018

A separate function could have less overhead, that's true. IMHO the idea with the ini sounds better as it gives more flexibility.

As i'd see it, there'd be hrtime.precision setting that would take a value in ns. If the value is zero, no adjustment is done. Otherwise, the value is used to reduce the actual timer. Say, one wants to reduce the timer to 20us precision, so the setting were 20000. Internally we still read the full precision, but do something like (php_hrtime_t)(time / 20000) * 20000 and then proceed further as before.

For the furture, this ini setting could be reused for the others like microtime(), if we ever need to do it. Also not sure about the ini setting name, please suggest. Also need to check for possible cast issues, etc.

Thanks.

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 9, 2018

I was doing some checks now, and seems the calculation is a bit more complicated. Actually for an INI it needs two settings - one is for precision and the other for unit. Say one could deliver nanosecond accuracy still, but the precision would be 20us. Alternatively, the INI could be just like "hrtime.low_resolution" which we could still set to something like 20us or 1ms, etc. In that case, perhaps a separate function would be more clear.

Thanks.

@smalyshev

This comment has been minimized.

Contributor

smalyshev commented Jan 9, 2018

I think it should be something like hrtime.resolution which provides resolution unit in ns - so if it's set to 20000, the measurement is always rounded up to 20us grid. If it's 0, then the measurement is reported as is.

@weltling

This comment has been minimized.

Contributor

weltling commented Jan 10, 2018

Ok, this seems the simplest variant. I probably went too complicated way of thought. To think, like in the 20000 example - one could first reduce to an arbitrary unit, and then round to a given interval. Fe what is done at Mozilla is first reducing to ms, then again to nearest 20us. But that's actually a specification JS has to follow.

I'm now working straight to this simple solution then - hrtime.resolution is expressed in ns, the function delivers ns and optionally reduces to the given interval. The setting is PHP_INI_SYSTEM and set default to zero. Thus admins have the full control. Later on the INI can be reused if needed. The flexibility to not to shrink down to ms is also a big plus, imo.

Thanks.

weltling referenced this pull request Jan 11, 2018

Add possibility to lower timer resolution
The recently discovered security flaw Spectre requires a high resolution
timer. To the today's knowledge, PHP can't be used to create an attack for
this flaw. Still some concerns were raised, that there might be impact in
shared hosting environments. This patch adds a possibility to reduce the
timer resolution by an ini setting, thus giving administrators full
control. Especially, as the flaw was also demonstrated by an abuse of
the JS engine in a browser, Firefox reduced several time sources to 20us.
Any programming language, that doesn't compile to JIT, won't be able to
produce an attack vector for Meltdown and Spectre, at least by todays
knowledge. There are also other factors that say that the security
concern on the hrtime feature is to the big part not justified, still we
aim JIT in the future. Thus, adding a possibility to control the timer
resolution is a good and small enough tradeoff for safety and future.
@weltling

This comment has been minimized.

Contributor

weltling commented Jan 11, 2018

OK, i've pushed c3717d9 and reverted it. We're too short in time after the discovery, there should be a better discussion after some time where the world have seen fixes to Meltdown/Spectre. I think it's sufficient to put this topic on hold and resurrect it anyway in time so a mitigation, if needed, lands in 7.3.

Thanks.

@nicolas-grekas

This comment has been minimized.

Contributor

nicolas-grekas commented Apr 19, 2018

For even higher precision, it would be great to add hrtime() to zend_try_compile_special_func() in zend_compile.c!

(see https://bugs.php.net/76241)

@ntzm ntzm referenced this pull request Aug 2, 2018

Open

[Meta] PHP 7.3 support #3697

0 of 6 tasks complete
@lunter2

This comment has been minimized.

lunter2 commented Dec 17, 2018

Please add to $_SERVER array:
key: REQUEST_HRTIME_FLOAT
value: hrtime(true)

<?php
 // PHP 7.2

 // ... page
 // ... code

 $elapsed=$_SERVER['REQUEST_TIME_FLOAT']-microtime(true);

?>
<?php
 // PHP 7.3

 // ... page
 // ... code

 $elapsed=$_SERVER['REQUEST_HRTIME_FLOAT']-hrtime(true);

?>

$_SERVER['REQUEST_HRTIME_FLOAT'] returns hrtime(true) of script start

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment