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.
Created attachment 118342 [details] Patch v1 I’m not entirely happy with some of the naming in this patch. I’m open to suggestions!
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.
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.
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.
Mark explains to me that this is not specified at launch time anyway.
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.
Created attachment 118547 [details] Patch v2 Now with sanity checking of the data we’re parsing.
(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.
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.
Created attachment 118610 [details] Patch v3
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.
Landed in r102497.
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
Yes. r102504 should fix that.