[webkit-reviews] review denied: [Bug 61009] Processes spawned by SnowLeopard's WebProcess attempt to install WebKit2 shims : [Attachment 93941] Patch v3 - Better, shared code for sanitizing the variables and do it in PluginProcessMain also.

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Brady Eidson
<beidson at apple.com>'s request for review:
Bug 61009: Processes spawned by SnowLeopard's WebProcess attempt to install
WebKit2 shims
https://bugs.webkit.org/show_bug.cgi?id=61009

Attachment 93941: Patch v3 - Better, shared code for sanitizing the variables
and do it in PluginProcessMain also.
https://bugs.webkit.org/attachment.cgi?id=93941&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=93941&action=review

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:38
> +    const char* environmentVariableBuffer =
environmentVariable.utf8().data();

This is a dangling pointer. You need to declare a CString.

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:39
> +    String environmentValue =
String::fromUTF8(getenv(environmentVariableBuffer));

I'd assign getenv to a pointer and check for null before calling
String::fromUTF8.

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:49
> +    size_t count = values.size();
> +    for (size_t i = 0; i < count; ++i) {

In WebKit2 we usually write this

for (size_t i = 0, count = values.size(); i < count; ++i)

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:63
> +    // If the builder's length is 1 longer than the original environment
variable, that 1 character
> +    // is the unneeded trailing ":" and the actual values are the same, so
we're done.
> +    if (builder.length() == environmentValue.length() + 1)
> +	   return;

Could you assert that this is the case?

> Source/WebKit2/Platform/unix/EnvironmentUtilities.cpp:68
> +    char* newValueBuffer = const_cast<char*>(newValueCString.data());

I think you can use mutableData here, instead of doing a const_cast.