198966 2019-06-18 09:03:06 -0700 StorageManager::removeAllowedSessionStorageNamespaceConnection should make sure its storageNamespaceID is valid 2019-07-01 16:58:25 -0700 1 1 1 Unclassified WebKit WebKit Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=199208 https://bugs.webkit.org/show_bug.cgi?id=199388 InRadar P2 Normal --- 1 youennf youennf achristensen beidson commit-queue sihui_liu oldest_to_newest 1545627 0 youennf 2019-06-18 09:03:06 -0700 StorageManager::removeAllowedSessionStorageNamespaceConnection should make sure its storageNamespaceID is valid 1545630 1 youennf 2019-06-18 09:07:17 -0700 <rdar://problem/51352080> 1545632 2 372341 youennf 2019-06-18 09:14:10 -0700 Created attachment 372341 Patch 1545648 3 372341 sihui_liu 2019-06-18 10:10:39 -0700 Comment on attachment 372341 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372341&action=review > Source/WebKit/ChangeLog:10 > + The namespace ID is coming straight from IPC so should not be trusted. I agree with this, but I don't think this crash is caused by untrusted message sent to web process. Likely there is a bug in the code (e.g. web page was removed before it's getting added because of some race), and using the If condition here will conceal the real bug. But as we also don't like the idea of notifying network process when page is added/removed in web process for sessionStorage, this may be a good enough fix before we drop those code. > Source/WebKit/ChangeLog:13 > + Also, namespace IDs are added/removed based on web pages being created/deleted. > + Namespace IDs are supposed to be scoped by session IDs. > + Using page IDs for namespace IDs works as long as the page does not change of session ID during its lifetime, which is not guaranteed. If my understanding is correct, if page is changing its session, it should add itself to new storage session(StorageManager) first, which will create a new sessionStorageNameSpace that have the corresponding namespace/page ID. 1545664 4 youennf 2019-06-18 10:38:25 -0700 > I agree with this, but I don't think this crash is caused by untrusted > message sent to web process. Likely there is a bug in the code (e.g. web > page was removed before it's getting added because of some race), and using > the If condition here will conceal the real bug. > > But as we also don't like the idea of notifying network process when page is > added/removed in web process for sessionStorage, this may be a good enough > fix before we drop those code. Yes, the patch is only about fixing the crash. It should be completed by either retiring the use of page IDs, or making it fully working. I think the end goal should be to retire the use of page IDs here. > > Source/WebKit/ChangeLog:13 > > + Also, namespace IDs are added/removed based on web pages being created/deleted. > > + Namespace IDs are supposed to be scoped by session IDs. > > + Using page IDs for namespace IDs works as long as the page does not change of session ID during its lifetime, which is not guaranteed. > > If my understanding is correct, if page is changing its session, it should > add itself to new storage session(StorageManager) first, which will create a > new sessionStorageNameSpace that have the corresponding namespace/page ID. I am not sure what we want in the short term. Maybe it is ok to fix this one before trying to remove the page ID usage. I fear that we may see other bugs in that area until we remove page ID usage. 1545666 5 youennf 2019-06-18 10:39:35 -0700 > I agree with this, but I don't think this crash is caused by untrusted > message sent to web process. Likely there is a bug in the code (e.g. web > page was removed before it's getting added because of some race), and using > the If condition here will conceal the real bug. There is a debug assertion to catch the real bug. The crash might come from session ID change. 1545678 6 372341 commit-queue 2019-06-18 11:10:14 -0700 Comment on attachment 372341 Patch Clearing flags on attachment: 372341 Committed r246552: <https://trac.webkit.org/changeset/246552> 1545679 7 commit-queue 2019-06-18 11:10:16 -0700 All reviewed patches have been landed. Closing bug. 1547870 8 achristensen 2019-06-25 16:27:31 -0700 *** Bug 199208 has been marked as a duplicate of this bug. *** 372341 2019-06-18 09:14:10 -0700 2019-06-18 11:10:14 -0700 Patch bug-198966-20190618091410.patch text/plain 2300 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ2NDUxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGYyYWFhYjQzYmQ4YzhlOGNm MDlhOGI4M2Y5ODU4YTdiMTQwNmZkMzEuLjFhMGIwNWJjZTIyYTI2YWQ2ZGRkODQxZjFkZTVhNTdk Y2IzMDQyOGIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjAgQEAKKzIwMTktMDYtMTggIFlvdWVubiBG YWJsZXQgIDx5b3Vlbm5AYXBwbGUuY29tPgorCisgICAgICAgIFN0b3JhZ2VNYW5hZ2VyOjpyZW1v dmVBbGxvd2VkU2Vzc2lvblN0b3JhZ2VOYW1lc3BhY2VDb25uZWN0aW9uIHNob3VsZCBtYWtlIHN1 cmUgaXRzIHN0b3JhZ2VOYW1lc3BhY2VJRCBpcyB2YWxpZAorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTk4OTY2CisgICAgICAgIHJkYXI6Ly9wcm9ibGVt LzUxMzUyMDgwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgTWFrZSBzdXJlIHRoZSBuYW1lc3BhY2UgSUQgaXMgYSBrZXkgb2YgdGhlIG1hcCBiZWZvcmUg dXNpbmcgdGhlIHZhbHVlLgorICAgICAgICBUaGUgbmFtZXNwYWNlIElEIGlzIGNvbWluZyBzdHJh aWdodCBmcm9tIElQQyBzbyBzaG91bGQgbm90IGJlIHRydXN0ZWQuCisgICAgICAgIEFsc28sIG5h bWVzcGFjZSBJRHMgYXJlIGFkZGVkL3JlbW92ZWQgYmFzZWQgb24gd2ViIHBhZ2VzIGJlaW5nIGNy ZWF0ZWQvZGVsZXRlZC4KKyAgICAgICAgTmFtZXNwYWNlIElEcyBhcmUgc3VwcG9zZWQgdG8gYmUg c2NvcGVkIGJ5IHNlc3Npb24gSURzLgorICAgICAgICBVc2luZyBwYWdlIElEcyBmb3IgbmFtZXNw YWNlIElEcyB3b3JrcyBhcyBsb25nIGFzIHRoZSBwYWdlIGRvZXMgbm90IGNoYW5nZSBvZiBzZXNz aW9uIElEIGR1cmluZyBpdHMgbGlmZXRpbWUsIHdoaWNoIGlzIG5vdCBndWFyYW50ZWVkLgorCisg ICAgICAgICogTmV0d29ya1Byb2Nlc3MvV2ViU3RvcmFnZS9TdG9yYWdlTWFuYWdlci5jcHA6Cisg ICAgICAgIChXZWJLaXQ6OlN0b3JhZ2VNYW5hZ2VyOjpyZW1vdmVBbGxvd2VkU2Vzc2lvblN0b3Jh Z2VOYW1lc3BhY2VDb25uZWN0aW9uKToKKwogMjAxOS0wNi0xNSAgWW91ZW5uIEZhYmxldCAgPHlv dWVubkBhcHBsZS5jb20+CiAKICAgICAgICAgV2ViUGFnZVByb3h5IHNob3VsZCB1c2UgdGhlIHJp Z2h0IHBhdGggZm9yIHNhbmRib3ggZXh0ZW5zaW9uCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0 L05ldHdvcmtQcm9jZXNzL1dlYlN0b3JhZ2UvU3RvcmFnZU1hbmFnZXIuY3BwIGIvU291cmNlL1dl YktpdC9OZXR3b3JrUHJvY2Vzcy9XZWJTdG9yYWdlL1N0b3JhZ2VNYW5hZ2VyLmNwcAppbmRleCA5 ZjNlNjgxOTVlYmRjYTRiYmY3OTM1ZWZmOTVkOGRiNzBjYTE0YjI2Li5jMjQxZmNkOWFiMmM0ZWJh NmI4ZjMzN2NhMTk2MWQ0Zjg2MTM2ODM2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L05ldHdv cmtQcm9jZXNzL1dlYlN0b3JhZ2UvU3RvcmFnZU1hbmFnZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJL aXQvTmV0d29ya1Byb2Nlc3MvV2ViU3RvcmFnZS9TdG9yYWdlTWFuYWdlci5jcHAKQEAgLTU1MCw4 ICs1NTAsOCBAQCB2b2lkIFN0b3JhZ2VNYW5hZ2VyOjpyZW1vdmVBbGxvd2VkU2Vzc2lvblN0b3Jh Z2VOYW1lc3BhY2VDb25uZWN0aW9uKHVpbnQ2NF90IHN0bwogICAgIGF1dG8gYWxsb3dlZENvbm5l Y3Rpb25JRCA9IGFsbG93ZWRDb25uZWN0aW9uLnVuaXF1ZUlEKCk7CiAgICAgbV9xdWV1ZS0+ZGlz cGF0Y2goW3RoaXMsIHByb3RlY3RlZFRoaXMgPSBtYWtlUmVmKCp0aGlzKSwgYWxsb3dlZENvbm5l Y3Rpb25JRCwgc3RvcmFnZU5hbWVzcGFjZUlEXSgpIG11dGFibGUgewogICAgICAgICBBU1NFUlQo bV9zZXNzaW9uU3RvcmFnZU5hbWVzcGFjZXMuY29udGFpbnMoc3RvcmFnZU5hbWVzcGFjZUlEKSk7 Ci0KLSAgICAgICAgbV9zZXNzaW9uU3RvcmFnZU5hbWVzcGFjZXMuZ2V0KHN0b3JhZ2VOYW1lc3Bh Y2VJRCktPnJlbW92ZUFsbG93ZWRDb25uZWN0aW9uKGFsbG93ZWRDb25uZWN0aW9uSUQpOworICAg ICAgICBpZiAoYXV0byogc2Vzc2lvblN0b3JhZ2VOYW1lc3BhY2UgPSBtX3Nlc3Npb25TdG9yYWdl TmFtZXNwYWNlcy5nZXQoc3RvcmFnZU5hbWVzcGFjZUlEKSkKKyAgICAgICAgICAgIHNlc3Npb25T dG9yYWdlTmFtZXNwYWNlLT5yZW1vdmVBbGxvd2VkQ29ubmVjdGlvbihhbGxvd2VkQ29ubmVjdGlv bklEKTsKICAgICB9KTsKIH0KIAo=