Skip to content

[JSC] Fix Uint8Array fromBase64 and setFromBase64 for invalid 1-chunk input #47926

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
Merged
webkit-commit-queue merged 1 commit into from
Jul 14, 2025
Merged

[JSC] Fix Uint8Array fromBase64 and setFromBase64 for invalid 1-chunk input #47926

webkit-commit-queue merged 1 commit into from
Jul 14, 2025

Conversation

@syg
Copy link
Contributor

@syg syg commented Jul 12, 2025 •
edited by webkit-early-warning-system
Loading

c5b50b5

[JSC] Fix Uint8Array fromBase64 and setFromBase64 for invalid 1-chunk input
https://bugs.webkit.org/show_bug.cgi?id=295578
rdar://155346981

Reviewed by Yusuke Suzuki.

This PR updates simdutf to 7.3.3, fixes handling of invalid 1-chunk input in
base64 decoding, and removes the slow fallback.

First, base64 input is changed to unconditionally decode even when the expected
decoded binary size is 0. Previously, there was a fast path in fromBase64 input
to an empty Uint8Array when the expected decoded binary size is 0. This is
incorrect when the input contains a single invalid chunk, as it skips decoding
entirely when it should attempt to and report the error.

Second, use simdutf to decode base64 for all values of lastChunkHandling.
Previously, simdutf was only used for 'loose', and a slow fallback was used for
'strict' and 'stop-before-partial'. Now simdutf has support for all 3, so use
it and remove the fallback.

Note that simdutf currently has a bug for the read count when lastChunkHandling
is 'stop-before-partial', which this patch works around.

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

33e2abc

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@syg syg requested a review from a team as a code owner July 12, 2025 01:27
@syg syg self-assigned this Jul 12, 2025
@syg syg added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Jul 12, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 12, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 8614a8a)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe   🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ❌ 🧪 wpe-wk2   🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac ❌ 🧪 api-wpe
  🧪 ios-wk2-wpt ⏳ 🧪 mac-wk1 ❌ 🛠 wpe-cairo
  🛠 🧪 jsc   🧪 api-ios   🧪 mac-wk2   🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
  🧪 vision-wk2 ⏳ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ❌ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 12, 2025
@syg syg removed the merging-blocked Applied to prevent a change from being merged label Jul 12, 2025
@syg syg force-pushed the dev/base64-reject-partial branch from 8614a8a to 6cb6e91 Compare July 12, 2025 01:40
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 12, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 6cb6e91)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

Constellation
Constellation reviewed Jul 12, 2025
View reviewed changes
@lemire
Copy link

lemire commented Jul 12, 2025

This should get fixed in the coming days upstream. @syg provided a PR at simdutf/simdutf#822

@lemire lemire mentioned this pull request Jul 12, 2025
Merged
@lemire
Copy link

lemire commented Jul 12, 2025

Please see simdutf/simdutf#823

@syg
Copy link
Contributor Author

syg commented Jul 13, 2025

This should get fixed in the coming days upstream. @syg provided a PR at simdutf/simdutf#822

To clarify I did not provide a fix, just imported the test262 test that shows the failure.

@syg syg force-pushed the dev/base64-reject-partial branch from 6cb6e91 to aeb3e7c Compare July 13, 2025 23:40
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 13, 2025 •
edited
Loading

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

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
❌ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
❌ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@syg syg force-pushed the dev/base64-reject-partial branch from aeb3e7c to 372b19a Compare July 14, 2025 03:58
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 14, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 372b19a)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc   🧪 api-ios   🧪 mac-wk2   🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision   🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
  🧪 vision-wk2   🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim   🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@syg syg force-pushed the dev/base64-reject-partial branch from 372b19a to 33e2abc Compare July 14, 2025 04:09
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 14, 2025 •
edited
Loading

EWS run on current version of this PR (hash 33e2abc)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2   🛠 playstation
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

syg
syg commented Jul 14, 2025
View reviewed changes
Source/WTF/wtf/text/Base64.cpp
static inline size_t fixSIMDUTFStopBeforePartialReadLength(std::span<const CharacterType> span, size_t readLengthFromSIMDUTF)
{
// Work around simdutf bug for stop-before-partial read length.
// FIXME: Remove once fixed in simdutf.
Copy link
Contributor Author

@syg syg Jul 14, 2025

Choose a reason for hiding this comment

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

@Constellation I chose to work around simdutf's non-compliant behavior for the read length instead of keeping the entire slow path. WDYT?

@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jul 14, 2025
@webkit-commit-queue webkit-commit-queue force-pushed the dev/base64-reject-partial branch from 33e2abc to 5353d81 Compare July 14, 2025 15:14
@syg @Constellation
[JSC] Fix Uint8Array fromBase64 and setFromBase64 for invalid 1-chunk…
c5b50b5 
… input

https://bugs.webkit.org/show_bug.cgi?id=295578
rdar://155346981

Reviewed by Yusuke Suzuki.

This PR updates simdutf to 7.3.3, fixes handling of invalid 1-chunk input in
base64 decoding, and removes the slow fallback.

First, base64 input is changed to unconditionally decode even when the expected
decoded binary size is 0. Previously, there was a fast path in fromBase64 input
to an empty Uint8Array when the expected decoded binary size is 0. This is
incorrect when the input contains a single invalid chunk, as it skips decoding
entirely when it should attempt to and report the error.

Second, use simdutf to decode base64 for all values of lastChunkHandling.
Previously, simdutf was only used for 'loose', and a slow fallback was used for
'strict' and 'stop-before-partial'. Now simdutf has support for all 3, so use
it and remove the fallback.

Note that simdutf currently has a bug for the read count when lastChunkHandling
is 'stop-before-partial', which this patch works around.

Canonical link: https://commits.webkit.org/297335@main
@webkit-commit-queue webkit-commit-queue force-pushed the dev/base64-reject-partial branch from 5353d81 to c5b50b5 Compare July 14, 2025 15:15
@webkit-commit-queue
Copy link
Collaborator

webkit-commit-queue commented Jul 14, 2025

Committed 297335@main (c5b50b5): https://commits.webkit.org/297335@main

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

@webkit-commit-queue webkit-commit-queue merged commit c5b50b5 into WebKit:main Jul 14, 2025
@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 Jul 14, 2025
@lemire
Copy link

lemire commented Jul 14, 2025

@syg

To further clarify... yes, you 'only' provided the test. But this is obviously helpful as the issue was immediately reproducible.

@syg syg deleted the dev/base64-reject-partial branch July 18, 2025 17:49
@dynst dynst mentioned this pull request Oct 11, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Constellation Constellation Constellation approved these changes

Assignees

@syg syg

Labels

JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@syg @webkit-early-warning-system @lemire @webkit-commit-queue @Constellation @webkit-ews-buildbot