Skip to content

Conversation

@szewai
Copy link
Contributor

@szewai szewai commented Nov 6, 2025

581d4c4

Null pointer dereference in LegacyWebArchive::protectedMainResource()
https://bugs.webkit.org/show_bug.cgi?id=302063
rdar://164088405

Reviewed by Ryosuke Niwa and Anne van Kesteren.

Archive::mainResource() can return nullptr and LegacyWebArchive::protectedMainResource() currently defererences it
without null check, so we are seeing crashes. However, LegacyWebArchive should not be created with null main resource,
as it cannot be loaded anyways. So instead of adding null check in protectedMainResource(), this patch fixes the crash
by ensuring LegacyWebArchive is always created non-null main resource: LegacyWebArchive::create*() functions now returns
nullptr instead of default LegacyWebArchive object on error.

* Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:
(WebCore::LegacyWebArchive::create):
(WebCore::LegacyWebArchive::createInternal):
(WebCore::LegacyWebArchive::extract): Deleted.
* Source/WebCore/loader/archive/cf/LegacyWebArchive.h:
* Source/WebKitLegacy/mac/WebView/WebArchive.mm:
(-[WebArchivePrivate init]):

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

ebd2137

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

@szewai szewai self-assigned this Nov 6, 2025
@szewai szewai added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Nov 6, 2025
@szewai szewai marked this pull request as ready for review November 6, 2025 07:27
@szewai szewai requested a review from cdumez as a code owner November 6, 2025 07:27
@szewai szewai requested a review from rniwa November 6, 2025 07:27
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 6, 2025
addSubframeArchive(WTFMove(subframeArchive));
else
LOG(Archives, "LegacyWebArchive - Invalid subframe archive skipped");
if (auto subFrameArchive = create(subframeDict))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (auto subFrameArchive = create(subframeDict))
if (Ref subFrameArchive = create(subframeDict))

@szewai szewai removed the merging-blocked Applied to prevent a change from being merged label Nov 6, 2025
@szewai szewai force-pushed the eng/Null-pointer-dereference-in-LegacyWebArchive-protectedMainResource branch from eae6715 to ebd2137 Compare November 6, 2025 20:04
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 6, 2025
@szewai szewai added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Nov 7, 2025
https://bugs.webkit.org/show_bug.cgi?id=302063
rdar://164088405

Reviewed by Ryosuke Niwa and Anne van Kesteren.

Archive::mainResource() can return nullptr and LegacyWebArchive::protectedMainResource() currently defererences it
without null check, so we are seeing crashes. However, LegacyWebArchive should not be created with null main resource,
as it cannot be loaded anyways. So instead of adding null check in protectedMainResource(), this patch fixes the crash
by ensuring LegacyWebArchive is always created non-null main resource: LegacyWebArchive::create*() functions now returns
nullptr instead of default LegacyWebArchive object on error.

* Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:
(WebCore::LegacyWebArchive::create):
(WebCore::LegacyWebArchive::createInternal):
(WebCore::LegacyWebArchive::extract): Deleted.
* Source/WebCore/loader/archive/cf/LegacyWebArchive.h:
* Source/WebKitLegacy/mac/WebView/WebArchive.mm:
(-[WebArchivePrivate init]):

Canonical link: https://commits.webkit.org/302691@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Null-pointer-dereference-in-LegacyWebArchive-protectedMainResource branch from ebd2137 to 581d4c4 Compare November 7, 2025 00:18
@webkit-commit-queue
Copy link
Collaborator

Committed 302691@main (581d4c4): https://commits.webkit.org/302691@main

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

@webkit-commit-queue webkit-commit-queue merged commit 581d4c4 into WebKit:main Nov 7, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New Bugs Unclassified bugs are placed in this component until the correct component can be determined.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants