Summary: | [iOS] ASSERTION FAILED: temporaryFilePath.last() == '/' in WebCore::openTemporaryFile() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
Component: | WebCore Misc. | Assignee: | Daniel Bates <dbates> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | andersca, ap, benjamin, cdumez, cmarcelo, commit-queue, msaboff | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Local Build | ||||||||
Hardware: | iPhone / iPad | ||||||||
OS: | iOS 9.0 | ||||||||
Attachments: |
|
Description
Daniel Bates
2015-11-18 10:12:57 PST
Created attachment 265751 [details]
Patch
Comment on attachment 265751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265751&action=review > Source/WTF/wtf/DataLog.cpp:-86 > - size_t lastComponentLength = strlen(logBasename) + suffixLength; suffixLength is ignored in the new code path, which doesn't seem right. > Source/WTF/wtf/DataLog.cpp:91 > + bool shouldAddPathSeparator = filenameBuffer[dirnameLength - 1] != '/' && logBasename[0] != '/'; It would be nice to check that these accesses are valid. > Source/WTF/wtf/DataLog.cpp:94 > + strcat(filenameBuffer, "/"); I think that attempting to use strcat will make some security tools yell at us. Please change to strncat. But also, is it feasible to switch this code over to Vector<char>? (In reply to comment #3) > Comment on attachment 265751 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265751&action=review > > > Source/WTF/wtf/DataLog.cpp:-86 > > - size_t lastComponentLength = strlen(logBasename) + suffixLength; > > suffixLength is ignored in the new code path, which doesn't seem right. > You're right! > > Source/WTF/wtf/DataLog.cpp:91 > > + bool shouldAddPathSeparator = filenameBuffer[dirnameLength - 1] != '/' && logBasename[0] != '/'; > > It would be nice to check that these accesses are valid. > Notice that confstr() returns a non-zero return value when the configuration-defined value exists. Specifically, when the configuration-defined value exists it returns the number bytes needed to hold the configuration-defined value. > > Source/WTF/wtf/DataLog.cpp:94 > > + strcat(filenameBuffer, "/"); > > I think that attempting to use strcat will make some security tools yell at > us. Please change to strncat. Will change to read: strncat(filenameBuffer, "/", 1); > But also, is it feasible to switch this code over to Vector<char>? From my understanding we want to support using data log functions anywhere for debugging purposes, including in Vector. Implementing data log in terms of Vector could lead to re-entrancy issues. (In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 265751 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=265751&action=review > > > > > Source/WTF/wtf/DataLog.cpp:-86 > > > - size_t lastComponentLength = strlen(logBasename) + suffixLength; > > > > suffixLength is ignored in the new code path, which doesn't seem right. > > > > You're right! > > > > Source/WTF/wtf/DataLog.cpp:91 > > > + bool shouldAddPathSeparator = filenameBuffer[dirnameLength - 1] != '/' && logBasename[0] != '/'; > > > > It would be nice to check that these accesses are valid. > > > > Notice that confstr() returns a non-zero return value when the > configuration-defined value exists. Specifically, when the > configuration-defined value exists it returns the number bytes needed to > hold the configuration-defined value. > I forgot to add that dirnameLength will always be >= 1 when confstr() returns a non-zero return value. Notice that logBasename is initialized with a string literal, DATA_LOG_FILENAME. So, it is always non-null. Having said that, logBasename is a char*. I take it you feel that future code may nullify logBasename? Created attachment 265753 [details]
Patch
Comment on attachment 265753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265753&action=review > Source/WTF/wtf/DataLog.cpp:89 > + // FIXME: As a workaround for <rdar://problem/23579077> - path retrieved by confstr(_CS_DARWIN_USER_TEMP_DIR, ..., ...) > + // may not have a trailing slash in iOS Simulator - we add a path separator, if needed. This FIXME is not a call for action. It should either say something like "FIXME: Change to a runtime assertion that the path ends with a slash once <rdar://problem/23579077> is fixed in all iOS Simulator versions that we use", or not be a FIXME. > Source/WebCore/platform/mac/FileSystemMac.mm:73 > + // FIXME: As a workaround for <rdar://problem/23579077> - path retrieved by confstr(_CS_DARWIN_USER_TEMP_DIR, ..., ...) Ditto. > Source/WebKit2/Shared/mac/SandboxExtensionMac.mm:253 > + // FIXME: As a workaround for <rdar://problem/23579077> - path retrieved by confstr(_CS_DARWIN_USER_TEMP_DIR, ..., ...) Ditto. (In reply to comment #7) > Comment on attachment 265753 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265753&action=review > > > Source/WTF/wtf/DataLog.cpp:89 > > + // FIXME: As a workaround for <rdar://problem/23579077> - path retrieved by confstr(_CS_DARWIN_USER_TEMP_DIR, ..., ...) > > + // may not have a trailing slash in iOS Simulator - we add a path separator, if needed. > > This FIXME is not a call for action. It should either say something like > "FIXME: Change to a runtime assertion that the path ends with a slash once > <rdar://problem/23579077> is fixed in all iOS Simulator versions that we > use", or not be a FIXME. > Will change this FIXME comment to read "FIXME: Assert that the path ends with a slash instead adding a slash if it does not exist once <rdar://problem/23579077> is fixed in all iOS Simulator versions that we use." > > Source/WebCore/platform/mac/FileSystemMac.mm:73 > > + // FIXME: As a workaround for <rdar://problem/23579077> - path retrieved by confstr(_CS_DARWIN_USER_TEMP_DIR, ..., ...) > > Ditto. Will change FIXME comment to read "FIXME: Change to a runtime assertion that the path ends with a slash once <rdar://problem/23579077> is fixed in all iOS Simulator versions that we use." > > > Source/WebKit2/Shared/mac/SandboxExtensionMac.mm:253 > > + // FIXME: As a workaround for <rdar://problem/23579077> - path retrieved by confstr(_CS_DARWIN_USER_TEMP_DIR, ..., ...) > > Ditto. Will change FIXME comment to read "FIXME: Change to a runtime assertion that the path ends with a slash once <rdar://problem/23579077> is fixed in all iOS Simulator versions that we use." (In reply to comment #8) > (In reply to comment #7) > > Comment on attachment 265753 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=265753&action=review > > > > > Source/WTF/wtf/DataLog.cpp:89 > > > + // FIXME: As a workaround for <rdar://problem/23579077> - path retrieved by confstr(_CS_DARWIN_USER_TEMP_DIR, ..., ...) > > > + // may not have a trailing slash in iOS Simulator - we add a path separator, if needed. > > > > This FIXME is not a call for action. It should either say something like > > "FIXME: Change to a runtime assertion that the path ends with a slash once > > <rdar://problem/23579077> is fixed in all iOS Simulator versions that we > > use", or not be a FIXME. > > > > Will change this FIXME comment to read "FIXME: Assert that the path ends > with a slash instead adding a slash if it does not exist once > <rdar://problem/23579077> is fixed in all iOS Simulator versions that we > use." *instead of Committed r192583: <http://trac.webkit.org/changeset/192583> |