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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied  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 118342: Patch v1
https://bugs.webkit.org/attachment.cgi?id=118342&action=review

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


> Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.h:53
> +    void processSingleArchitecture(const void *fileData, size_t fileSize);
> +    void processFatFile(const void *fileData, size_t fileSize);
> +    void processLoadCommands(const void *fileData, int32_t numberOfCommands,
bool shouldByteSwap);
> +    size_t processLoadCommand(const load_command *rawLoadCommand, bool
shouldByteSwap);

Should move those stars over so it's const void*.

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

> +    const void* mainExecutableBytes = [mainExecutableData bytes];
> +    uint32_t magicValue = *static_cast<const
uint32_t*>(mainExecutableBytes);

Should check that length >= sizeof(uint32_t) before doing this.

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

> +#define DEFINE_BYTE_SWAPPER(type) static type byteSwapIfNeeded(const type&
data, bool shouldByteSwap) \

Should these be inlined?

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

> +size_t DynamicLinkerEnvironmentExtractor::processLoadCommand(const
load_command *rawLoadCommand, bool shouldByteSwap)

Move the *.

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

> +	   dylinker_command environmentCommand =
byteSwapIfNeeded(*reinterpret_cast<const dylinker_command*>(rawLoadCommand),
shouldByteSwap);
> +	   const char* environmentString = reinterpret_cast<const
char*>(rawLoadCommand) + environmentCommand.name.offset;

What prevents this from reading past the end of the buffer? Seems like we are
assuming the data is valid.

Even casting to dylinker_command assumes that it’s no longer than a
load_command.

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

> +    const load_command *loadCommand = static_cast<const
load_command*>(fileData);

What guarantees this doesn’t read past the end of the buffer.

>
Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:100
> +	   size_t commandLength = processLoadCommand(loadCommand,
shouldByteSwap);

Maybe the cast to load_command* should be here and the pointer should just be a
const char*. That would be a smaller number of casts.

>
Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:107
> +    const mach_header *header = static_cast<const mach_header*>(fileData);

Need to check file size before doing reading the data.

>
Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:112
> +	       processLoadCommands(static_cast<const char*>(fileData) +
sizeof(*header), swappedHeader.ncmds, shouldByteSwap);

Need to pass file size in, I think.

>
Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:126
> +    const fat_header *header = static_cast<const fat_header*>(fileData);
> +    const fat_arch *archs = reinterpret_cast<const
fat_arch*>(reinterpret_cast<const char*>(fileData) + sizeof(*header));

Should move the * characters. Should check the file size.

>
Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:134
> +	   CString name = it->first.utf8();

Why UTF-8? When we constructed the strings we treated them as Latin-1, not
UTF-8.