191152 2018-11-01 07:35:43 -0700 ERROR: ResourceLoadStatisticsPersistentStorage: Unable to delete statistics file 2018-11-05 09:14:46 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 csaavedra csaavedra bfulgham cdumez commit-queue darin fred.wang mcatanzaro webkit-bug-importer wilander oldest_to_newest 1474091 0 csaavedra 2018-11-01 07:35:43 -0700 ERROR: ResourceLoadStatisticsPersistentStorage: Unable to delete statistics file 1474092 1 353598 csaavedra 2018-11-01 07:36:14 -0700 Created attachment 353598 Patch 1474095 2 csaavedra 2018-11-01 07:41:20 -0700 This is a speculative fix to this error that we're seeing in the WPE and GTK+ ports: ERROR: ResourceLoadStatisticsPersistentStorage: Unable to delete statistics file: /tmp/WebKitTestRunners-l3BJR9/ResourceLoadStatistics/full_browsing_session_resourceLog.plist ../../Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp(311) : void WebKit::ResourceLoadStatisticsPersistentStorage::clear() We don't have resource load statistics yet so I think that the file is not even created. Trying to remove it unconditionally when it's not necessarily created doesn't sound right to me. 1474096 3 fred.wang 2018-11-01 07:47:25 -0700 Do we tests for this change? Maybe we can try unskipping some service-worker tests from bug 175419 and see if that make them pass. 1474102 4 csaavedra 2018-11-01 08:10:53 -0700 I think those are unrelated. They all have an unrelated assertion failure below this message. 1474109 5 353598 cdumez 2018-11-01 08:47:41 -0700 Comment on attachment 353598 Patch r=me 1474113 6 mcatanzaro 2018-11-01 08:55:32 -0700 > Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp:310 > - if (!FileSystem::deleteFile(filePath)) > + if (FileSystem::fileExists(filePath) && !FileSystem::deleteFile(filePath)) Let's say an insane attacker decides to rename the file to something else right before ResourceLoadStatisticsPersistentStorage::clear is called and then move it right back. Unclear what that would accomplish or why anyone would want to try. Supremely silly scenario. So this is a really minor point. But it makes sense to avoid pedantic TOCTOU issues when it's easy to do so. You can just flip the check: if (!FileSystem::deleteFile(filePath) && FileSystem::fileExists(filePath)) RELEASE_LOG_ERROR(...); As a rule, it's better to just try to do the file operation and see if it works or not, that where there's no potential for TOCTOU. 1474114 7 mcatanzaro 2018-11-01 08:56:24 -0700 (In reply to Michael Catanzaro from comment #6) > that where there's no potential for TOCTOU. I meant "that way" 1474116 8 353604 csaavedra 2018-11-01 09:03:17 -0700 Created attachment 353604 Patch for landing 1474131 9 353604 commit-queue 2018-11-01 09:40:23 -0700 Comment on attachment 353604 Patch for landing Clearing flags on attachment: 353604 Committed r237682: <https://trac.webkit.org/changeset/237682> 1474132 10 commit-queue 2018-11-01 09:40:24 -0700 All reviewed patches have been landed. Closing bug. 1474135 11 webkit-bug-importer 2018-11-01 09:42:41 -0700 <rdar://problem/45731874> 1474140 12 wilander 2018-11-01 09:49:26 -0700 Did anyone test that this change didn't regress functionality on Cocoa-platforms? Access to the file system is guarded by sandbox rules and if FileSystem::fileExists(filePath) is not allowed, this code won't work. 1474144 13 cdumez 2018-11-01 09:52:45 -0700 (In reply to John Wilander from comment #12) > Did anyone test that this change didn't regress functionality on > Cocoa-platforms? Access to the file system is guarded by sandbox rules and > if FileSystem::fileExists(filePath) is not allowed, this code won't work. What are your concerns exactly? That the sandbox would allow you to delete the file but not know that it exist? 1474147 14 wilander 2018-11-01 09:57:35 -0700 (In reply to Chris Dumez from comment #13) > (In reply to John Wilander from comment #12) > > Did anyone test that this change didn't regress functionality on > > Cocoa-platforms? Access to the file system is guarded by sandbox rules and > > if FileSystem::fileExists(filePath) is not allowed, this code won't work. > > What are your concerns exactly? That the sandbox would allow you to delete > the file but not know that it exist? No, the case where we want the RELEASE_LOG_ERROR() to happen. If the code is not allowed to call FileSystem::fileExists(filePath) we will never get the log output. 1474212 15 mcatanzaro 2018-11-01 12:15:43 -0700 I'm not sure I understand the concern. Are you worried that if the sandbox policy is misconfigured such that access to filePath is denied, there won't be an error message when deleting it fails? (But surely creating it would also have failed?) 1474219 16 cdumez 2018-11-01 12:21:30 -0700 (In reply to Michael Catanzaro from comment #15) > I'm not sure I understand the concern. Are you worried that if the sandbox > policy is misconfigured such that access to filePath is denied, there won't > be an error message when deleting it fails? (But surely creating it would > also have failed?) I am not super concerned either because if that were to happen, you'd see sandbox violations in the logs that would already tell you that something is wrong. 1474418 17 csaavedra 2018-11-02 00:53:25 -0700 And you have plenty of other uses of the same method as well in the same class, so if you have sandboxing violations related to calling it you have other problems as well: claudio@patanjali:~/git/gnome/WebKit/Source/WebKit/UIProcess$ grep fileExists ResourceLoadStatisticsPersistentStorage.cpp -C 3 String storagePath = storageDirectoryPath(); ASSERT(!storagePath.isEmpty()); if (!FileSystem::fileExists(storagePath)) { if (!FileSystem::makeAllDirectories(storagePath)) { RELEASE_LOG_ERROR(ResourceLoadStatistics, "ResourceLoadStatisticsPersistentStorage: Failed to create directory path %s", storagePath.utf8().data()); return; -- String resourceLogPath = resourceLogFilePath(); ASSERT(!resourceLogPath.isEmpty()); if (!FileSystem::fileExists(resourceLogPath)) return; m_fileMonitor = nullptr; -- ASSERT(!RunLoop::isMain()); String filePath = resourceLogFilePath(); if (filePath.isEmpty() || !FileSystem::fileExists(filePath)) { m_memoryStore.grandfatherExistingWebsiteData([]() { }); monitorDirectoryForNewStatistics(); return; -- stopMonitoringDisk(); if (!FileSystem::deleteFile(filePath) && FileSystem::fileExists(filePath)) RELEASE_LOG_ERROR(ResourceLoadStatistics, "ResourceLoadStatisticsPersistentStorage: Unable to delete statistics file: %s", filePath.utf8().data()); } 1474507 18 mcatanzaro 2018-11-02 08:52:48 -0700 (In reply to Claudio Saavedra from comment #17) > if (!FileSystem::fileExists(storagePath)) { > if (!FileSystem::makeAllDirectories(storagePath)) { (I'd flip checks like this, too, for the same reason.) 1475105 19 darin 2018-11-05 09:14:46 -0800 It’s annoying that we have to make this change and introduce a file system race. Even though it’s almost certainly harmless in this case. It seems to me that on every platform it is likely that the deleteFile function implementation can tell the difference between failure because there is no file and other types of failures; it just doesn’t make this clear to the caller. We should consider changing our deleteFile function to clarify the three distinct outcomes: success, failure because no file exists, other types of failure. Then we would not need to call two functions. Not sure what the best idiom for that is. Maybe returning an value of an enumeration type? Need to make sure that old code that thinks it returns a boolean doesn’t quietly do the wrong thing. 353598 2018-11-01 07:36:14 -0700 2018-11-01 09:03:08 -0700 Patch bug-191152-20181101163613.patch text/plain 1723 csaavedra U3VidmVyc2lvbiBSZXZpc2lvbjogMjM3Njc4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDg1ZDVkYzQ4NWVlZTg5NjI4 MDFjYjYwZWFiMzhhYWMxOWE5ZDg4NTEuLjliMjgwNTRjMDgxYmJmOTdhODliZTFkMWJmNmYxY2M2 OTUzNzgwNjEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTgtMTEtMDEgIENsYXVkaW8g U2FhdmVkcmEgIDxjc2FhdmVkcmFAaWdhbGlhLmNvbT4KKworICAgICAgICBFUlJPUjogUmVzb3Vy Y2VMb2FkU3RhdGlzdGljc1BlcnNpc3RlbnRTdG9yYWdlOiBVbmFibGUgdG8gZGVsZXRlIHN0YXRp c3RpY3MgZmlsZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTkxMTUyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgVGhlIHN0YXRpc3RpY3MgZmlsZSBpcyBub3QgY3JlYXRlZCB1bmNvbmRpdGlvbmFsbHksIHNv CisgICAgICAgIG1ha2Ugc3VyZSBpdCBleGlzdHMgYmVmb3JlIHRyeWluZyB0byBkZWxldGUgaXQu CisKKyAgICAgICAgKiBVSVByb2Nlc3MvUmVzb3VyY2VMb2FkU3RhdGlzdGljc1BlcnNpc3RlbnRT dG9yYWdlLmNwcDoKKyAgICAgICAgKFdlYktpdDo6UmVzb3VyY2VMb2FkU3RhdGlzdGljc1BlcnNp c3RlbnRTdG9yYWdlOjpjbGVhcik6CisgICAgICAgIENoZWNrIGlmIHRoZSBmaWxlIGV4aXN0cyBi ZWZvcmUgZGVsZXRpbmcgaXQuCisKIDIwMTgtMTEtMDEgIENsYXVkaW8gU2FhdmVkcmEgIDxjc2Fh dmVkcmFAaWdhbGlhLmNvbT4KIAogICAgICAgICBbR0xJQl0gU2lsZW50IGEgYnVpbGQgd2Fybmlu ZyB3aGVuIG5vdCB1c2luZyBXYXlsYW5kCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1VJUHJv Y2Vzcy9SZXNvdXJjZUxvYWRTdGF0aXN0aWNzUGVyc2lzdGVudFN0b3JhZ2UuY3BwIGIvU291cmNl L1dlYktpdC9VSVByb2Nlc3MvUmVzb3VyY2VMb2FkU3RhdGlzdGljc1BlcnNpc3RlbnRTdG9yYWdl LmNwcAppbmRleCAwYjkyNmQ1MTA4NTU2YTliNDIxODQ5NzFjZjlkZWYyZjg3YjIwMjg0Li4zYzk4 M2I4NzFjYTNlZTgwZjNiOTU2MDc3ZWMzODk0YWY5YjEwN2RiIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L1VJUHJvY2Vzcy9SZXNvdXJjZUxvYWRTdGF0aXN0aWNzUGVyc2lzdGVudFN0b3JhZ2Uu Y3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL1Jlc291cmNlTG9hZFN0YXRpc3RpY3NQ ZXJzaXN0ZW50U3RvcmFnZS5jcHAKQEAgLTMwNyw3ICszMDcsNyBAQCB2b2lkIFJlc291cmNlTG9h ZFN0YXRpc3RpY3NQZXJzaXN0ZW50U3RvcmFnZTo6Y2xlYXIoKQogCiAgICAgc3RvcE1vbml0b3Jp bmdEaXNrKCk7CiAKLSAgICBpZiAoIUZpbGVTeXN0ZW06OmRlbGV0ZUZpbGUoZmlsZVBhdGgpKQor ICAgIGlmIChGaWxlU3lzdGVtOjpmaWxlRXhpc3RzKGZpbGVQYXRoKSAmJiAhRmlsZVN5c3RlbTo6 ZGVsZXRlRmlsZShmaWxlUGF0aCkpCiAgICAgICAgIFJFTEVBU0VfTE9HX0VSUk9SKFJlc291cmNl TG9hZFN0YXRpc3RpY3MsICJSZXNvdXJjZUxvYWRTdGF0aXN0aWNzUGVyc2lzdGVudFN0b3JhZ2U6 IFVuYWJsZSB0byBkZWxldGUgc3RhdGlzdGljcyBmaWxlOiAlcyIsIGZpbGVQYXRoLnV0ZjgoKS5k YXRhKCkpOwogfQogCg== 353604 2018-11-01 09:03:17 -0700 2018-11-01 09:40:23 -0700 Patch for landing bug-191152-20181101180316.patch text/plain 1724 csaavedra U3VidmVyc2lvbiBSZXZpc2lvbjogMjM3Njc4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDg1ZDVkYzQ4NWVlZTg5NjI4 MDFjYjYwZWFiMzhhYWMxOWE5ZDg4NTEuLmQ3NTRiMjk4ZjkxYjdkZjY3YTI1MzZiZmU1Yjk1Yjk2 NmFkNzFlMjcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTgtMTEtMDEgIENsYXVkaW8g U2FhdmVkcmEgIDxjc2FhdmVkcmFAaWdhbGlhLmNvbT4KKworICAgICAgICBFUlJPUjogUmVzb3Vy Y2VMb2FkU3RhdGlzdGljc1BlcnNpc3RlbnRTdG9yYWdlOiBVbmFibGUgdG8gZGVsZXRlIHN0YXRp c3RpY3MgZmlsZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTkxMTUyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgQ2hyaXMgRHVtZXouCisKKyAgICAgICAg VGhlIHN0YXRpc3RpY3MgZmlsZSBpcyBub3QgY3JlYXRlZCB1bmNvbmRpdGlvbmFsbHksIHNvCisg ICAgICAgIG1ha2Ugc3VyZSBpdCBleGlzdHMgYmVmb3JlIGxvZ2dpbmcgYW4gZXJyb3IuCisKKyAg ICAgICAgKiBVSVByb2Nlc3MvUmVzb3VyY2VMb2FkU3RhdGlzdGljc1BlcnNpc3RlbnRTdG9yYWdl LmNwcDoKKyAgICAgICAgKFdlYktpdDo6UmVzb3VyY2VMb2FkU3RhdGlzdGljc1BlcnNpc3RlbnRT dG9yYWdlOjpjbGVhcik6CisgICAgICAgIENoZWNrIHRoYXQgdGhlIGZpbGUgZXhpc3RzIGJlZm9y ZSBsb2dnaW5nIGFuIGVycm9yLgorCiAyMDE4LTExLTAxICBDbGF1ZGlvIFNhYXZlZHJhICA8Y3Nh YXZlZHJhQGlnYWxpYS5jb20+CiAKICAgICAgICAgW0dMSUJdIFNpbGVudCBhIGJ1aWxkIHdhcm5p bmcgd2hlbiBub3QgdXNpbmcgV2F5bGFuZApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9VSVBy b2Nlc3MvUmVzb3VyY2VMb2FkU3RhdGlzdGljc1BlcnNpc3RlbnRTdG9yYWdlLmNwcCBiL1NvdXJj ZS9XZWJLaXQvVUlQcm9jZXNzL1Jlc291cmNlTG9hZFN0YXRpc3RpY3NQZXJzaXN0ZW50U3RvcmFn ZS5jcHAKaW5kZXggMGI5MjZkNTEwODU1NmE5YjQyMTg0OTcxY2Y5ZGVmMmY4N2IyMDI4NC4uNmY4 NWQzNmY5NDdhYzIxNjQ1MGY3NWMwNTM1ODU2NTY4Njc4NGFmYyAxMDA2NDQKLS0tIGEvU291cmNl L1dlYktpdC9VSVByb2Nlc3MvUmVzb3VyY2VMb2FkU3RhdGlzdGljc1BlcnNpc3RlbnRTdG9yYWdl LmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9SZXNvdXJjZUxvYWRTdGF0aXN0aWNz UGVyc2lzdGVudFN0b3JhZ2UuY3BwCkBAIC0zMDcsNyArMzA3LDcgQEAgdm9pZCBSZXNvdXJjZUxv YWRTdGF0aXN0aWNzUGVyc2lzdGVudFN0b3JhZ2U6OmNsZWFyKCkKIAogICAgIHN0b3BNb25pdG9y aW5nRGlzaygpOwogCi0gICAgaWYgKCFGaWxlU3lzdGVtOjpkZWxldGVGaWxlKGZpbGVQYXRoKSkK KyAgICBpZiAoIUZpbGVTeXN0ZW06OmRlbGV0ZUZpbGUoZmlsZVBhdGgpICYmIEZpbGVTeXN0ZW06 OmZpbGVFeGlzdHMoZmlsZVBhdGgpKQogICAgICAgICBSRUxFQVNFX0xPR19FUlJPUihSZXNvdXJj ZUxvYWRTdGF0aXN0aWNzLCAiUmVzb3VyY2VMb2FkU3RhdGlzdGljc1BlcnNpc3RlbnRTdG9yYWdl OiBVbmFibGUgdG8gZGVsZXRlIHN0YXRpc3RpY3MgZmlsZTogJXMiLCBmaWxlUGF0aC51dGY4KCku ZGF0YSgpKTsKIH0KIAo=