Bug 187270 – [Cocoa] Disable vnode guard related simulated crashes for WKTR / DRT and WebSQL

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Disable vnode guard related simulated crashes for WKTR / DRT and WebSQL.

[Chris Dumez]

<rdar://problem/40674034>

[Chris Dumez]

Created attachment 344149 [details]
Patch

[Chris Dumez]

Created attachment 344150 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 344150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344150&action=review

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:454
> +String realPath(const String& filePath)
> +{
> +    CString fsRep = fileSystemRepresentation(filePath);
> +    char resolvedName[PATH_MAX];
> +    const char* result = realpath(fsRep.data(), resolvedName);
> +    return result ? String::fromUTF8(result) : filePath;
> +}

This doesn't look right to me. The idea behind fileSystemRepresentation() is that this is what one passes to POSIX functions, but here, the result is round-tripped through String::fromUTF8() and then utf8(). I don't really have an example of when this would fail, but this is conceptually incorrect and of course wasteful.

[Chris Dumez]

Comment on attachment 344150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344150&action=review

>> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:454
>> +}
> 
> This doesn't look right to me. The idea behind fileSystemRepresentation() is that this is what one passes to POSIX functions, but here, the result is round-tripped through String::fromUTF8() and then utf8(). I don't really have an example of when this would fail, but this is conceptually incorrect and of course wasteful.

I am passing the result of fileSystemRepresentation() to a Posix function: realpath().
realpath() returns a const char* so I have to call String::fromUTF8() to return a String.

I do not see what is wrong with my code.

Also, if you look at the implementation of directoryName(), it is extremely similar:
String directoryName(const String& path)
{
    CString fsRep = fileSystemRepresentation(path);

    if (!fsRep.data() || fsRep.data()[0] == '\0')
        return String();

    return String::fromUTF8(dirname(fsRep.mutableData()));
}

[Chris Dumez]

Created attachment 344196 [details]
Patch

[Geoffrey Garen]

Comment on attachment 344196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344196&action=review

> Source/WebCore/Modules/webdatabase/cocoa/DatabaseManagerCocoa.mm:43
> +    setenv("SQLITE_EXEMPT_PATH_FROM_VNODE_GUARDS", FileSystem::realPath(databasePath).utf8().data(), 0 /* overwrite */);

A minor point, but I prefer this style for giving symbolic names to constant arguments:

int overwrite = 0;
setenv("SQLITE_EXEMPT_PATH_FROM_VNODE_GUARDS", FileSystem::realPath(databasePath).utf8().data(), overwrite);

It's nice when we can document our intent in code. Code has some nice advantages over comments, like compiler validation and debugging and so on.

[Geoffrey Garen]

> The idea behind fileSystemRepresentation() is
> that this is what one passes to POSIX functions

This function calls fileSystemRepresentation() in order to pass the result to the POSIX function realpath(). So, I think that part of the function matches your expectation.

> but here, the result is
> round-tripped through String::fromUTF8() and then utf8(). I don't really
> have an example of when this would fail, but this is conceptually incorrect
> and of course wasteful.

I think your comment here is about the return value type?

Most functions in FileSystemPOSIX.cpp return String.

You could imagine returning CString from this function because that's what POSIX provides, and that's what our caller wants. 

On the other hand, there's a conflict here between POSIX and Windows. Windows returns a buffer of UChar (i.e. String). So returning CString would require getFinalPathName() in FileSystemWin to convert from UChar buffer to CString. Maybe that's the right tradeoff to make POSIX slightly faster?

[Chris Dumez]

Created attachment 344215 [details]
Patch

[Geoffrey Garen]

Comment on attachment 344215 [details]
Patch

I’m going to say r+ to move this fix along — but we can continue to discuss whether there’s a good way to avoid utf8 round tripping.

[WebKit Commit Bot]

Comment on attachment 344215 [details]
Patch

Clearing flags on attachment: 344215

Committed r233487: <https://trac.webkit.org/changeset/233487>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

I like Geoff's suggestion. Alternatively, maybe there is a way to restructure code so that it doesn't even need to be called on Windows. Realpath is a POSIX concept, so adding a Windows function with this name which does nothing is quite misleading.

Or maybe we need to actually do something on Windows too, like https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getfullpathnamea

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #13)
> I like Geoff's suggestion. Alternatively, maybe there is a way to
> restructure code so that it doesn't even need to be called on Windows.
> Realpath is a POSIX concept, so adding a Windows function with this name
> which does nothing is quite misleading.
> Or maybe we need to actually do something on Windows too, like
> https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-
> getfullpathnamea

This is inaccurate, my patch did provide a meaningful Windows implementation.
Note that the very first patch you took a look at did not have a review flag and the lack of windows implementation was one of the reason.

[Geoffrey Garen]

> Or maybe we need to actually do something on Windows too, like
> https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-
> getfullpathnamea

Right, it seems clear that Windows does have a concept of "~" and similar abbreviations in file names.