Bug 151392 – [iOS] ASSERTION FAILED: temporaryFilePath.last() == '/' in WebCore::openTemporaryFile()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

When I run run-test-api --debug --ios-simulator I see messages in the iOS simulator system log of the form:

Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: ASSERTION FAILED: temporaryFilePath.last() == '/'
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: /Volumes/Data/WebKitDevGit/OpenSource/Source/WebCore/platform/mac/FileSystemMac.mm(72) : WTF::String WebCore::openTemporaryFile(const WTF::String &, PlatformFileHandle &)
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 1   0x105de427d WTFCrash
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 2   0x10a4f08b7 WebCore::openTemporaryFile(WTF::String const&, int&)
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 3   0x104e16b16 TestWebKitAPI::FileSystemTest::SetUp()
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 4   0x104e3266e testing::Test::Run()
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 5   0x104e32f7d testing::internal::TestInfoImpl::Run()
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 6   0x104e33abd testing::TestCase::Run()
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 7   0x104e398bb testing::internal::UnitTestImpl::RunAllTests()
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 8   0x104e39539 testing::UnitTest::Run()
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 9   0x104daaf6c TestWebKitAPI::TestsController::run(int, char**)
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 10  0x104dfe3d5 main
Nov 18 09:57:17 dbates2-ruby TestWebKitAPI[96829]: 11  0x1132c592d start

[Daniel Bates]

<rdar://problem/23595341>

[Daniel Bates]

Created attachment 265751 [details]
Patch

[Alexey Proskuryakov]

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>?

[Daniel Bates]

(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.

[Daniel Bates]

(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?

[Daniel Bates]

Created attachment 265753 [details]
Patch

[Alexey Proskuryakov]

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.

[Daniel Bates]

(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."

[Daniel Bates]

(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

[Daniel Bates]

Committed r192583: <http://trac.webkit.org/changeset/192583>