ERROR: ResourceLoadStatisticsPersistentStorage: Unable to delete statistics file

[Claudio Saavedra]

ERROR: ResourceLoadStatisticsPersistentStorage: Unable to delete statistics file

[Claudio Saavedra]

Created attachment 353598 [details]
Patch

[Claudio Saavedra]

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.

[Frédéric Wang (:fredw)]

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.

[Claudio Saavedra]

I think those are unrelated. They all have an unrelated assertion failure below this message.

[Chris Dumez]

Comment on attachment 353598 [details]
Patch

r=me

[Michael Catanzaro]

> 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.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #6)
> that where there's no potential for TOCTOU.

I meant "that way"

[Claudio Saavedra]

Created attachment 353604 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 353604 [details]
Patch for landing

Clearing flags on attachment: 353604

Committed r237682: <https://trac.webkit.org/changeset/237682>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45731874>

[John Wilander]

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.

[Chris Dumez]

(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?

[John Wilander]

(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.

[Michael Catanzaro]

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?)

[Chris Dumez]

(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.

[Claudio Saavedra]

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());
}

[Michael Catanzaro]

(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.)

[Darin Adler]

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.