Attachment 362659 Details for Bug 194612 – Patch

[patch] Patch

bug-194612-20190221160122.patch (text/plain), 18.01 KB, created by Ross Kirsling on 2019-02-21 16:01:23 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Ross Kirsling

Created: 2019-02-21 16:01:23 PST

Size: 18.01 KB

patch

obsolete

>Subversion Revision: 241916
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index cba59768c20198cf950ebfc9ecfe2abdd76c7c86..da3bf65b1619b7748aaecec83cb252e749c843a4 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,31 @@
>+2019-02-21  Ross Kirsling  <ross.kirsling@sony.com>
>+
>+        EnvironmentUtilities::stripValuesEndingWithString isn't thread-safe
>+        https://bugs.webkit.org/show_bug.cgi?id=194612
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This API test really shouldn't be verifying that the actual environment was successfully modified.
>+
>+        At its core, stripValuesEndingWithString is really just split-filter-join. By replacing it with a pair of
>+        simple functions -- one for string processing, one for environment processing -- the API test only needs to
>+        worry about the former.
>+
>+        * Platform/unix/EnvironmentUtilities.cpp:
>+        (WebKit::EnvironmentUtilities::stripEntriesEndingWith):
>+        (WebKit::EnvironmentUtilities::removeValuesEndingWith):
>+        (WebKit::EnvironmentUtilities::stripValuesEndingWithString): Deleted.
>+        * Platform/unix/EnvironmentUtilities.h:
>+        Replace old function with a pair of simpler ones.
>+
>+        * NetworkProcess/EntryPoint/Cocoa/XPCService/NetworkServiceEntryPoint.mm:
>+        (NetworkServiceInitializer):
>+        * PluginProcess/EntryPoint/Cocoa/XPCService/PluginServiceEntryPoint.mm:
>+        (PluginServiceInitializer):
>+        * WebProcess/EntryPoint/Cocoa/XPCService/WebContentServiceEntryPoint.mm:
>+        (WebContentServiceInitializer):
>+        Update function name.
>+
> 2019-02-21  Alex Christensen  <achristensen@webkit.org>
> 
>         API::HTTPCookieStore should expose setCookies()
>diff --git a/Source/WebKit/NetworkProcess/EntryPoint/Cocoa/XPCService/NetworkServiceEntryPoint.mm b/Source/WebKit/NetworkProcess/EntryPoint/Cocoa/XPCService/NetworkServiceEntryPoint.mm
>index b7f5392c0c012ddf03bf5339186551262badbe0b..4d64ced5174094dbb43bd7a4a5dbcd557a6fbc5f 100644
>--- a/Source/WebKit/NetworkProcess/EntryPoint/Cocoa/XPCService/NetworkServiceEntryPoint.mm
>+++ b/Source/WebKit/NetworkProcess/EntryPoint/Cocoa/XPCService/NetworkServiceEntryPoint.mm
>@@ -56,6 +56,6 @@ void NetworkServiceInitializer(xpc_connection_t connection, xpc_object_t initial
> {
>     // Remove the SecItemShim from the DYLD_INSERT_LIBRARIES environment variable so any processes spawned by
>     // the this process don't try to insert the shim and crash.
>-    EnvironmentUtilities::stripValuesEndingWithString("DYLD_INSERT_LIBRARIES", "/SecItemShim.dylib");
>+    EnvironmentUtilities::removeValuesEndingWith("DYLD_INSERT_LIBRARIES", "/SecItemShim.dylib");
>     XPCServiceInitializer<NetworkProcess, NetworkServiceInitializerDelegate>(adoptOSObject(connection), initializerMessage, priorityBoostMessage);
> }
>diff --git a/Source/WebKit/Platform/unix/EnvironmentUtilities.cpp b/Source/WebKit/Platform/unix/EnvironmentUtilities.cpp
>index bb80120fc22c4b29cfe446f8f4fefb46b6cfca3f..9a2c6aab417dcaa2dc7ff0f78a7d153a7e72c25f 100644
>--- a/Source/WebKit/Platform/unix/EnvironmentUtilities.cpp
>+++ b/Source/WebKit/Platform/unix/EnvironmentUtilities.cpp
>@@ -1,5 +1,6 @@
> /*
>  * Copyright (C) 2011 Apple Inc. All rights reserved.
>+ * Copyright (C) 2019 Sony Interactive Entertainment Inc.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -26,104 +27,46 @@
> #include "config.h"
> #include "EnvironmentUtilities.h"
> 
>-#include <wtf/text/CString.h>
>+#include <cstdlib>
>+#include <wtf/text/StringBuilder.h>
> 
> namespace WebKit {
> 
> namespace EnvironmentUtilities {
> 
>-void stripValuesEndingWithString(const char* environmentVariable, const char* searchValue)
>+String stripEntriesEndingWith(StringView input, StringView suffix)
> {
>-    ASSERT(environmentVariable);
>-    ASSERT(searchValue);
>-    
>-    // Grab the current value of the environment variable.
>-    char* environmentValue = getenv(environmentVariable);
>+    StringBuilder output;
> 
>-    if (!environmentValue || environmentValue[0] == '\0')
>-        return;
>-
>-    const size_t environmentValueLength = strlen(environmentValue);
>-    const size_t environmentValueBufferLength = environmentValueLength + 1;
>-
>-    // Set up the strings we'll be searching for.
>-    size_t searchLength = strlen(searchValue);
>-    if (!searchLength)
>-        return;
>-
>-    Vector<char> searchValueWithColonVector;
>-    searchValueWithColonVector.grow(searchLength + 2);
>-    char* searchValueWithColon = searchValueWithColonVector.data();
>-    size_t searchLengthWithColon = searchLength + 1;
>-
>-    memcpy(searchValueWithColon, searchValue, searchLength);
>-    searchValueWithColon[searchLength] = ':';
>-    searchValueWithColon[searchLengthWithColon] = '\0';
>-    
>-    // Loop over environmentValueBuffer, removing any components that match the search value ending with a colon.
>-    char* componentStart = environmentValue;
>-    char* match = strnstr(componentStart, searchValueWithColon, environmentValueLength - static_cast<size_t>(componentStart - environmentValue));
>-    bool foundAnyMatches = match != NULL;
>-    while (match != NULL) {
>-        // Update componentStart to point to the colon immediately preceding the match.
>-        char* nextColon = strnstr(componentStart, ":", environmentValueLength - static_cast<size_t>(componentStart - environmentValue));
>-        while (nextColon && nextColon < match) {
>-            componentStart = nextColon;
>-            nextColon = strnstr(componentStart + 1, ":", environmentValueLength - static_cast<size_t>(componentStart + 1 - environmentValue));
>-        }
>+    auto hasAppended = false;
>+    for (auto entry : input.splitAllowingEmptyEntries(':')) {
>+        if (entry.endsWith(suffix))
>+            continue;
> 
>-        RELEASE_ASSERT(componentStart >= environmentValue);
>-        size_t environmentValueOffset = static_cast<size_t>(componentStart - environmentValue);
>-        RELEASE_ASSERT(environmentValueOffset < environmentValueBufferLength);
>+        if (hasAppended)
>+            output.append(':');
>+        else
>+            hasAppended = true;
> 
>-        // Copy over everything right of the match to the current component start, and search from there again.
>-        if (componentStart[0] == ':') {
>-            // If componentStart points to a colon, copy the colon over.
>-            strlcpy(componentStart, match + searchLength, environmentValueBufferLength - environmentValueOffset);
>-        } else {
>-            // Otherwise, componentStart still points to the beginning of environmentValueBuffer, so don't copy over the colon.
>-            // The edge case is if the colon is the last character in the string, so "match + searchLengthWithoutColon + 1" is the
>-            // null terminator of the original input, in which case this is still safe.
>-            strlcpy(componentStart, match + searchLengthWithColon, environmentValueBufferLength - environmentValueOffset);
>-        }
>-        
>-        match = strnstr(componentStart, searchValueWithColon, environmentValueLength - static_cast<size_t>(componentStart - environmentValue));
>+        output.append(entry);
>     }
>-    
>-    // Search for the value without a trailing colon, seeing if the original input ends with it.
>-    match = strnstr(componentStart, searchValue, environmentValueLength - static_cast<size_t>(componentStart - environmentValue));
>-    while (match != NULL) {
>-        if (match[searchLength] == '\0')
>-            break;
>-        match = strnstr(match + 1, searchValue, environmentValueLength - static_cast<size_t>(match + 1 - environmentValue));
>-    }
>-    
>-    // Since the original input ends with the search, strip out the last component.
>-    if (match) {
>-        // Update componentStart to point to the colon immediately preceding the match.
>-        char* nextColon = strnstr(componentStart, ":", environmentValueLength - static_cast<size_t>(componentStart - environmentValue));
>-        while (nextColon && nextColon < match) {
>-            componentStart = nextColon;
>-            nextColon = strnstr(componentStart + 1, ":", environmentValueLength - static_cast<size_t>(componentStart + 1 - environmentValue));
>-        }
>-        
>-        // Whether componentStart points to the original string or the last colon, putting the null terminator there will get us the desired result.
>-        componentStart[0] = '\0';
> 
>-        foundAnyMatches = true;
>-    }
>+    return output.toString();
>+}
> 
>-    // If we found no matches, don't change anything.
>-    if (!foundAnyMatches)
>+void removeValuesEndingWith(const char* environmentVariable, const char* searchValue)
>+{
>+    const char* before = getenv(environmentVariable);
>+    if (!before)
>         return;
> 
>-    // If we have nothing left, just unset the variable
>-    if (environmentValue[0] == '\0') {
>+    auto after = stripEntriesEndingWith(before, searchValue);
>+    if (after.isEmpty()) {
>         unsetenv(environmentVariable);
>         return;
>     }
>-    
>-    setenv(environmentVariable, environmentValue, 1);
>+
>+    setenv(environmentVariable, after.utf8().data(), 1);
> }
> 
> } // namespace EnvironmentUtilities
>diff --git a/Source/WebKit/Platform/unix/EnvironmentUtilities.h b/Source/WebKit/Platform/unix/EnvironmentUtilities.h
>index 20c31b949916ebd16373a307bdf82196227c2469..e828d74ea53b971d31353671817210a9b4b2508c 100644
>--- a/Source/WebKit/Platform/unix/EnvironmentUtilities.h
>+++ b/Source/WebKit/Platform/unix/EnvironmentUtilities.h
>@@ -1,5 +1,6 @@
> /*
>  * Copyright (C) 2011 Apple Inc. All rights reserved.
>+ * Copyright (C) 2019 Sony Interactive Entertainment Inc.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -26,13 +27,15 @@
> #pragma once
> 
> #include "WKDeclarationSpecifiers.h"
>+#include <wtf/text/StringView.h>
> #include <wtf/text/WTFString.h>
> 
> namespace WebKit {
> 
> namespace EnvironmentUtilities {
> 
>-WK_EXPORT void stripValuesEndingWithString(const char* environmentVariable, const char* search);
>+WK_EXPORT String stripEntriesEndingWith(StringView input, StringView suffix);
>+WK_EXPORT void removeValuesEndingWith(const char* environmentVariable, const char* search);
> 
> } // namespace EnvironmentUtilities
> 
>diff --git a/Source/WebKit/PluginProcess/EntryPoint/Cocoa/XPCService/PluginServiceEntryPoint.mm b/Source/WebKit/PluginProcess/EntryPoint/Cocoa/XPCService/PluginServiceEntryPoint.mm
>index a1fe527de82fe2dde337dea3a4ef844e4a41f35b..e0de2d6280782ffcc825329db36a3044b4fb4177 100644
>--- a/Source/WebKit/PluginProcess/EntryPoint/Cocoa/XPCService/PluginServiceEntryPoint.mm
>+++ b/Source/WebKit/PluginProcess/EntryPoint/Cocoa/XPCService/PluginServiceEntryPoint.mm
>@@ -78,7 +78,7 @@ void PluginServiceInitializer(xpc_connection_t connection, xpc_object_t initiali
> 
>     // Remove the PluginProcess shim from the DYLD_INSERT_LIBRARIES environment variable so any processes
>     // spawned by the PluginProcess don't try to insert the shim and crash.
>-    EnvironmentUtilities::stripValuesEndingWithString("DYLD_INSERT_LIBRARIES", "/PluginProcessShim.dylib");
>+    EnvironmentUtilities::removeValuesEndingWith("DYLD_INSERT_LIBRARIES", "/PluginProcessShim.dylib");
>     XPCServiceInitializer<PluginProcess, PluginServiceInitializerDelegate>(adoptOSObject(connection), initializerMessage, priorityBoostMessage);
> #endif // ENABLE(NETSCAPE_PLUGIN_API)
> }
>diff --git a/Source/WebKit/WebProcess/EntryPoint/Cocoa/XPCService/WebContentServiceEntryPoint.mm b/Source/WebKit/WebProcess/EntryPoint/Cocoa/XPCService/WebContentServiceEntryPoint.mm
>index 87ac13289a341232703cf6620bbaa28088334f16..28ce024c7c54930755658b998b1f4c084ed9c103 100644
>--- a/Source/WebKit/WebProcess/EntryPoint/Cocoa/XPCService/WebContentServiceEntryPoint.mm
>+++ b/Source/WebKit/WebProcess/EntryPoint/Cocoa/XPCService/WebContentServiceEntryPoint.mm
>@@ -41,7 +41,7 @@ void WebContentServiceInitializer(xpc_connection_t connection, xpc_object_t init
> {
>     // Remove the WebProcessShim from the DYLD_INSERT_LIBRARIES environment variable so any processes spawned by
>     // the this process don't try to insert the shim and crash.
>-    WebKit::EnvironmentUtilities::stripValuesEndingWithString("DYLD_INSERT_LIBRARIES", "/WebProcessShim.dylib");
>+    WebKit::EnvironmentUtilities::removeValuesEndingWith("DYLD_INSERT_LIBRARIES", "/WebProcessShim.dylib");
> 
> #if PLATFORM(IOS_FAMILY)
>     GSInitialize();
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 7b24a87773a53f6d84aea224a7dbdb2f2572fd4c..97d5180ff036010e157f6e0351b3c68abd38d980 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-02-21  Ross Kirsling  <ross.kirsling@sony.com>
>+
>+        EnvironmentUtilities::stripValuesEndingWithString isn't thread-safe
>+        https://bugs.webkit.org/show_bug.cgi?id=194612
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * TestWebKitAPI/Tests/WebKit/EnvironmentUtilitiesTest.cpp:
>+        Just test the new string-processing function and don't touch the actual environment.
>+
> 2019-02-21  Tim Horton  <timothy_horton@apple.com>
> 
>         Fix the macOS build
>diff --git a/Tools/TestWebKitAPI/Tests/WebKit/EnvironmentUtilitiesTest.cpp b/Tools/TestWebKitAPI/Tests/WebKit/EnvironmentUtilitiesTest.cpp
>index e1de83265fbf71dc683e7f3c51b553cb377ca8a4..41d035d23cb8ca11837f7863467bd212b9db45d2 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKit/EnvironmentUtilitiesTest.cpp
>+++ b/Tools/TestWebKitAPI/Tests/WebKit/EnvironmentUtilitiesTest.cpp
>@@ -1,5 +1,6 @@
> /*
>  * Copyright (C) 2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2019 Sony Interactive Entertainment Inc.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -27,60 +28,56 @@
> #include "Test.h"
> 
> #include <WebKit/EnvironmentUtilities.h>
>-#include <stdlib.h>
> 
> namespace TestWebKitAPI {
> 
>-const char* const environmentVariable = "DYLD_INSERT_LIBRARIES";
> #define PROCESS_DYLIB "Process.dylib"
> const char* const stripValue = "/" PROCESS_DYLIB;
> 
>-static const char* strip(const char* input)
>+static String strip(StringView input)
> {
>-    setenv(environmentVariable, input, 1);
>-    WebKit::EnvironmentUtilities::stripValuesEndingWithString(environmentVariable, stripValue);
>-    return getenv(environmentVariable);
>+    return WebKit::EnvironmentUtilities::stripEntriesEndingWith(input, stripValue);
> }
> 
>-TEST(WebKit, StripValuesEndingWithString)
>+TEST(WebKit, StripEntriesEndingWith)
> {
>-    EXPECT_STREQ(strip(""), "");
>-    EXPECT_STREQ(strip(":"), ":");
>-    EXPECT_STREQ(strip("::"), "::");
>-    EXPECT_STREQ(strip(":::"), ":::");
>-    EXPECT_STREQ(strip("::::"), "::::");
>-    EXPECT_STREQ(strip(":::::"), ":::::");
>+    EXPECT_EQ(strip(""), "");
>+    EXPECT_EQ(strip(":"), ":");
>+    EXPECT_EQ(strip("::"), "::");
>+    EXPECT_EQ(strip(":::"), ":::");
>+    EXPECT_EQ(strip("::::"), "::::");
>+    EXPECT_EQ(strip(":::::"), ":::::");
> 
>-    EXPECT_STREQ(strip(PROCESS_DYLIB), PROCESS_DYLIB);
>-    EXPECT_STREQ(strip(":" PROCESS_DYLIB), ":" PROCESS_DYLIB);
>-    EXPECT_STREQ(strip(PROCESS_DYLIB ":"), PROCESS_DYLIB ":");
>-    EXPECT_STREQ(strip(":" PROCESS_DYLIB ":"), ":" PROCESS_DYLIB ":");
>+    EXPECT_EQ(strip(PROCESS_DYLIB), PROCESS_DYLIB);
>+    EXPECT_EQ(strip(":" PROCESS_DYLIB), ":" PROCESS_DYLIB);
>+    EXPECT_EQ(strip(PROCESS_DYLIB ":"), PROCESS_DYLIB ":");
>+    EXPECT_EQ(strip(":" PROCESS_DYLIB ":"), ":" PROCESS_DYLIB ":");
> 
>-    EXPECT_STREQ(strip("/" PROCESS_DYLIB), nullptr);
>-    EXPECT_STREQ(strip(":/" PROCESS_DYLIB), nullptr);
>-    EXPECT_STREQ(strip("/" PROCESS_DYLIB ":"), nullptr);
>-    EXPECT_STREQ(strip(":/" PROCESS_DYLIB ":"), ":");
>+    EXPECT_EQ(strip("/" PROCESS_DYLIB), "");
>+    EXPECT_EQ(strip(":/" PROCESS_DYLIB), "");
>+    EXPECT_EQ(strip("/" PROCESS_DYLIB ":"), "");
>+    EXPECT_EQ(strip(":/" PROCESS_DYLIB ":"), ":");
> 
>-    EXPECT_STREQ(strip(PROCESS_DYLIB "/"), PROCESS_DYLIB "/");
>-    EXPECT_STREQ(strip(":" PROCESS_DYLIB "/"), ":" PROCESS_DYLIB "/");
>-    EXPECT_STREQ(strip(PROCESS_DYLIB "/:"), PROCESS_DYLIB "/:");
>-    EXPECT_STREQ(strip(":" PROCESS_DYLIB "/:"), ":" PROCESS_DYLIB "/:");
>+    EXPECT_EQ(strip(PROCESS_DYLIB "/"), PROCESS_DYLIB "/");
>+    EXPECT_EQ(strip(":" PROCESS_DYLIB "/"), ":" PROCESS_DYLIB "/");
>+    EXPECT_EQ(strip(PROCESS_DYLIB "/:"), PROCESS_DYLIB "/:");
>+    EXPECT_EQ(strip(":" PROCESS_DYLIB "/:"), ":" PROCESS_DYLIB "/:");
> 
>-    EXPECT_STREQ(strip("/" PROCESS_DYLIB "/"), "/" PROCESS_DYLIB "/");
>-    EXPECT_STREQ(strip(":/" PROCESS_DYLIB "/"), ":/" PROCESS_DYLIB "/");
>-    EXPECT_STREQ(strip("/" PROCESS_DYLIB "/:"), "/" PROCESS_DYLIB "/:");
>-    EXPECT_STREQ(strip(":/" PROCESS_DYLIB "/:"), ":/" PROCESS_DYLIB "/:");
>+    EXPECT_EQ(strip("/" PROCESS_DYLIB "/"), "/" PROCESS_DYLIB "/");
>+    EXPECT_EQ(strip(":/" PROCESS_DYLIB "/"), ":/" PROCESS_DYLIB "/");
>+    EXPECT_EQ(strip("/" PROCESS_DYLIB "/:"), "/" PROCESS_DYLIB "/:");
>+    EXPECT_EQ(strip(":/" PROCESS_DYLIB "/:"), ":/" PROCESS_DYLIB "/:");
> 
>-    EXPECT_STREQ(strip("/Before.dylib:/" PROCESS_DYLIB), "/Before.dylib");
>-    EXPECT_STREQ(strip("/" PROCESS_DYLIB ":/After.dylib"), "/After.dylib");
>-    EXPECT_STREQ(strip("/Before.dylib:/" PROCESS_DYLIB ":/After.dylib"), "/Before.dylib:/After.dylib");
>-    EXPECT_STREQ(strip("/Before.dylib:/" PROCESS_DYLIB ":/Middle.dylib:/" PROCESS_DYLIB ":/After.dylib"), "/Before.dylib:/Middle.dylib:/After.dylib");
>+    EXPECT_EQ(strip("/Before.dylib:/" PROCESS_DYLIB), "/Before.dylib");
>+    EXPECT_EQ(strip("/" PROCESS_DYLIB ":/After.dylib"), "/After.dylib");
>+    EXPECT_EQ(strip("/Before.dylib:/" PROCESS_DYLIB ":/After.dylib"), "/Before.dylib:/After.dylib");
>+    EXPECT_EQ(strip("/Before.dylib:/" PROCESS_DYLIB ":/Middle.dylib:/" PROCESS_DYLIB ":/After.dylib"), "/Before.dylib:/Middle.dylib:/After.dylib");
> 
>-    EXPECT_STREQ(strip("/" PROCESS_DYLIB ":/" PROCESS_DYLIB), nullptr);
>-    EXPECT_STREQ(strip("/" PROCESS_DYLIB ":/" PROCESS_DYLIB ":/" PROCESS_DYLIB), nullptr);
>+    EXPECT_EQ(strip("/" PROCESS_DYLIB ":/" PROCESS_DYLIB), "");
>+    EXPECT_EQ(strip("/" PROCESS_DYLIB ":/" PROCESS_DYLIB ":/" PROCESS_DYLIB), "");
> 
>-    EXPECT_STREQ(strip("/usr/lib/" PROCESS_DYLIB), nullptr);
>-    EXPECT_STREQ(strip("/" PROCESS_DYLIB "/" PROCESS_DYLIB), nullptr);
>+    EXPECT_EQ(strip("/usr/lib/" PROCESS_DYLIB), "");
>+    EXPECT_EQ(strip("/" PROCESS_DYLIB "/" PROCESS_DYLIB), "");
> }
> 
> }

Actions: View | Formatted Diff | Diff

Attachments on bug 194612: 362539 | 362557 | 362659 | 363377