Attachment 369156 Details for Bug 197389 – Patch for landing

[patch] Patch for landing

bug-197389-20190506123516.patch (text/plain), 5.43 KB, created by Brent Fulgham on 2019-05-06 12:35:16 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Brent Fulgham

Created: 2019-05-06 12:35:16 PDT

Size: 5.43 KB

patch

obsolete

>Subversion Revision: 244964
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 335df3376f0455f40b0684592edbe66c446a008d..44b5b825eac7f2a15d1e1f9780d29d188c6f9011 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,26 @@
>+2019-05-06  Brent Fulgham  <bfulgham@apple.com>
>+
>+        Use more efficient path resolution logic
>+        https://bugs.webkit.org/show_bug.cgi?id=197389
>+        <rdar://problem/50268491>
>+
>+        Reviewed by Maciej Stachowiak.
>+
>+        The code in SandboxExtensionsCocoa.mm 'resolveSymlinksInPath' is pretty inefficient, and tries to reproduce (badly)
>+        logic that is already provided by the operating system.
>+
>+        To make matters worse, 'resolvePathForSandboxExtension' was effectively performing the work of fully resolving
>+        symlinks twice, since NSString's 'stringByStandardizingPath' method does some of this already.
>+
>+        Instead, we should just use NSString's 'stringByResolvingSymlinksInPath', which does the symlink resolution
>+        using more efficient logic than our 'resolveSymlinksInPath' code.
>+
>+        * Shared/Cocoa/SandboxExtensionCocoa.mm:
>+        (WebKit::resolveSymlinksInPath): Removed.
>+        (WebKit::resolvePathForSandboxExtension): Remove redundant call to 'resolveSymlinksInPath', and switches from
>+        'stringByStandardizingPath' to 'stringByResolvingSymlinksInPath', which can take the place of both calls.
>+        (WebKit::stringByResolvingSymlinksInPath): Switch to call 'stringByResolvingSymlinksInPath'.
>+
> 2019-05-06  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r244917.
>diff --git a/Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm b/Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm
>index c2ec473e9b0d28bb731c71136d8bf416ee4485db..7b6d52be7f15dc7e9a04abf23c9b068b11ef1432 100644
>--- a/Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm
>+++ b/Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2010-2016 Apple Inc. All rights reserved.
>+ * Copyright (C) 2010-2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -31,7 +31,6 @@
> #import "DataReference.h"
> #import "Decoder.h"
> #import "Encoder.h"
>-#import <sys/stat.h>
> #import <wtf/FileSystem.h>
> #import <wtf/spi/darwin/SandboxSPI.h>
> #import <wtf/text/CString.h>
>@@ -225,52 +224,9 @@ RefPtr<SandboxExtension> SandboxExtension::create(Handle&& handle)
>     return adoptRef(new SandboxExtension(handle));
> }
> 
>-static CString resolveSymlinksInPath(const CString& path)
>-{
>-    struct stat statBuf;
>-
>-    // Check if this file exists.
>-    if (!stat(path.data(), &statBuf)) {
>-        char resolvedName[PATH_MAX];
>-
>-        return realpath(path.data(), resolvedName);
>-    }
>-
>-    const char* slashPtr = strrchr(path.data(), '/');
>-    if (slashPtr == path.data())
>-        return path;
>-
>-    size_t parentDirectoryLength = slashPtr - path.data();
>-    if (parentDirectoryLength >= PATH_MAX)
>-        return CString();
>-
>-    // Get the parent directory.
>-    char parentDirectory[PATH_MAX];
>-    memcpy(parentDirectory, path.data(), parentDirectoryLength);
>-    parentDirectory[parentDirectoryLength] = '\0';
>-
>-    // Resolve it.
>-    CString resolvedParentDirectory = resolveSymlinksInPath(CString(parentDirectory));
>-    if (resolvedParentDirectory.isNull())
>-        return CString();
>-
>-    size_t lastPathComponentLength = path.length() - parentDirectoryLength;
>-    size_t resolvedPathLength = resolvedParentDirectory.length() + lastPathComponentLength;
>-    if (resolvedPathLength >= PATH_MAX)
>-        return CString();
>-
>-    // Combine the resolved parent directory with the last path component.
>-    char* resolvedPathBuffer;
>-    CString resolvedPath = CString::newUninitialized(resolvedPathLength, resolvedPathBuffer);
>-    memcpy(resolvedPathBuffer, resolvedParentDirectory.data(), resolvedParentDirectory.length());
>-    memcpy(resolvedPathBuffer + resolvedParentDirectory.length(), slashPtr, lastPathComponentLength);
>-
>-    return resolvedPath;
>-}
>-
> String stringByResolvingSymlinksInPath(const String& path)
> {
>-    return String::fromUTF8(resolveSymlinksInPath(path.utf8()));
>+    return [(NSString *)path stringByResolvingSymlinksInPath];
> }
> 
> String resolveAndCreateReadWriteDirectoryForSandboxExtension(const String& path)
>@@ -288,15 +244,13 @@ String resolveAndCreateReadWriteDirectoryForSandboxExtension(const String& path)
> 
> String resolvePathForSandboxExtension(const String& path)
> {
>-    // FIXME: Do we need both resolveSymlinksInPath() and -stringByStandardizingPath?
>-    CString fileSystemPath = FileSystem::fileSystemRepresentation([(NSString *)path stringByStandardizingPath]);
>-    if (fileSystemPath.isNull()) {
>-        LOG_ERROR("Could not create a valid file system representation for the string '%s' of length %lu", fileSystemPath.data(), fileSystemPath.length());
>+    String resolvedPath = stringByResolvingSymlinksInPath(path);
>+    if (resolvedPath.isNull()) {
>+        LOG_ERROR("Could not create a valid file system representation for the string '%s' of length %lu", resolvedPath.utf8().data(), resolvedPath.length());
>         return { };
>     }
> 
>-    CString standardizedPath = resolveSymlinksInPath(fileSystemPath);
>-    return String::fromUTF8(standardizedPath);
>+    return resolvedPath;
> }
> 
> bool SandboxExtension::createHandleWithoutResolvingPath(const String& path, Type type, Handle& handle)

Actions: View | Formatted Diff | Diff

Attachments on bug 197389: 368496 | 368512 | 368519 | 368624 | 368806 | 368872 | 368961 | 369156