[webkit-reviews] review granted: [Bug 74061] WebProcess and PluginProcess should inherit environment variables provided in LC_DYLD_ENVIRONMENT of main executable binary : [Attachment 118547] Patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mark Rowe (bdash) <mrowe at apple.com>'s
request for review:
Bug 74061: WebProcess and PluginProcess should inherit environment variables
provided in LC_DYLD_ENVIRONMENT of main executable binary
https://bugs.webkit.org/show_bug.cgi?id=74061

Attachment 118547: Patch v2
https://bugs.webkit.org/attachment.cgi?id=118547&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=118547&action=review


Not sure why the patch failed to apply for EWS.

> Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:83

> +    String name(environmentString, nameLength);

If the only thing we ever do with this String is change it back into a C
string, maybe it should be CString instead of String.

> Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:89

> +    String value(equalsLocation + 1);

Ditto.

> Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:90

> +    m_extractedVariables.add(name, value);

If the only thing we do with these is turn them back into a vector to pass
along, maybe we should use a vector rather than a map.

>
Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:110
> +	   if (length < sizeof(dylinker_command) +
environmentCommand.name.offset)
> +	       return 0;

I think it would be clearer to instead check that loadCommand.cmdsize is >
environmentCommand.name.offset.

>
Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:132
> +    for (int i = 0; i < numberOfCommands; i++) {
> +	   size_t commandLength = processLoadCommand(data, length,
shouldByteSwap);
> +	   if (!commandLength || length < commandLength)
> +	       return;
> +
> +	   data = static_cast<const char*>(data) + commandLength;
> +	   length -= commandLength;
> +    }

I think it would be a little clearer to use a local variable named
dataRemaining of type const char* and another named lengthRemaining rather than
changing the arguments, but I think it’s an arguable style issue.

>
Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:168
> +    size_t numberOfArchitectures = OSSwapBigToHostInt32(header->nfat_arch);
> +    if (length < sizeof(fat_header) + sizeof(fat_arch) *
numberOfArchitectures)
> +	   return;

The multiplication here can overflow. It would be better to write this in a way
that can’t overflow, perhaps using division.