Changeset 242292 in webkit


Ignore:
Timestamp:
Mar 1, 2019 3:30:16 PM (5 years ago)
Author:
Ross Kirsling
Message:

EnvironmentUtilities::stripValuesEndingWithString isn't thread-safe
​https://bugs.webkit.org/show_bug.cgi?id=194612

Reviewed by Alex Christensen.

Source/WebKit:

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.

Tools:

  • TestWebKitAPI/Tests/WebKit/EnvironmentUtilitiesTest.cpp:

Just test the new string-processing function and don't touch the actual environment.
(Test cases are all as before, but based on operator== instead of strcmp.)

Location:
trunk
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r242289 r242292  
     12019-03-01  Ross Kirsling  <ross.kirsling@sony.com>
     2
     3        EnvironmentUtilities::stripValuesEndingWithString isn't thread-safe
     4        https://bugs.webkit.org/show_bug.cgi?id=194612
     5
     6        Reviewed by Alex Christensen.
     7
     8        This API test really shouldn't be verifying that the actual environment was successfully modified.
     9
     10        At its core, stripValuesEndingWithString is really just split-filter-join. By replacing it with a pair of
     11        simple functions -- one for string processing, one for environment processing -- the API test only needs to
     12        worry about the former.
     13
     14        * Platform/unix/EnvironmentUtilities.cpp:
     15        (WebKit::EnvironmentUtilities::stripEntriesEndingWith):
     16        (WebKit::EnvironmentUtilities::removeValuesEndingWith):
     17        (WebKit::EnvironmentUtilities::stripValuesEndingWithString): Deleted.
     18        * Platform/unix/EnvironmentUtilities.h:
     19        Replace old function with a pair of simpler ones.
     20
     21        * NetworkProcess/EntryPoint/Cocoa/XPCService/NetworkServiceEntryPoint.mm:
     22        (NetworkServiceInitializer):
     23        * PluginProcess/EntryPoint/Cocoa/XPCService/PluginServiceEntryPoint.mm:
     24        (PluginServiceInitializer):
     25        * WebProcess/EntryPoint/Cocoa/XPCService/WebContentServiceEntryPoint.mm:
     26        (WebContentServiceInitializer):
     27        Update function name.
     28
    1292019-03-01  Don Olmstead  <don.olmstead@sony.com>
    230

  • trunk/Source/WebKit/NetworkProcess/EntryPoint/Cocoa/XPCService/NetworkServiceEntryPoint.mm

    r240683 r242292  
    5757    // Remove the SecItemShim from the DYLD_INSERT_LIBRARIES environment variable so any processes spawned by
    5858    // the this process don't try to insert the shim and crash.
    59     EnvironmentUtilities::stripValuesEndingWithString("DYLD_INSERT_LIBRARIES", "/SecItemShim.dylib");
     59    EnvironmentUtilities::removeValuesEndingWith("DYLD_INSERT_LIBRARIES", "/SecItemShim.dylib");
    6060    XPCServiceInitializer<NetworkProcess, NetworkServiceInitializerDelegate>(adoptOSObject(connection), initializerMessage, priorityBoostMessage);
    6161}

  • trunk/Source/WebKit/Platform/unix/EnvironmentUtilities.cpp

    r241593 r242292  
    11/*
    22 * Copyright (C) 2011 Apple Inc. All rights reserved.
     3 * Copyright (C) 2019 Sony Interactive Entertainment Inc.
    34 *
    45 * Redistribution and use in source and binary forms, with or without
    … …  
    2728#include "EnvironmentUtilities.h"
    2829
    29 #include <wtf/text/CString.h>
     30#include <cstdlib>
     31#include <wtf/text/StringBuilder.h>
    3032
    3133namespace WebKit {
    … …  
    3335namespace EnvironmentUtilities {
    3436
    35 void stripValuesEndingWithString(const char* environmentVariable, const char* searchValue)
     37String stripEntriesEndingWith(StringView input, StringView suffix)
    3638{
    37     ASSERT(environmentVariable);
    38     ASSERT(searchValue);
    39    
    40     // Grab the current value of the environment variable.
    41     char* environmentValue = getenv(environmentVariable);
     39    StringBuilder output;
    4240
    43     if (!environmentValue || environmentValue[0] == '\0')
     41    auto hasAppended = false;
     42    for (auto entry : input.splitAllowingEmptyEntries(':')) {
     43        if (entry.endsWith(suffix))
     44            continue;
     45
     46        if (hasAppended)
     47            output.append(':');
     48        else
     49            hasAppended = true;
     50
     51        output.append(entry);
     52    }
     53
     54    return output.toString();
     55}
     56
     57void removeValuesEndingWith(const char* environmentVariable, const char* searchValue)
     58{
     59    const char* before = getenv(environmentVariable);
     60    if (!before)
    4461        return;
    4562
    46     const size_t environmentValueLength = strlen(environmentValue);
    47     const size_t environmentValueBufferLength = environmentValueLength + 1;
    48 
    49     // Set up the strings we'll be searching for.
    50     size_t searchLength = strlen(searchValue);
    51     if (!searchLength)
    52         return;
    53 
    54     Vector<char> searchValueWithColonVector;
    55     searchValueWithColonVector.grow(searchLength + 2);
    56     char* searchValueWithColon = searchValueWithColonVector.data();
    57     size_t searchLengthWithColon = searchLength + 1;
    58 
    59     memcpy(searchValueWithColon, searchValue, searchLength);
    60     searchValueWithColon[searchLength] = ':';
    61     searchValueWithColon[searchLengthWithColon] = '\0';
    62    
    63     // Loop over environmentValueBuffer, removing any components that match the search value ending with a colon.
    64     char* componentStart = environmentValue;
    65     char* match = strnstr(componentStart, searchValueWithColon, environmentValueLength - static_cast<size_t>(componentStart - environmentValue));
    66     bool foundAnyMatches = match != NULL;
    67     while (match != NULL) {
    68         // Update componentStart to point to the colon immediately preceding the match.
    69         char* nextColon = strnstr(componentStart, ":", environmentValueLength - static_cast<size_t>(componentStart - environmentValue));
    70         while (nextColon && nextColon < match) {
    71             componentStart = nextColon;
    72             nextColon = strnstr(componentStart + 1, ":", environmentValueLength - static_cast<size_t>(componentStart + 1 - environmentValue));
    73         }
    74 
    75         RELEASE_ASSERT(componentStart >= environmentValue);
    76         size_t environmentValueOffset = static_cast<size_t>(componentStart - environmentValue);
    77         RELEASE_ASSERT(environmentValueOffset < environmentValueBufferLength);
    78 
    79         // Copy over everything right of the match to the current component start, and search from there again.
    80         if (componentStart[0] == ':') {
    81             // If componentStart points to a colon, copy the colon over.
    82             strlcpy(componentStart, match + searchLength, environmentValueBufferLength - environmentValueOffset);
    83         } else {
    84             // Otherwise, componentStart still points to the beginning of environmentValueBuffer, so don't copy over the colon.
    85             // The edge case is if the colon is the last character in the string, so "match + searchLengthWithoutColon + 1" is the
    86             // null terminator of the original input, in which case this is still safe.
    87             strlcpy(componentStart, match + searchLengthWithColon, environmentValueBufferLength - environmentValueOffset);
    88         }
    89        
    90         match = strnstr(componentStart, searchValueWithColon, environmentValueLength - static_cast<size_t>(componentStart - environmentValue));
    91     }
    92    
    93     // Search for the value without a trailing colon, seeing if the original input ends with it.
    94     match = strnstr(componentStart, searchValue, environmentValueLength - static_cast<size_t>(componentStart - environmentValue));
    95     while (match != NULL) {
    96         if (match[searchLength] == '\0')
    97             break;
    98         match = strnstr(match + 1, searchValue, environmentValueLength - static_cast<size_t>(match + 1 - environmentValue));
    99     }
    100    
    101     // Since the original input ends with the search, strip out the last component.
    102     if (match) {
    103         // Update componentStart to point to the colon immediately preceding the match.
    104         char* nextColon = strnstr(componentStart, ":", environmentValueLength - static_cast<size_t>(componentStart - environmentValue));
    105         while (nextColon && nextColon < match) {
    106             componentStart = nextColon;
    107             nextColon = strnstr(componentStart + 1, ":", environmentValueLength - static_cast<size_t>(componentStart + 1 - environmentValue));
    108         }
    109        
    110         // Whether componentStart points to the original string or the last colon, putting the null terminator there will get us the desired result.
    111         componentStart[0] = '\0';
    112 
    113         foundAnyMatches = true;
    114     }
    115 
    116     // If we found no matches, don't change anything.
    117     if (!foundAnyMatches)
    118         return;
    119 
    120     // If we have nothing left, just unset the variable
    121     if (environmentValue[0] == '\0') {
     63    auto after = stripEntriesEndingWith(before, searchValue);
     64    if (after.isEmpty()) {
    12265        unsetenv(environmentVariable);
    12366        return;
    12467    }
    125    
    126     setenv(environmentVariable, environmentValue, 1);
     68
     69    setenv(environmentVariable, after.utf8().data(), 1);
    12770}
    12871

  • trunk/Source/WebKit/Platform/unix/EnvironmentUtilities.h

    r215521 r242292  
    11/*
    22 * Copyright (C) 2011 Apple Inc. All rights reserved.
     3 * Copyright (C) 2019 Sony Interactive Entertainment Inc.
    34 *
    45 * Redistribution and use in source and binary forms, with or without
    … …  
    2728
    2829#include "WKDeclarationSpecifiers.h"
     30#include <wtf/text/StringView.h>
    2931#include <wtf/text/WTFString.h>
    3032
    … …  
    3335namespace EnvironmentUtilities {
    3436
    35 WK_EXPORT void stripValuesEndingWithString(const char* environmentVariable, const char* search);
     37WK_EXPORT String stripEntriesEndingWith(StringView input, StringView suffix);
     38WK_EXPORT void removeValuesEndingWith(const char* environmentVariable, const char* search);
    3639
    3740} // namespace EnvironmentUtilities

  • trunk/Source/WebKit/PluginProcess/EntryPoint/Cocoa/XPCService/PluginServiceEntryPoint.mm

    r233983 r242292  
    7979    // Remove the PluginProcess shim from the DYLD_INSERT_LIBRARIES environment variable so any processes
    8080    // spawned by the PluginProcess don't try to insert the shim and crash.
    81     EnvironmentUtilities::stripValuesEndingWithString("DYLD_INSERT_LIBRARIES", "/PluginProcessShim.dylib");
     81    EnvironmentUtilities::removeValuesEndingWith("DYLD_INSERT_LIBRARIES", "/PluginProcessShim.dylib");
    8282    XPCServiceInitializer<PluginProcess, PluginServiceInitializerDelegate>(adoptOSObject(connection), initializerMessage, priorityBoostMessage);
    8383#endif // ENABLE(NETSCAPE_PLUGIN_API)

  • trunk/Source/WebKit/WebProcess/EntryPoint/Cocoa/XPCService/WebContentServiceEntryPoint.mm

    r237266 r242292  
    4242    // Remove the WebProcessShim from the DYLD_INSERT_LIBRARIES environment variable so any processes spawned by
    4343    // the this process don't try to insert the shim and crash.
    44     WebKit::EnvironmentUtilities::stripValuesEndingWithString("DYLD_INSERT_LIBRARIES", "/WebProcessShim.dylib");
     44    WebKit::EnvironmentUtilities::removeValuesEndingWith("DYLD_INSERT_LIBRARIES", "/WebProcessShim.dylib");
    4545
    4646#if PLATFORM(IOS_FAMILY)

  • trunk/Tools/ChangeLog

    r242291 r242292  
     12019-03-01  Ross Kirsling  <ross.kirsling@sony.com>
     2
     3        EnvironmentUtilities::stripValuesEndingWithString isn't thread-safe
     4        https://bugs.webkit.org/show_bug.cgi?id=194612
     5
     6        Reviewed by Alex Christensen.
     7
     8        * TestWebKitAPI/Tests/WebKit/EnvironmentUtilitiesTest.cpp:
     9        Just test the new string-processing function and don't touch the actual environment.
     10        (Test cases are all as before, but based on operator== instead of strcmp.)
     11
    1122019-03-01  Aakash Jain  <aakash_jain@apple.com>
    213

  • trunk/Tools/TestWebKitAPI/Tests/WebKit/EnvironmentUtilitiesTest.cpp

    r241593 r242292  
    11/*
    22 * Copyright (C) 2017 Apple Inc. All rights reserved.
     3 * Copyright (C) 2019 Sony Interactive Entertainment Inc.
    34 *
    45 * Redistribution and use in source and binary forms, with or without
    … …  
    2829
    2930#include <WebKit/EnvironmentUtilities.h>
    30 #include <stdlib.h>
    3131
    3232namespace TestWebKitAPI {
    3333
    34 const char* const environmentVariable = "DYLD_INSERT_LIBRARIES";
    3534#define PROCESS_DYLIB "Process.dylib"
    3635const char* const stripValue = "/" PROCESS_DYLIB;
    3736
    38 static const char* strip(const char* input)
     37static String strip(StringView input)
    3938{
    40     setenv(environmentVariable, input, 1);
    41     WebKit::EnvironmentUtilities::stripValuesEndingWithString(environmentVariable, stripValue);
    42     return getenv(environmentVariable);
     39    return WebKit::EnvironmentUtilities::stripEntriesEndingWith(input, stripValue);
    4340}
    4441
    45 TEST(WebKit, StripValuesEndingWithString)
     42TEST(WebKit, StripEntriesEndingWith)
    4643{
    47     EXPECT_STREQ(strip(""), "");
    48     EXPECT_STREQ(strip(":"), ":");
    49     EXPECT_STREQ(strip("::"), "::");
    50     EXPECT_STREQ(strip(":::"), ":::");
    51     EXPECT_STREQ(strip("::::"), "::::");
    52     EXPECT_STREQ(strip(":::::"), ":::::");
     44    EXPECT_EQ(strip(""), "");
     45    EXPECT_EQ(strip(":"), ":");
     46    EXPECT_EQ(strip("::"), "::");
     47    EXPECT_EQ(strip(":::"), ":::");
     48    EXPECT_EQ(strip("::::"), "::::");
     49    EXPECT_EQ(strip(":::::"), ":::::");
    5350
    54     EXPECT_STREQ(strip(PROCESS_DYLIB), PROCESS_DYLIB);
    55     EXPECT_STREQ(strip(":" PROCESS_DYLIB), ":" PROCESS_DYLIB);
    56     EXPECT_STREQ(strip(PROCESS_DYLIB ":"), PROCESS_DYLIB ":");
    57     EXPECT_STREQ(strip(":" PROCESS_DYLIB ":"), ":" PROCESS_DYLIB ":");
     51    EXPECT_EQ(strip(PROCESS_DYLIB), PROCESS_DYLIB);
     52    EXPECT_EQ(strip(":" PROCESS_DYLIB), ":" PROCESS_DYLIB);
     53    EXPECT_EQ(strip(PROCESS_DYLIB ":"), PROCESS_DYLIB ":");
     54    EXPECT_EQ(strip(":" PROCESS_DYLIB ":"), ":" PROCESS_DYLIB ":");
    5855
    59     EXPECT_STREQ(strip("/" PROCESS_DYLIB), nullptr);
    60     EXPECT_STREQ(strip(":/" PROCESS_DYLIB), nullptr);
    61     EXPECT_STREQ(strip("/" PROCESS_DYLIB ":"), nullptr);
    62     EXPECT_STREQ(strip(":/" PROCESS_DYLIB ":"), ":");
     56    EXPECT_EQ(strip("/" PROCESS_DYLIB), "");
     57    EXPECT_EQ(strip(":/" PROCESS_DYLIB), "");
     58    EXPECT_EQ(strip("/" PROCESS_DYLIB ":"), "");
     59    EXPECT_EQ(strip(":/" PROCESS_DYLIB ":"), ":");
    6360
    64     EXPECT_STREQ(strip(PROCESS_DYLIB "/"), PROCESS_DYLIB "/");
    65     EXPECT_STREQ(strip(":" PROCESS_DYLIB "/"), ":" PROCESS_DYLIB "/");
    66     EXPECT_STREQ(strip(PROCESS_DYLIB "/:"), PROCESS_DYLIB "/:");
    67     EXPECT_STREQ(strip(":" PROCESS_DYLIB "/:"), ":" PROCESS_DYLIB "/:");
     61    EXPECT_EQ(strip(PROCESS_DYLIB "/"), PROCESS_DYLIB "/");
     62    EXPECT_EQ(strip(":" PROCESS_DYLIB "/"), ":" PROCESS_DYLIB "/");
     63    EXPECT_EQ(strip(PROCESS_DYLIB "/:"), PROCESS_DYLIB "/:");
     64    EXPECT_EQ(strip(":" PROCESS_DYLIB "/:"), ":" PROCESS_DYLIB "/:");
    6865
    69     EXPECT_STREQ(strip("/" PROCESS_DYLIB "/"), "/" PROCESS_DYLIB "/");
    70     EXPECT_STREQ(strip(":/" PROCESS_DYLIB "/"), ":/" PROCESS_DYLIB "/");
    71     EXPECT_STREQ(strip("/" PROCESS_DYLIB "/:"), "/" PROCESS_DYLIB "/:");
    72     EXPECT_STREQ(strip(":/" PROCESS_DYLIB "/:"), ":/" PROCESS_DYLIB "/:");
     66    EXPECT_EQ(strip("/" PROCESS_DYLIB "/"), "/" PROCESS_DYLIB "/");
     67    EXPECT_EQ(strip(":/" PROCESS_DYLIB "/"), ":/" PROCESS_DYLIB "/");
     68    EXPECT_EQ(strip("/" PROCESS_DYLIB "/:"), "/" PROCESS_DYLIB "/:");
     69    EXPECT_EQ(strip(":/" PROCESS_DYLIB "/:"), ":/" PROCESS_DYLIB "/:");
    7370
    74     EXPECT_STREQ(strip("/Before.dylib:/" PROCESS_DYLIB), "/Before.dylib");
    75     EXPECT_STREQ(strip("/" PROCESS_DYLIB ":/After.dylib"), "/After.dylib");
    76     EXPECT_STREQ(strip("/Before.dylib:/" PROCESS_DYLIB ":/After.dylib"), "/Before.dylib:/After.dylib");
    77     EXPECT_STREQ(strip("/Before.dylib:/" PROCESS_DYLIB ":/Middle.dylib:/" PROCESS_DYLIB ":/After.dylib"), "/Before.dylib:/Middle.dylib:/After.dylib");
     71    EXPECT_EQ(strip("/Before.dylib:/" PROCESS_DYLIB), "/Before.dylib");
     72    EXPECT_EQ(strip("/" PROCESS_DYLIB ":/After.dylib"), "/After.dylib");
     73    EXPECT_EQ(strip("/Before.dylib:/" PROCESS_DYLIB ":/After.dylib"), "/Before.dylib:/After.dylib");
     74    EXPECT_EQ(strip("/Before.dylib:/" PROCESS_DYLIB ":/Middle.dylib:/" PROCESS_DYLIB ":/After.dylib"), "/Before.dylib:/Middle.dylib:/After.dylib");
    7875
    79     EXPECT_STREQ(strip("/" PROCESS_DYLIB ":/" PROCESS_DYLIB), nullptr);
    80     EXPECT_STREQ(strip("/" PROCESS_DYLIB ":/" PROCESS_DYLIB ":/" PROCESS_DYLIB), nullptr);
     76    EXPECT_EQ(strip("/" PROCESS_DYLIB ":/" PROCESS_DYLIB), "");
     77    EXPECT_EQ(strip("/" PROCESS_DYLIB ":/" PROCESS_DYLIB ":/" PROCESS_DYLIB), "");
    8178
    82     EXPECT_STREQ(strip("/usr/lib/" PROCESS_DYLIB), nullptr);
    83     EXPECT_STREQ(strip("/" PROCESS_DYLIB "/" PROCESS_DYLIB), nullptr);
     79    EXPECT_EQ(strip("/usr/lib/" PROCESS_DYLIB), "");
     80    EXPECT_EQ(strip("/" PROCESS_DYLIB "/" PROCESS_DYLIB), "");
    8481}
    8582
Note: See TracChangeset for help on using the changeset viewer.