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.

Sign up for GitHub

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

Jump to bottom

WTF: Fix unsigned overflows/underflows in string conversion #13308

Merged
merged 1 commit into from May 9, 2023
Merged

WTF: Fix unsigned overflows/underflows in string conversion #13308

merged 1 commit into from May 9, 2023

Conversation

AtariDreams
Copy link
Contributor

@AtariDreams AtariDreams commented Apr 29, 2023 •
edited by webkit-early-warning-system

a83ee57

WTF: Fix unsigned overflows/underflows in string conversion
https://bugs.webkit.org/show_bug.cgi?id=256164

Reviewed by David Kilzer.

Although we check that integer is less than 0 before negating it, the
UBSanitizer keeps tripping up at this section. The proper fix would be
to use std::make_unsigned_t<IntegerType> here, to convert the variable
to its unsigned equivalent.

* Source/WTF/wtf/text/IntegerToStringConversion.h:
  (writeIntegerToBuffer): Cast to unsigned.

Canonical link: https://commits.webkit.org/263866@main

713b9e6

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🛠 jsc-mips
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash ea2d616)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt   🧪 mac-wk1   🧪 gtk-wk2
⏳ 🛠 🧪 jsc ✅ 🧪 api-ios   🧪 mac-wk2   🧪 api-gtk
⏳ 🛠 🧪 jsc-arm64 ✅ 🛠 tv   🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 8813208)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🧪 api-gtk
⏳ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 29, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 7b64c7a)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ❌ 🧪 mac-wk2 ✅ 🧪 api-gtk
⏳ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 8f3ba5b)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
⏳ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@AtariDreams AtariDreams changed the title Fix unsigned overflows/underflows WTF: Fix unsigned overflows/underflows in string conversion May 1, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 8a9e745)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac ✅ 🛠 gtk
  🧪 ios-wk2-wpt   🧪 mac-wk1   🧪 gtk-wk2
✅ 🛠 🧪 jsc   🧪 api-ios   🧪 mac-wk2   🧪 api-gtk
⏳ 🛠 🧪 jsc-arm64 ✅ 🛠 tv   🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim   🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🛠 jsc-mips
✅ 🛠 watch-sim   🧪 jsc-mips-tests

@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 713b9e6)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🛠 jsc-mips
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@Ahmad-S792 Ahmad-S792 added Platform Portability improvements and other general platform improvements not driven directly by site bugs. and removed merging-blocked Applied to prevent a change from being merged labels May 1, 2023
@justinmichaud justinmichaud added the skip-ews Applied to prevent a change from being run on EWS label May 3, 2023
ddkilzer
ddkilzer requested changes May 9, 2023
View reviewed changes
Copy link
Contributor

@ddkilzer ddkilzer left a comment

Choose a reason for hiding this comment

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

Please update the PR to use static_cast<>() with std::make_unsigned_t<>.

Source/WTF/wtf/text/IntegerToStringConversion.h Show resolved Hide resolved
@ddkilzer ddkilzer self-requested a review May 9, 2023 15:52
ddkilzer
ddkilzer approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@ddkilzer ddkilzer left a comment

Choose a reason for hiding this comment

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

r=me

@ddkilzer ddkilzer added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 9, 2023
@AtariDreams @ddkilzer
WTF: Fix unsigned overflows/underflows in string conversion
a83ee57 
https://bugs.webkit.org/show_bug.cgi?id=256164

Reviewed by David Kilzer.

Although we check that integer is less than 0 before negating it, the
UBSanitizer keeps tripping up at this section. The proper fix would be
to use std::make_unsigned_t<IntegerType> here, to convert the variable
to its unsigned equivalent.

* Source/WTF/wtf/text/IntegerToStringConversion.h:
  (writeIntegerToBuffer): Cast to unsigned.

Canonical link: https://commits.webkit.org/263866@main
@webkit-commit-queue
Copy link
Collaborator

Committed 263866@main (a83ee57): https://commits.webkit.org/263866@main

Reviewed commits have been landed. Closing PR #13308 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 9, 2023
@webkit-commit-queue webkit-commit-queue merged commit a83ee57 into WebKit:main May 9, 2023
@AtariDreams AtariDreams deleted the Overflowing branch May 30, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddkilzer ddkilzer ddkilzer approved these changes

Assignees

@AtariDreams AtariDreams

Labels
Platform Portability improvements and other general platform improvements not driven directly by site bugs. skip-ews Applied to prevent a change from being run on EWS
Projects
None yet
Milestone
No milestone
7 participants
@AtariDreams @webkit-early-warning-system @webkit-commit-queue @ddkilzer @justinmichaud @Ahmad-S792 @webkit-ews-buildbot