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

Normative: Limit Duration units to prevent arbitrary-precision integer arithmetic, and prevent loops #2612

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Jun 20, 2023

This is the integer math refactor that I have mentioned in the last couple of Temporal presentations.

It is stacked on top of #2519 and #2609; only the last 7 commits belong to this change. The GitHub UI doesn't let me show a comparison with HEAD~7 but here is a view that includes the editorial commits from #2609 but without the large PR #2519: https://github.com/tc39/proposal-temporal/compare/user-code-calls...duration-normalize-96?expand=1

Best reviewed commit by commit. There is more information in each individual commit message.

Test262 tests are in https://github.com/ptomato/test262/commits/duration-normalize but I still need to review them for complete coverage.

See: #2195, #1700.

This is now part 3 of 3 of this change.
Part 1: #2722
Part 2: #2727

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (7bd3589) 96.59% compared to head (891769a) 96.53%.

❗ Current head 891769a differs from pull request most recent head a57b62a. Consider uploading reports for the commit a57b62a to get more accurate results

Files Patch % Lines
polyfill/lib/ecmascript.mjs 81.39% 7 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2612      +/-   ##
==========================================
- Coverage   96.59%   96.53%   -0.06%     
==========================================
  Files          23       23              
  Lines       12218    12243      +25     
  Branches     2263     2268       +5     
==========================================
+ Hits        11802    11819      +17     
- Misses        356      364       +8     
  Partials       60       60              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ptomato
Copy link
Collaborator Author

ptomato commented Jun 20, 2023

Draft until presented at TC39.

cc: @syg, @anba, @rkirsling

Copy link
Contributor

@Mrashes Mrashes left a comment

Choose a reason for hiding this comment

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

Few comments after looking over the PR - new around here but wanted to contribute!

polyfill/lib/duration.mjs Outdated Show resolved Hide resolved
polyfill/lib/duration.mjs Outdated Show resolved Hide resolved
polyfill/lib/timeduration.mjs Outdated Show resolved Hide resolved
@ptomato
Copy link
Collaborator Author

ptomato commented Nov 6, 2023

This is now split into 2 parts. Part 1 is in #2722 and the test262 tests are in tc39/test262#3957.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 7, 2023
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this pull request Nov 7, 2023
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Nov 8, 2023
…y-reviewers,mgaudet

This code will likely be removed when <tc39/proposal-temporal#2612> lands.

Differential Revision: https://phabricator.services.mozilla.com/D189814

UltraBlame original commit: 8941d99e1eb17454a255df18d412ab32963d6aae
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Nov 8, 2023
…y-reviewers,mgaudet

This code will likely be removed when <tc39/proposal-temporal#2612> lands.

Differential Revision: https://phabricator.services.mozilla.com/D189814

UltraBlame original commit: 8941d99e1eb17454a255df18d412ab32963d6aae
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Nov 8, 2023
…y-reviewers,mgaudet

This code will likely be removed when <tc39/proposal-temporal#2612> lands.

Differential Revision: https://phabricator.services.mozilla.com/D189814

UltraBlame original commit: 8941d99e1eb17454a255df18d412ab32963d6aae
@ptomato ptomato force-pushed the duration-normalize-96 branch 2 times, most recently from bdebb14 to 6a79293 Compare November 16, 2023 19:17
@ptomato ptomato force-pushed the duration-normalize-96 branch 2 times, most recently from c11cc80 to 80a09a0 Compare February 1, 2024 00:21
@ptomato
Copy link
Collaborator Author

ptomato commented Feb 2, 2024

The last thing I needed to investigate on this one was some odd behaviour that @cjtenny and @anba discovered when the UTC offset shift is > 24 hours in a custom time zone. (e.g, your UTC offset changes from -13:00 to +13:00, meaning a UTC offset shift of +26 hours.) A shift this large doesn't exist in any actual time zones on earth. (±24:00 is the maximum, on the rare occasions when a region decides to jump to the other side of the International Date Line.)

So what I've done here is to add checks forbidding such transitions. The only place such a transition occurs is in contrived custom time zones, where we also have the precedent of forbidding similar shenanigans (see #2725 for example.) If such a transition ever occurs in future in the real world (quite unlikely), we'd lift those restrictions and also consider running this loop an extra time.

I think all concerns have now been addressed on part 3/3 of this PR and it's ready to be merged once it gets a positive review. Tests are in tc39/test262#3999, which also need a positive review.

It's possible to make at least the second loop continue arbitrarily long
until going out of range, using a contrived custom time zone.

This unrolls the loops and executes them no more than twice.

In order to weed out this situation earlier, when possible, also put a
limit on the maximum possible UTC offset shift:
- For backwards UTC offset shifts, if getPossibleInstantsFor returns more
  than one instant, the difference between the earliest and latest
  instants in the returned array may not be more than 24 hours.
- For forwards UTC offset shifts, if getPossibleInstantsFor returns zero
  instants, the difference between the offsets 24 hours before and after
  returned by getOffsetNanosecondsFor may not be more than 24 hours.
@ptomato ptomato merged commit 133cddc into main Feb 2, 2024
5 checks passed
@ptomato ptomato deleted the duration-normalize-96 branch February 2, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants