Attachment 361041 Details for Bug 194213 – Patch

[patch] Patch

bug-194213-20190204081839.patch (text/plain), 16.31 KB, created by Zan Dobersek on 2019-02-03 23:18:40 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Zan Dobersek

Created: 2019-02-03 23:18:40 PST

Size: 16.31 KB

patch

obsolete

>Subversion Revision: 240914
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 13ca9221fd9e1e2038fe424b9f613caaf0845e31..46fcf998ae1030b77875085ef6f4926f5a633ffd 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,53 @@
>+2019-02-03  Zan Dobersek  <zdobersek@igalia.com>
>+
>+        [GLib] Stop URI-escaping file system representations
>+        https://bugs.webkit.org/show_bug.cgi?id=194213
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Stop URI-escaping of file representation strings in
>+        FileSystem::stringFromFileSystemRepresentation(), and URI-unescaping
>+        of strings in FileSystem::fileSystemRepresentation().
>+
>+        This behavior deviates from POSIX and CF implementations and is
>+        currently breaking IndexedDB-specific calculation of database sizes due
>+        to directory components used in that process that are URL-based and are
>+        as such URI-escaped. When unescaped, those single directory components
>+        explode into multiple directory components, leading to incorrect total
>+        database size calculation when iterating the database directory.
>+
>+        FileSystem::stringFromFileSystemRepresentation() now retrieves GLib's
>+        filename charsets and in worst case converts the filesystem
>+        representation to UTF-8 before String::fromUTF8() is used.
>+        FileSystem::fileSystemRepresentation() reverses that process, taking
>+        String's UTF-8 data and converting it to target charset if necessary.
>+
>+        Other FileSystem functions are adjusted to convert passed-in String
>+        objects to filesystem representations.
>+
>+        * wtf/glib/FileSystemGlib.cpp:
>+        (WTF::FileSystemImpl::stringFromFileSystemRepresentation):
>+        (WTF::FileSystemImpl::fileSystemRepresentation):
>+        (WTF::FileSystemImpl::validRepresentation):
>+        (WTF::FileSystemImpl::filenameForDisplay):
>+        (WTF::FileSystemImpl::fileExists):
>+        (WTF::FileSystemImpl::deleteFile):
>+        (WTF::FileSystemImpl::deleteEmptyDirectory):
>+        (WTF::FileSystemImpl::getFileStat):
>+        (WTF::FileSystemImpl::getFileLStat):
>+        (WTF::FileSystemImpl::makeAllDirectories):
>+        (WTF::FileSystemImpl::createSymbolicLink):
>+        (WTF::FileSystemImpl::pathGetFileName):
>+        (WTF::FileSystemImpl::getVolumeFreeSpace):
>+        (WTF::FileSystemImpl::directoryName):
>+        (WTF::FileSystemImpl::listDirectory):
>+        (WTF::FileSystemImpl::openFile):
>+        (WTF::FileSystemImpl::moveFile):
>+        (WTF::FileSystemImpl::hardLinkOrCopyFile):
>+        (WTF::FileSystemImpl::getFileDeviceId): Align with POSIX implementation
>+        and treat input CString as an existing filesystem representation.
>+        (WTF::FileSystemImpl::unescapedFilename): Deleted.
>+
> 2019-02-03  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r240896.
>diff --git a/Source/WTF/wtf/glib/FileSystemGlib.cpp b/Source/WTF/wtf/glib/FileSystemGlib.cpp
>index c3ec99a3d1b491eb19631e0058a41d1a7516fe5c..a7231429448375f98a0bd33c4c55b63e28c3668c 100644
>--- a/Source/WTF/wtf/glib/FileSystemGlib.cpp
>+++ b/Source/WTF/wtf/glib/FileSystemGlib.cpp
>@@ -34,6 +34,7 @@
> #include <wtf/glib/GLibUtilities.h>
> #include <wtf/glib/GRefPtr.h>
> #include <wtf/glib/GUniquePtr.h>
>+#include <wtf/text/ASCIIFastPath.h>
> #include <wtf/text/CString.h>
> #include <wtf/text/StringBuilder.h>
> #include <wtf/text/WTFString.h>
>@@ -42,42 +43,57 @@ namespace WTF {
> 
> namespace FileSystemImpl {
> 
>-/* On linux file names are just raw bytes, so also strings that cannot be encoded in any way
>- * are valid file names. This mean that we cannot just store a file name as-is in a String
>- * but we have to escape it.
>- * On Windows the GLib file name encoding is always UTF-8 so we can optimize this case. */
>-String stringFromFileSystemRepresentation(const char* fileSystemRepresentation)
>+String stringFromFileSystemRepresentation(const char* representation)
> {
>-    if (!fileSystemRepresentation)
>-        return String();
>+    if (!representation)
>+        return { };
> 
>-#if OS(WINDOWS)
>-    return String::fromUTF8(fileSystemRepresentation);
>-#else
>-    GUniquePtr<gchar> escapedString(g_uri_escape_string(fileSystemRepresentation, "/:", FALSE));
>-    return escapedString.get();
>-#endif
>+    // Short-cut to String creation when only ASCII characters are used.
>+    size_t representationLength = strlen(representation);
>+    if (charactersAreAllASCII(reinterpret_cast<const LChar*>(representation), representationLength))
>+        return String(representation, representationLength);
>+
>+    // If the returned charset is UTF-8, go directly to String creation.
>+    const gchar** filenameCharsets = nullptr;
>+    if (g_get_filename_charsets(&filenameCharsets))
>+        return String::fromUTF8(representation, representationLength);
>+
>+    ASSERT(filenameCharsets);
>+    size_t utf8Length = 0;
>+    GUniquePtr<gchar> utf8(g_convert(representation, representationLength,
>+        "UTF-8", filenameCharsets[0], nullptr, &utf8Length, nullptr));
>+    if (!utf8)
>+        return { };
>+
>+    return String::fromUTF8(utf8.get(), utf8Length);
> }
> 
>-static GUniquePtr<char> unescapedFilename(const String& path)
>+CString fileSystemRepresentation(const String& path)
> {
>     if (path.isEmpty())
>-        return nullptr;
>-#if OS(WINDOWS)
>-    return GUniquePtr<char>(g_strdup(path.utf8().data()));
>-#else
>-    return GUniquePtr<char>(g_uri_unescape_string(path.utf8().data(), nullptr));
>-#endif
>+        return { };
>+
>+    CString utf8 = path.utf8();
>+
>+    // If the returned charset is UTF-8, simply return the CString object.
>+    const gchar** filenameCharsets = nullptr;
>+    if (g_get_filename_charsets(&filenameCharsets))
>+        return utf8;
>+
>+    ASSERT(filenameCharsets);
>+    size_t representationLength = 0;
>+    GUniquePtr<gchar> representation(g_convert(utf8.data(), utf8.length(),
>+        filenameCharsets[0], "UTF-8", nullptr, &representationLength, nullptr));
>+    if (!representation)
>+        return { };
>+
>+    return CString(representation.get(), representationLength);
> }
> 
>-CString fileSystemRepresentation(const String& path)
>+bool validRepresentation(const CString& representation)
> {
>-#if OS(WINDOWS)
>-    return path.utf8();
>-#else
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    return filename.get();
>-#endif
>+    auto* data = representation.data();
>+    return !!data && data[0] != '\0';
> }
> 
> // Converts a string to something suitable to be displayed to the user.
>@@ -86,52 +102,51 @@ String filenameForDisplay(const String& string)
> #if OS(WINDOWS)
>     return string;
> #else
>-    GUniquePtr<gchar> filename = unescapedFilename(string);
>-    if (!filename)
>+    auto filename = fileSystemRepresentation(string);
>+    if (!validRepresentation(filename))
>         return string;
> 
>-    GUniquePtr<gchar> display(g_filename_to_utf8(filename.get(), -1, nullptr, nullptr, nullptr));
>+    GUniquePtr<gchar> display(g_filename_display_name(filename.data()));
>     if (!display)
>         return string;
>-
>     return String::fromUTF8(display.get());
> #endif
> }
> 
> bool fileExists(const String& path)
> {
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    return filename ? g_file_test(filename.get(), G_FILE_TEST_EXISTS) : false;
>+    auto filename = fileSystemRepresentation(path);
>+    return validRepresentation(filename) ? g_file_test(filename.data(), G_FILE_TEST_EXISTS) : false;
> }
> 
> bool deleteFile(const String& path)
> {
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    return filename ? g_remove(filename.get()) != -1 : false;
>+    auto filename = fileSystemRepresentation(path);
>+    return validRepresentation(filename) ? g_remove(filename.data()) != -1 : false;
> }
> 
> bool deleteEmptyDirectory(const String& path)
> {
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    return filename ? g_rmdir(filename.get()) != -1 : false;
>+    auto filename = fileSystemRepresentation(path);
>+    return validRepresentation(filename) ? g_rmdir(filename.data()) != -1 : false;
> }
> 
> static bool getFileStat(const String& path, GStatBuf* statBuffer)
> {
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    if (!filename)
>+    auto filename = fileSystemRepresentation(path);
>+    if (!validRepresentation(filename))
>         return false;
> 
>-    return g_stat(filename.get(), statBuffer) != -1;
>+    return g_stat(filename.data(), statBuffer) != -1;
> }
> 
> static bool getFileLStat(const String& path, GStatBuf* statBuffer)
> {
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    if (!filename)
>+    auto filename = fileSystemRepresentation(path);
>+    if (!validRepresentation(filename))
>         return false;
> 
>-    return g_lstat(filename.get(), statBuffer) != -1;
>+    return g_lstat(filename.data(), statBuffer) != -1;
> }
> 
> bool getFileSize(const String& path, long long& resultSize)
>@@ -225,8 +240,8 @@ String pathByAppendingComponents(StringView path, const Vector<StringView>& comp
> 
> bool makeAllDirectories(const String& path)
> {
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    return filename ? g_mkdir_with_parents(filename.get(), S_IRWXU) != -1 : false;
>+    auto filename = fileSystemRepresentation(path);
>+    return validRepresentation(filename) ? g_mkdir_with_parents(filename.data(), S_IRWXU) != -1 : false;
> }
> 
> String homeDirectoryPath()
>@@ -237,33 +252,33 @@ String homeDirectoryPath()
> bool createSymbolicLink(const String& targetPath, const String& symbolicLinkPath)
> {
>     CString targetPathFSRep = fileSystemRepresentation(targetPath);
>-    if (!targetPathFSRep.data() || targetPathFSRep.data()[0] == '\0')
>+    if (!validRepresentation(targetPathFSRep))
>         return false;
> 
>     CString symbolicLinkPathFSRep = fileSystemRepresentation(symbolicLinkPath);
>-    if (!symbolicLinkPathFSRep.data() || symbolicLinkPathFSRep.data()[0] == '\0')
>+    if (!validRepresentation(symbolicLinkPathFSRep))
>         return false;
> 
>     return !symlink(targetPathFSRep.data(), symbolicLinkPathFSRep.data());
> }
> 
>-String pathGetFileName(const String& pathName)
>+String pathGetFileName(const String& path)
> {
>-    GUniquePtr<gchar> tmpFilename = unescapedFilename(pathName);
>-    if (!tmpFilename)
>-        return pathName;
>+    auto filename = fileSystemRepresentation(path);
>+    if (!validRepresentation(filename))
>+        return path;
> 
>-    GUniquePtr<gchar> baseName(g_path_get_basename(tmpFilename.get()));
>+    GUniquePtr<gchar> baseName(g_path_get_basename(filename.data()));
>     return String::fromUTF8(baseName.get());
> }
> 
> bool getVolumeFreeSpace(const String& path, uint64_t& freeSpace)
> {
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    if (!filename)
>+    auto filename = fileSystemRepresentation(path);
>+    if (!validRepresentation(filename))
>         return false;
> 
>-    GRefPtr<GFile> file = adoptGRef(g_file_new_for_path(filename.get()));
>+    GRefPtr<GFile> file = adoptGRef(g_file_new_for_path(filename.data()));
>     GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_filesystem_info(file.get(), G_FILE_ATTRIBUTE_FILESYSTEM_FREE, 0, 0));
>     if (!fileInfo)
>         return false;
>@@ -274,11 +289,11 @@ bool getVolumeFreeSpace(const String& path, uint64_t& freeSpace)
> 
> String directoryName(const String& path)
> {
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    if (!filename)
>+    auto filename = fileSystemRepresentation(path);
>+    if (!validRepresentation(filename))
>         return String();
> 
>-    GUniquePtr<char> dirname(g_path_get_dirname(filename.get()));
>+    GUniquePtr<char> dirname(g_path_get_dirname(filename.data()));
>     return String::fromUTF8(dirname.get());
> }
> 
>@@ -286,11 +301,11 @@ Vector<String> listDirectory(const String& path, const String& filter)
> {
>     Vector<String> entries;
> 
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    if (!filename)
>+    auto filename = fileSystemRepresentation(path);
>+    if (!validRepresentation(filename))
>         return entries;
> 
>-    GUniquePtr<GDir> dir(g_dir_open(filename.get(), 0, nullptr));
>+    GUniquePtr<GDir> dir(g_dir_open(filename.data(), 0, nullptr));
>     if (!dir)
>         return entries;
> 
>@@ -299,7 +314,7 @@ Vector<String> listDirectory(const String& path, const String& filter)
>         if (!g_pattern_match_string(pspec.get(), name))
>             continue;
> 
>-        GUniquePtr<gchar> entry(g_build_filename(filename.get(), name, nullptr));
>+        GUniquePtr<gchar> entry(g_build_filename(filename.data(), name, nullptr));
>         entries.append(stringFromFileSystemRepresentation(entry.get()));
>     }
> 
>@@ -320,16 +335,16 @@ String openTemporaryFile(const String& prefix, PlatformFileHandle& handle)
> 
> PlatformFileHandle openFile(const String& path, FileOpenMode mode)
> {
>-    GUniquePtr<gchar> filename = unescapedFilename(path);
>-    if (!filename)
>+    auto filename = fileSystemRepresentation(path);
>+    if (!validRepresentation(filename))
>         return invalidPlatformFileHandle;
> 
>-    GRefPtr<GFile> file = adoptGRef(g_file_new_for_path(filename.get()));
>+    GRefPtr<GFile> file = adoptGRef(g_file_new_for_path(filename.data()));
>     GFileIOStream* ioStream = 0;
>     if (mode == FileOpenMode::Read)
>         ioStream = g_file_open_readwrite(file.get(), 0, 0);
>     else if (mode == FileOpenMode::Write) {
>-        if (g_file_test(filename.get(), static_cast<GFileTest>(G_FILE_TEST_EXISTS | G_FILE_TEST_IS_REGULAR)))
>+        if (g_file_test(filename.data(), static_cast<GFileTest>(G_FILE_TEST_EXISTS | G_FILE_TEST_IS_REGULAR)))
>             ioStream = g_file_open_readwrite(file.get(), 0, 0);
>         else
>             ioStream = g_file_create_readwrite(file.get(), G_FILE_CREATE_NONE, 0, 0);
>@@ -395,16 +410,16 @@ int readFromFile(PlatformFileHandle handle, char* data, int length)
> 
> bool moveFile(const String& oldPath, const String& newPath)
> {
>-    GUniquePtr<gchar> oldFilename = unescapedFilename(oldPath);
>-    if (!oldFilename)
>+    auto oldFilename = fileSystemRepresentation(oldPath);
>+    if (!validRepresentation(oldFilename))
>         return false;
> 
>-    GUniquePtr<gchar> newFilename = unescapedFilename(newPath);
>-    if (!newFilename)
>+    auto newFilename = fileSystemRepresentation(newPath);
>+    if (!validRepresentation(newFilename))
>         return false;
> 
>-    GRefPtr<GFile> oldFile = adoptGRef(g_file_new_for_path(oldFilename.get()));
>-    GRefPtr<GFile> newFile = adoptGRef(g_file_new_for_path(newFilename.get()));
>+    GRefPtr<GFile> oldFile = adoptGRef(g_file_new_for_path(oldFilename.data()));
>+    GRefPtr<GFile> newFile = adoptGRef(g_file_new_for_path(newFilename.data()));
> 
>     return g_file_move(oldFile.get(), newFile.get(), G_FILE_COPY_OVERWRITE, nullptr, nullptr, nullptr, nullptr);
> }
>@@ -414,31 +429,27 @@ bool hardLinkOrCopyFile(const String& source, const String& destination)
> #if OS(WINDOWS)
>     return !!::CopyFile(source.charactersWithNullTermination().data(), destination.charactersWithNullTermination().data(), TRUE);
> #else
>-    GUniquePtr<gchar> sourceFilename = unescapedFilename(source);
>-    if (!sourceFilename)
>+    auto sourceFilename = fileSystemRepresentation(source);
>+    if (!validRepresentation(sourceFilename))
>         return false;
> 
>-    GUniquePtr<gchar> destinationFilename = unescapedFilename(destination);
>-    if (!destinationFilename)
>+    auto destinationFilename = fileSystemRepresentation(destination);
>+    if (!validRepresentation(destinationFilename))
>         return false;
> 
>-    if (!link(sourceFilename.get(), destinationFilename.get()))
>+    if (!link(sourceFilename.data(), destinationFilename.data()))
>         return true;
> 
>     // Hard link failed. Perform a copy instead.
>-    GRefPtr<GFile> sourceFile = adoptGRef(g_file_new_for_path(sourceFilename.get()));
>-    GRefPtr<GFile> destinationFile = adoptGRef(g_file_new_for_path(destinationFilename.get()));
>+    GRefPtr<GFile> sourceFile = adoptGRef(g_file_new_for_path(sourceFilename.data()));
>+    GRefPtr<GFile> destinationFile = adoptGRef(g_file_new_for_path(destinationFilename.data()));
>     return g_file_copy(sourceFile.get(), destinationFile.get(), G_FILE_COPY_NONE, nullptr, nullptr, nullptr, nullptr);
> #endif
> }
> 
> Optional<int32_t> getFileDeviceId(const CString& fsFile)
> {
>-    GUniquePtr<gchar> filename = unescapedFilename(fsFile.data());
>-    if (!filename)
>-        return WTF::nullopt;
>-
>-    GRefPtr<GFile> file = adoptGRef(g_file_new_for_path(filename.get()));
>+    GRefPtr<GFile> file = adoptGRef(g_file_new_for_path(fsFile.data()));
>     GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_filesystem_info(file.get(), G_FILE_ATTRIBUTE_UNIX_DEVICE, nullptr, nullptr));
>     if (!fileInfo)
>         return WTF::nullopt;

Actions: View | Formatted Diff | Diff

Attachments on bug 194213: 361041 | 361179