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

Possible value overflow in erts_time_unit_conversion #8186

Closed
cocoa-xu opened this issue Feb 25, 2024 · 4 comments
Closed

Possible value overflow in erts_time_unit_conversion #8186

cocoa-xu opened this issue Feb 25, 2024 · 4 comments
Assignees
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM

Comments

@cocoa-xu
Copy link
Contributor

cocoa-xu commented Feb 25, 2024

Describe the bug
The result of the converted time can exceed the maximum possible value of uint64_t.

To Reproduce
For an instance, on my ARM64 Windows machine (this issue can happen on any machine that meets the following conditions), it doesn't have the macro ERTS_COMPILE_TIME_MONOTONIC_TIME_UNIT defined, and the ERTS_MONOTONIC_TIME_UNIT is initialised to 24000000 in erts_init_sys_time_sup

void erts_init_sys_time_sup(void)
{
ErtsSysInitTimeResult sys_init_time_res
= ERTS_SYS_INIT_TIME_RESULT_INITER;
sys_init_time(&sys_init_time_res);
erts_time_sup__.r.o.monotonic_time_unit
= sys_init_time_res.os_monotonic_time_unit;

The converted value using nano seconds as the time unit can overflow in

erts_time_sup__.r.o.start_offset.nsec = (ErtsMonotonicTime)
erts_time_unit_conversion((Uint64) abs_native_offset,
(Uint32) ERTS_MONOTONIC_TIME_UNIT,
(Uint32) 1000*1000*1000);

Let's say the abs_native_offset is 576460752312000000, the above code is equivalent to

	erts_time_sup__.r.o.start_offset.nsec = (ErtsMonotonicTime)
	erts_time_unit_conversion((Uint64) 576460752312000000,
				  (Uint32) 24000000,
				  (Uint32) 1000*1000*1000);

The erts_time_unit_conversion is expecting to return 24019198012999999488 (btw it's impossible because this value cannot be representable using a uint64_t)

> trunc(576460752312000000 / 24000000 * 1000*1000*1000).
24019198012999999488

In this case, in erts_time_unit_conversion, high will be 5592405333 (16#1_4D55_5555) before left shift 32bits, which will overflow after the bit shift operation.

high /= from_time_unit;
high <<= 32;

Expected behavior
erts_time_unit_conversion can handle large integers without overflow.

Affected versions
Tested on OTP 27.0-rc1. But this issue should exist in all OTP versions.

Additional context
poc.c

@cocoa-xu cocoa-xu added the bug Issue is reported as a bug label Feb 25, 2024
@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Feb 26, 2024
@jhogberg jhogberg assigned jhogberg and unassigned FieldsOfGold06 Mar 13, 2024
@jhogberg
Copy link
Contributor

jhogberg commented Mar 13, 2024

Thanks for your report! As far as I can tell the result is correct: it's supposed to calculate the converted time modulo 2 ** 64. This should not cause issues beyond a certain optimization going away, so you might have run into a bug where some code didn't expect this.

How did you discover this? Did something crash?

@cocoa-xu
Copy link
Contributor Author

cocoa-xu commented Mar 13, 2024

How did you discover this? Did something crash?

I was trying to build Erlang/OTP for ARM64 Windows (#8142). And I found that the testcase, tests.emulator_test.time_SUITE.time_unit_conversion, failed because this value will overflow when converting time using nanoseconds as the time unit.

Screenshot 2024-03-13 at 21 45 55

jhogberg added a commit that referenced this issue Mar 25, 2024
…-19036' into maint

* john/erts/fix-win64-monotonic-time-overflow/GH-8186/OTP-19036:
  erts: Fix overflow when calculating monotonic start_offset
u3s pushed a commit that referenced this issue Apr 9, 2024
…-19036' into maint-24

* john/erts/fix-win64-monotonic-time-overflow/GH-8186/OTP-19036:
  erts: Fix overflow when calculating monotonic start_offset
rickard-green pushed a commit that referenced this issue Apr 12, 2024
…-19036' into maint-26

* john/erts/fix-win64-monotonic-time-overflow/GH-8186/OTP-19036:
  erts: Fix overflow when calculating monotonic start_offset
rickard-green pushed a commit that referenced this issue Apr 12, 2024
…-19036' into maint-25

* john/erts/fix-win64-monotonic-time-overflow/GH-8186/OTP-19036:
  erts: Fix overflow when calculating monotonic start_offset
@jhogberg
Copy link
Contributor

Thanks again for your report, we merged a fix three weeks ago but I forgot to make a note of it here. :)

@cocoa-xu
Copy link
Contributor Author

Thanks again for your report, we merged a fix three weeks ago but I forgot to make a note of it here. :)

Thank you very much for the update! ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM
Projects
None yet
Development

No branches or pull requests

4 participants