Changeset 221441 in webkit


Ignore:
Timestamp:
Aug 31, 2017 1:53:38 PM (7 years ago)
Author:
Chris Dumez
Message:

getFileMetadata() does not work as expected for symbolic links
https://bugs.webkit.org/show_bug.cgi?id=176143

Reviewed by Andreas Kling.

Source/WebCore:

getFileMetadata() does not work as expected for symbolic links:
On POSIX, getFileMetadata() always followed symlinks, which meant that FileMetadata.type could
never be TypeSymbolicLink. On Windows, the function properly did not follow symlinks but failed to set
FileMetadata.type to TypeSymbolicLink when the file was a symbolic link.

This patch adds a new ShouldFollowSymbolicLinks parameter to getFileMetadata() so that
the call site can decide the behavior it wants. If getFileMetadata() is called on a
symbolic link with ShouldFollowSymbolicLinks::No as parameter, FileMetadata.type is now
properly set to TypeSymbolicLink.

No new tests, covered by new API test.

  • Modules/entriesapi/DOMFileSystem.cpp:

(WebCore::listDirectoryWithMetadata):
It is important we do not follow symlinks here since the code wants to discard them
and does so by checking FileMetadata.type.

  • WebCore.xcodeproj/project.pbxproj:
  • fileapi/File.cpp:

(WebCore::File::isDirectory const):

  • html/FileListCreator.cpp:

(WebCore::appendDirectoryFiles):
(WebCore::FileListCreator::createFileList):
It is important we do not follow symlinks here since the code wants to discard them
and does so by checking FileMetadata.type.

  • platform/FileSystem.cpp:

(WebCore::fileIsDirectory):

  • platform/FileSystem.h:
  • platform/glib/FileSystemGlib.cpp:

(WebCore::getFileLStat):
(WebCore::getFileMetadata):

  • platform/posix/FileSystemPOSIX.cpp:

(WebCore::getFileMetadata):
(WebCore::createSymbolicLink):

  • platform/win/FileSystemWin.cpp:

(WebCore::getFinalPathName):
(WebCore::getFileMetadata):
(WebCore::createSymbolicLink):

  • Add new createSymbolicLink() function for testing purposes.
  • On Posix, call lstat() instead of stat if ShouldFollowSymbolicLinks::No.
  • On Windows, since FindFirstFileW() does not follow symlinks, resolve final path using GetFinalPathNameByHandleW() if ShouldFollowSymbolicLinks::Yes.
  • On Windows, properly set FileMetadata.type to TypeSymbolicLink if the file is a symbolic link.

Tools:

Add API test coverage.

  • TestWebKitAPI/Tests/WebCore/FileSystem.cpp:

(TestWebKitAPI::TEST_F):

Location:
trunk
Files:
14 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r221438 r221441  
     12017-08-31  Chris Dumez  <cdumez@apple.com>
     2
     3        getFileMetadata() does not work as expected for symbolic links
     4        https://bugs.webkit.org/show_bug.cgi?id=176143
     5
     6        Reviewed by Andreas Kling.
     7
     8        getFileMetadata() does not work as expected for symbolic links:
     9        On POSIX, getFileMetadata() always followed symlinks, which meant that FileMetadata.type could
     10        never be TypeSymbolicLink. On Windows, the function properly did not follow symlinks but failed to set
     11        FileMetadata.type to TypeSymbolicLink when the file was a symbolic link.
     12
     13        This patch adds a new ShouldFollowSymbolicLinks parameter to getFileMetadata() so that
     14        the call site can decide the behavior it wants. If getFileMetadata() is called on a
     15        symbolic link with ShouldFollowSymbolicLinks::No as parameter, FileMetadata.type is now
     16        properly set to TypeSymbolicLink.
     17
     18        No new tests, covered by new API test.
     19
     20        * Modules/entriesapi/DOMFileSystem.cpp:
     21        (WebCore::listDirectoryWithMetadata):
     22        It is important we do not follow symlinks here since the code wants to discard them
     23        and does so by checking FileMetadata.type.
     24
     25        * WebCore.xcodeproj/project.pbxproj:
     26        * fileapi/File.cpp:
     27        (WebCore::File::isDirectory const):
     28
     29        * html/FileListCreator.cpp:
     30        (WebCore::appendDirectoryFiles):
     31        (WebCore::FileListCreator::createFileList):
     32        It is important we do not follow symlinks here since the code wants to discard them
     33        and does so by checking FileMetadata.type.
     34
     35        * platform/FileSystem.cpp:
     36        (WebCore::fileIsDirectory):
     37        * platform/FileSystem.h:
     38        * platform/glib/FileSystemGlib.cpp:
     39        (WebCore::getFileLStat):
     40        (WebCore::getFileMetadata):
     41        * platform/posix/FileSystemPOSIX.cpp:
     42        (WebCore::getFileMetadata):
     43        (WebCore::createSymbolicLink):
     44        * platform/win/FileSystemWin.cpp:
     45        (WebCore::getFinalPathName):
     46        (WebCore::getFileMetadata):
     47        (WebCore::createSymbolicLink):
     48        - Add new createSymbolicLink() function for testing purposes.
     49        - On Posix, call lstat() instead of stat if ShouldFollowSymbolicLinks::No.
     50        - On Windows, since FindFirstFileW() does not follow symlinks, resolve
     51          final path using GetFinalPathNameByHandleW() if ShouldFollowSymbolicLinks::Yes.
     52        - On Windows, properly set FileMetadata.type to TypeSymbolicLink if the file
     53          is a symbolic link.
     54
    1552017-08-31  Michael Catanzaro  <mcatanzaro@igalia.com>
    256
  • trunk/Source/WebCore/Modules/entriesapi/DOMFileSystem.cpp

    r221414 r221441  
    4747{
    4848    ASSERT(!isMainThread());
    49     if (!fileIsDirectory(fullPath))
     49    if (!fileIsDirectory(fullPath, ShouldFollowSymbolicLinks::No))
    5050        return Exception { NotFoundError, "Path no longer exists or is no longer a directory" };
    5151
     
    5555    for (auto& childPath : childPaths) {
    5656        FileMetadata metadata;
    57         if (!getFileMetadata(childPath, metadata))
     57        if (!getFileMetadata(childPath, metadata, ShouldFollowSymbolicLinks::No))
    5858            continue;
    5959        listedChildren.uncheckedAppend(ListedChild { pathGetFileName(childPath), metadata.type });
  • trunk/Source/WebCore/WebCore.xcodeproj/project.pbxproj

    r221437 r221441  
    19401940                468344DF1EDDFAAA00B7795B /* DOMRectList.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 468344DD1EDDFA5F00B7795B /* DOMRectList.cpp */; };
    19411941                468344E01EDDFAAA00B7795B /* DOMRectList.h in Headers */ = {isa = PBXBuildFile; fileRef = 468344DE1EDDFA5F00B7795B /* DOMRectList.h */; settings = {ATTRIBUTES = (Private, ); }; };
    1942                 4689F1AF1267BAE100E8D380 /* FileMetadata.h in Headers */ = {isa = PBXBuildFile; fileRef = 4689F1AE1267BAE100E8D380 /* FileMetadata.h */; };
     1942                4689F1AF1267BAE100E8D380 /* FileMetadata.h in Headers */ = {isa = PBXBuildFile; fileRef = 4689F1AE1267BAE100E8D380 /* FileMetadata.h */; settings = {ATTRIBUTES = (Private, ); }; };
    19431943                46B63F6C1C6E8D19002E914B /* JSEventTargetCustom.h in Headers */ = {isa = PBXBuildFile; fileRef = 46B63F6B1C6E8CDF002E914B /* JSEventTargetCustom.h */; settings = {ATTRIBUTES = (Private, ); }; };
    19441944                46C696CB1E7205F700597937 /* CPUMonitor.h in Headers */ = {isa = PBXBuildFile; fileRef = 46C696C91E7205E400597937 /* CPUMonitor.h */; settings = {ATTRIBUTES = (Private, ); }; };
  • trunk/Source/WebCore/fileapi/File.cpp

    r221302 r221441  
    131131{
    132132    if (!m_isDirectory)
    133         m_isDirectory = fileIsDirectory(m_path);
     133        m_isDirectory = fileIsDirectory(m_path, ShouldFollowSymbolicLinks::Yes);
    134134    return *m_isDirectory;
    135135}
  • trunk/Source/WebCore/html/FileListCreator.cpp

    r221414 r221441  
    4444    for (auto& childPath : listDirectory(directory, "*")) {
    4545        FileMetadata metadata;
    46         if (!getFileMetadata(childPath, metadata))
     46        if (!getFileMetadata(childPath, metadata, ShouldFollowSymbolicLinks::No))
    4747            continue;
    4848
     
    8181    Vector<RefPtr<File>> fileObjects;
    8282    for (auto& info : paths) {
    83         if (shouldResolveDirectories == ShouldResolveDirectories::Yes && fileIsDirectory(info.path))
     83        if (shouldResolveDirectories == ShouldResolveDirectories::Yes && fileIsDirectory(info.path, ShouldFollowSymbolicLinks::No))
    8484            appendDirectoryFiles(info.path, pathGetFileName(info.path), fileObjects);
    8585        else
  • trunk/Source/WebCore/platform/FileSystem.cpp

    r221291 r221441  
    352352}
    353353
    354 bool fileIsDirectory(const String& path)
     354bool fileIsDirectory(const String& path, ShouldFollowSymbolicLinks shouldFollowSymbolicLinks)
    355355{
    356356    FileMetadata metadata;
    357     if (!getFileMetadata(path, metadata))
     357    if (!getFileMetadata(path, metadata, shouldFollowSymbolicLinks))
    358358        return false;
    359359    return metadata.type == FileMetadata::TypeDirectory;
  • trunk/Source/WebCore/platform/FileSystem.h

    r221414 r221441  
    8989};
    9090
     91enum class ShouldFollowSymbolicLinks { No, Yes };
     92
    9193struct FileMetadata;
    9294
     
    99101WEBCORE_EXPORT bool getFileModificationTime(const String&, time_t& result);
    100102WEBCORE_EXPORT bool getFileCreationTime(const String&, time_t& result); // Not all platforms store file creation time.
    101 bool getFileMetadata(const String&, FileMetadata&);
    102 bool fileIsDirectory(const String&);
     103WEBCORE_EXPORT bool getFileMetadata(const String&, FileMetadata&, ShouldFollowSymbolicLinks);
     104bool fileIsDirectory(const String&, ShouldFollowSymbolicLinks);
    103105WEBCORE_EXPORT String pathByAppendingComponent(const String& path, const String& component);
    104106String pathByAppendingComponents(const String& path, const Vector<String>& components);
     
    110112WEBCORE_EXPORT bool getVolumeFreeSpace(const String&, uint64_t&);
    111113WEBCORE_EXPORT std::optional<int32_t> getFileDeviceId(const CString&);
     114WEBCORE_EXPORT bool createSymbolicLink(const String& targetPath, const String& symbolicLinkPath);
    112115
    113116WEBCORE_EXPORT void setMetadataURL(const String& path, const String& urlString, const String& referrer = { });
  • trunk/Source/WebCore/platform/glib/FileSystemGlib.cpp

    r221414 r221441  
    124124}
    125125
     126static bool getFileLStat(const String& path, GStatBuf* statBuffer)
     127{
     128    GUniquePtr<gchar> filename = unescapedFilename(path);
     129    if (!filename)
     130        return false;
     131
     132    return g_lstat(filename.get(), statBuffer) != -1;
     133}
     134
    126135bool getFileSize(const String& path, long long& resultSize)
    127136{
     
    156165}
    157166
    158 bool getFileMetadata(const String& path, FileMetadata& metadata)
     167bool getFileMetadata(const String& path, FileMetadata& metadata, ShouldFollowSymbolicLinks shouldFollowSymbolicLinks)
    159168{
    160169    GStatBuf statResult;
    161     if (!getFileStat(path, &statResult))
    162         return false;
     170
     171    if (shouldFollowSymbolicLinks == ShouldFollowSymbolicLinks::Yes) {
     172        if (!getFileStat(path, &statResult))
     173            return false;
     174    } else {
     175        if (!getFileLStat(path, &statResult))
     176            return false;
     177    }
    163178
    164179    String filename = pathGetFileName(path);
     
    202217{
    203218    return stringFromFileSystemRepresentation(g_get_home_dir());
     219}
     220
     221bool createSymbolicLink(const String& targetPath, const String& symbolicLinkPath)
     222{
     223    CString targetPathFSRep = fileSystemRepresentation(targetPath);
     224    if (!targetPathFSRep.data() || targetPathFSRep.data()[0] == '\0')
     225        return false;
     226
     227    CString symbolicLinkPathFSRep = fileSystemRepresentation(symbolicLinkPath);
     228    if (!symbolicLinkPathFSRep.data() || symbolicLinkPathFSRep.data()[0] == '\0')
     229        return false;
     230
     231    return !symlink(targetPathFSRep.data(), symbolicLinkPathFSRep.data());
    204232}
    205233
  • trunk/Source/WebCore/platform/network/BlobDataFileReference.cpp

    r202414 r221441  
    9797    // FIXME: Some platforms provide better ways to listen for file system object changes, consider using these.
    9898    FileMetadata metadata;
    99     if (!getFileMetadata(m_path, metadata))
     99    if (!getFileMetadata(m_path, metadata, ShouldFollowSymbolicLinks::Yes))
    100100        return;
    101101
  • trunk/Source/WebCore/platform/network/mac/BlobDataFileReferenceMac.mm

    r219191 r221441  
    8181    if (!m_replacementPath.isNull()) {
    8282        FileMetadata metadata;
    83         if (getFileMetadata(m_replacementPath, metadata))
     83        if (getFileMetadata(m_replacementPath, metadata, ShouldFollowSymbolicLinks::Yes))
    8484            m_size = metadata.length;
    8585    }
  • trunk/Source/WebCore/platform/posix/FileSystemPOSIX.cpp

    r221414 r221441  
    240240}
    241241
    242 bool getFileMetadata(const String& path, FileMetadata& metadata)
    243 {
    244     CString fsRep = fileSystemRepresentation(path);
    245 
    246     if (!fsRep.data() || fsRep.data()[0] == '\0')
    247         return false;
    248 
    249     struct stat fileInfo;
    250     if (stat(fsRep.data(), &fileInfo))
    251         return false;
     242bool getFileMetadata(const String& path, FileMetadata& metadata, ShouldFollowSymbolicLinks shouldFollowSymbolicLinks)
     243{
     244    CString fsRep = fileSystemRepresentation(path);
     245
     246    if (!fsRep.data() || fsRep.data()[0] == '\0')
     247        return false;
     248
     249    struct stat fileInfo;
     250
     251    if (shouldFollowSymbolicLinks == ShouldFollowSymbolicLinks::Yes) {
     252        if (stat(fsRep.data(), &fileInfo))
     253            return false;
     254    } else {
     255        if (lstat(fsRep.data(), &fileInfo))
     256            return false;
     257    }
    252258
    253259    String filename = pathGetFileName(path);
     
    263269        metadata.type = FileMetadata::TypeFile;
    264270    return true;
     271}
     272
     273bool createSymbolicLink(const String& targetPath, const String& symbolicLinkPath)
     274{
     275    CString targetPathFSRep = fileSystemRepresentation(targetPath);
     276    if (!targetPathFSRep.data() || targetPathFSRep.data()[0] == '\0')
     277        return false;
     278
     279    CString symbolicLinkPathFSRep = fileSystemRepresentation(symbolicLinkPath);
     280    if (!symbolicLinkPathFSRep.data() || symbolicLinkPathFSRep.data()[0] == '\0')
     281        return false;
     282
     283    return !symlink(targetPathFSRep.data(), symbolicLinkPathFSRep.data());
    265284}
    266285
  • trunk/Source/WebCore/platform/win/FileSystemWin.cpp

    r221422 r221441  
    142142}
    143143
    144 bool getFileMetadata(const String& path, FileMetadata& metadata)
     144static String getFinalPathName(const String& path)
     145{
     146    auto handle = openFile(path, OpenForRead);
     147    if (!isHandleValid(handle))
     148        return String();
     149
     150    StringVector<UChar> buffer(MAX_PATH);
     151    if (::GetFinalPathNameByHandleW(handle, buffer.data(), buffer.size(), VOLUME_NAME_NT) >= MAX_PATH) {
     152        closeFile(handle);
     153        return String();
     154    }
     155    closeFile(handle);
     156
     157    buffer.shrink(wcslen(buffer.data()));
     158    return String::adopt(WTFMove(buffer));
     159}
     160
     161bool getFileMetadata(const String& path, FileMetadata& metadata, ShouldFollowSymbolicLinks shouldFollowSymbolicLinks)
    145162{
    146163    WIN32_FIND_DATAW findData;
    147164    if (!getFindData(path, findData))
    148165        return false;
     166
     167    bool isSymbolicLink = findData.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT && findData.dwReserved0 == IO_REPARSE_TAG_SYMLINK;
     168    if (isSymbolicLink && shouldFollowSymbolicLinks == ShouldFollowSymbolicLinks::Yes) {
     169        String targetPath = getFinalPathName(path);
     170        if (targetPath.isNull())
     171            return false;
     172        if (!getFindData(targetPath, findData))
     173            return false;
     174        isSymbolicLink = false;
     175    }
    149176
    150177    if (!getFileSizeFromFindData(findData, metadata.length))
     
    155182    metadata.modificationTime = modificationTime;
    156183    metadata.isHidden = findData.dwFileAttributes & FILE_ATTRIBUTE_HIDDEN;
    157     metadata.type = (findData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) ? FileMetadata::TypeDirectory : FileMetadata::TypeFile;
    158 
    159     return true;
     184    if (findData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
     185        metadata.type = FileMetadata::TypeDirectory;
     186    else if (isSymbolicLink)
     187        metadata.type = FileMetadata::TypeSymbolicLink;
     188    else
     189        metadata.type = FileMetadata::TypeFile;
     190
     191    return true;
     192}
     193
     194bool createSymbolicLink(const String& targetPath, const String& symbolicLinkPath)
     195{
     196    return !::CreateSymbolicLinkW(symbolicLinkPath.charactersWithNullTermination().data(), targetPath.charactersWithNullTermination().data(), 0);
    160197}
    161198
  • trunk/Tools/ChangeLog

    r221436 r221441  
     12017-08-31  Chris Dumez  <cdumez@apple.com>
     2
     3        getFileMetadata() does not work as expected for symbolic links
     4        https://bugs.webkit.org/show_bug.cgi?id=176143
     5
     6        Reviewed by Andreas Kling.
     7
     8        Add API test coverage.
     9
     10        * TestWebKitAPI/Tests/WebCore/FileSystem.cpp:
     11        (TestWebKitAPI::TEST_F):
     12
    1132017-08-31  Filip Pizlo  <fpizlo@apple.com>
    214
  • trunk/Tools/TestWebKitAPI/Tests/WebCore/FileSystem.cpp

    r211763 r221441  
    2828
    2929#include "Test.h"
     30#include <WebCore/FileMetadata.h>
    3031#include <WebCore/FileSystem.h>
    3132#include <wtf/MainThread.h>
     
    4950        m_tempFilePath = openTemporaryFile("tempTestFile", handle);
    5051        writeToFile(handle, FileSystemTestData, strlen(FileSystemTestData));
    51         closeFile(handle);
     52        closeFile(handle);
     53
     54        m_tempFileSymlinkPath = m_tempFilePath + "-symlink";
     55        createSymbolicLink(m_tempFilePath, m_tempFileSymlinkPath);
    5256
    5357        m_tempEmptyFilePath = openTemporaryFile("tempEmptyTestFile", handle);
     
    6771    {
    6872        deleteFile(m_tempFilePath);
     73        deleteFile(m_tempFileSymlinkPath);
    6974        deleteFile(m_tempEmptyFilePath);
    7075        deleteFile(m_spaceContainingFilePath);
     
    7479
    7580    const String& tempFilePath() { return m_tempFilePath; }
     81    const String& tempFileSymlinkPath() { return m_tempFileSymlinkPath; }
    7682    const String& tempEmptyFilePath() { return m_tempEmptyFilePath; }
    7783    const String& spaceContainingFilePath() { return m_spaceContainingFilePath; }
     
    8187private:
    8288    String m_tempFilePath;
     89    String m_tempFileSymlinkPath;
    8390    String m_tempEmptyFilePath;
    8491    String m_spaceContainingFilePath;
     
    120127}
    121128
     129TEST_F(FileSystemTest, GetFileMetadataSymlink)
     130{
     131    FileMetadata symlinkMetadata;
     132    EXPECT_TRUE(getFileMetadata(tempFileSymlinkPath(), symlinkMetadata, ShouldFollowSymbolicLinks::No));
     133    EXPECT_TRUE(symlinkMetadata.type == FileMetadata::TypeSymbolicLink);
     134    EXPECT_FALSE(static_cast<size_t>(symlinkMetadata.length) == strlen(FileSystemTestData));
     135
     136    FileMetadata targetMetadata;
     137    EXPECT_TRUE(getFileMetadata(tempFileSymlinkPath(), targetMetadata, ShouldFollowSymbolicLinks::Yes));
     138    EXPECT_TRUE(targetMetadata.type == FileMetadata::TypeFile);
     139    EXPECT_EQ(strlen(FileSystemTestData), static_cast<size_t>(targetMetadata.length));
    122140}
     141
     142}
Note: See TracChangeset for help on using the changeset viewer.