Bug 74061 – WebProcess and PluginProcess should inherit environment variables provided in LC_DYLD_ENVIRONMENT of main executable binary

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Mark Rowe (bdash)]

Having WebProcess and PluginProcess inherit environment variables provided in any LC_DYLD_ENVIRONMENT load commands in the main executable will ensure that they will use the same set of libraries as the UI process.

[Mark Rowe (bdash)]

Created attachment 118342 [details]
Patch v1

I’m not entirely happy with some of the naming in this patch. I’m open to suggestions!

[Sam Weinig]

Comment on attachment 118342 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=118342&action=review

Please fix the *s, otherwise, r=me.

> Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.h:45
> +    DynamicLinkerEnvironmentExtractor(NSString* executablePath, cpu_type_t architecture);

Please move * to the other side.

> 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);

Please move * to the other-other side.

> Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:133
> +    for (HashMap<String, String>::const_iterator it = m_extractedVariables.begin(); it != m_extractedVariables.end(); ++it) {

We usually pull the end iterator out of the loop so it is not recomputed.

[Darin Adler]

Comment on attachment 118342 [details]
Patch v1

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.

[Alexey Proskuryakov]

I have no objection against this patch, but my understanding is that longer term, we may not want Safari to respect custom libraries specified at launch.

[Alexey Proskuryakov]

Mark explains to me that this is not specified at launch time anyway.

[Mark Rowe (bdash)]

I had considered doing stricter verification of the Mach-O structures but decided that since this is for the main executable then if they were in a bogus state we wouldn’t have been launched. That’s not really a good reason though so I’ll post a new patch in a moment that addresses this and the other feedback.

[Mark Rowe (bdash)]

Created attachment 118547 [details]
Patch v2

Now with sanity checking of the data we’re parsing.

[Darin Adler]

(In reply to comment #6)
> I had considered doing stricter verification of the Mach-O structures but decided that since this is for the main executable then if they were in a bogus state we wouldn’t have been launched.

Honestly, I think that would have been OK. If you had:

1) Stated this.
2) Not bothered to read the file size at all, rather than passing it to functions and then dropping it.

[Darin Adler]

Comment on attachment 118547 [details]
Patch v2

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.

[Mark Rowe (bdash)]

Created attachment 118610 [details]
Patch v3

[Darin Adler]

Comment on attachment 118610 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=118610&action=review

> Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:85
> +    String name(environmentString, nameLength);
> +
> +    // LC_DYLD_ENVIRONMENT only respects DYLD_*_PATH variables.
> +    if (!name.startsWith("DYLD_") || !name.endsWith("_PATH"))
> +        return;

Some day we should write a startsWith function that works on a CString and then we can remove this otherwise-unneeded conversion to String.

> Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:111
> +        OwnPtr<char> environmentString = adoptPtr(new char[environmentStringLength + 1]);

Needs to be OwnArrayPtr.

(Or you could use Vector<char, 256> and make the code a little faster since it will almost never malloc.)

> Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:186
> +    Vector<std::pair<CString, CString> >::const_iterator end = m_extractedVariables.end();
> +    for (Vector<std::pair<CString, CString> >::const_iterator it = m_extractedVariables.begin(); it != end; ++it) {

We normally just use indexing to iterate a vector.

> Source/WebKit2/UIProcess/Launcher/mac/DynamicLinkerEnvironmentExtractor.mm:187
> +        CString name = it->first;

More efficient to use const CString& here.

[Mark Rowe (bdash)]

Landed in r102497.

[Ryosuke Niwa]

It appears that this patch broke SL builds:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/36585
http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/36585/steps/compile-webkit/logs/stdio

[Mark Rowe (bdash)]

Yes. r102504 should fix that.