Summary: | cryptographicallyRandomValuesFromOS should use CCRandomCopyBytes when available. | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||||||||||||
Component: | Web Template Framework | Assignee: | Keith Miller <keith_miller> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | ap, benjamin, cmarcelo, commit-queue, dbates, ggaren, webkit-bug-importer | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Attachments: |
|
Description
Keith Miller
2015-08-25 11:56:48 PDT
Created attachment 259869 [details]
Patch
Created attachment 259873 [details]
Patch
Should have pulled before formatting the patch...
Attachment 259873 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/OSRandomSource.cpp:70: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 259874 [details]
Patch
Didn't unmangle well enough.
Created attachment 259877 [details]
Patch
Removed debugging code that is now dead.
Created attachment 259878 [details]
Patch
(In reply to comment #6) > Created attachment 259878 [details] > Patch Tried to manually delete whitespace changes... it was a bad idea. Wpointer-arith -Wundef -Wwrite-strings -fPIC -MMD -MT Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o -MF Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o.d -o Source/WTF/wtf/CMakeFiles/WTF.dir/Atomics.cpp.o -c ../../Source/WTF/wtf/Atomics.cpp In file included from ../../Source/WTF/wtf/StdLibExtras.h:32:0, from ../../Source/WTF/wtf/FastMalloc.h:26, from ../../Source/WTF/config.h:51, from ../../Source/WTF/wtf/Atomics.cpp:59: ../../Source/WTF/wtf/Assertions.h:179:5: error: "HAVE_OS_TRACE" is not defined [-Werror=undef] #if HAVE_OS_TRACE Comment on attachment 259878 [details]
Patch
EWS build failed.
Created attachment 259880 [details]
Patch
Added defines for 0 case also.
Comment on attachment 259880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259880&action=review > Source/WTF/wtf/Assertions.h:55 > +#if defined(__has_include) > +#if __has_include(<os/trace.h>) > +#define HAVE_OS_TRACE 1 > +#include <os/trace.h> > +#else > +#define HAVE_OS_TRACE 0 > +#endif > +#endif Would it make sense to move the define for HAVE_OS_TRACE to wtf/Platform.h? Notice that wtf/Platform.h defines HAVE_OS_ACTIVITY, which we use to guard inclusion and data types defined in header os/activity.h. Comment on attachment 259880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259880&action=review > Source/WTF/wtf/Assertions.h:51 > +#include <os/trace.h> is this what breaks iOS build? /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/spi/darwin/XPCSPI.h:37:1: error: typedef redefinition with different types ('NSObject<OS_xpc_object> *' vs 'void *') OS_OBJECT_DECL(xpc_object); ^ > Source/WTF/wtf/OSRandomSource.cpp:33 > +#if __has_include(<CommonCrypto/CommonRandomSPI.h>) This check will fail for open source WebKit builds (including nightlies), because CommonRandomSPI.h is an internal header. In WebCore, we declare it in crypto/CommonCryptoUtilities.h - or you could add a CommonCryptoSPI header in WTF. > Source/WTF/wtf/OSRandomSource.cpp:72 > + if (result != kCCSuccess) > + CRASH_WITH_MESSAGE("crytographicallyRandomValuesFromOS could not get cryptographically random information from CCRandomCopyBytes and had error: %d", result); I'm not sure how this is more helpful than a simple RELEASE_ASSERT. We can see where that one happens, and CCRandomCopyBytes can't fail anyway. Comment on attachment 259880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259880&action=review >> Source/WTF/wtf/Assertions.h:51 >> +#include <os/trace.h> > > is this what breaks iOS build? > > /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/spi/darwin/XPCSPI.h:37:1: error: typedef redefinition with different types ('NSObject<OS_xpc_object> *' vs 'void *') > OS_OBJECT_DECL(xpc_object); > ^ It looks like it. There might be a work around however. >> Source/WTF/wtf/Assertions.h:55 >> +#endif > > Would it make sense to move the define for HAVE_OS_TRACE to wtf/Platform.h? Notice that wtf/Platform.h defines HAVE_OS_ACTIVITY, which we use to guard inclusion and data types defined in header os/activity.h. I'm not sure, It doesn't look like anyone else in the webkit uses os_trace as far as I can tell. I'm not opposed to this idea. >> Source/WTF/wtf/OSRandomSource.cpp:33 >> +#if __has_include(<CommonCrypto/CommonRandomSPI.h>) > > This check will fail for open source WebKit builds (including nightlies), because CommonRandomSPI.h is an internal header. > > In WebCore, we declare it in crypto/CommonCryptoUtilities.h - or you could add a CommonCryptoSPI header in WTF. I see, do you know of a reasonable upper bound on when CommonCrypto will be available? I would use the ENABLE(SUBTLE_CRYTPO) that is used in WebCore but it is not set in WTF and I'm not sure what conditions cause it to be set. >> Source/WTF/wtf/OSRandomSource.cpp:72 >> + CRASH_WITH_MESSAGE("crytographicallyRandomValuesFromOS could not get cryptographically random information from CCRandomCopyBytes and had error: %d", result); > > I'm not sure how this is more helpful than a simple RELEASE_ASSERT. We can see where that one happens, and CCRandomCopyBytes can't fail anyway. It's helpful in that if/when CCRandomCopyBytes fails the crash log will tell us the exact reason why it failed. Perhaps you are right, the documentation only states that CCRandomCopyBytes returns kCCSuccess if it succeeds but it doesn't say what it will do if it fails. Regardless, if in the future CCRandomCopyBytes can fail I would rather avoid a potential security risk and crash. > I see, do you know of a reasonable upper bound on when CommonCrypto will be available? CCRandomCopyBytes is available on all OS X and iOS versions that we build for. CommonCrypto/CommonRandomSPI.h is available on all OS X and iOS versions that we build for, but only for internal builds. I would just keep using #if PLATFORM(DARWIN). > Regardless, if in the future CCRandomCopyBytes can fail I would rather avoid a potential security risk and crash. Yes, we should have a RELEASE_ASSERT here. Adding as much code as you did just for this case doesn't seem worth it to me. If you still think that a more detailed message would be useful here, please send an e-mail to webkit-dev, so that everyone could discuss the best way to do this. Personally, I would do RELEASE_ASSERT_WITH_MESSAGE, so that we wouldn't need two lines of code at call sites. > > Regardless, if in the future CCRandomCopyBytes can fail I would rather avoid a potential security risk and crash.
>
> Yes, we should have a RELEASE_ASSERT here. Adding as much code as you did
> just for this case doesn't seem worth it to me.
>
> If you still think that a more detailed message would be useful here, please
> send an e-mail to webkit-dev, so that everyone could discuss the best way to
> do this. Personally, I would do RELEASE_ASSERT_WITH_MESSAGE, so that we
> wouldn't need two lines of code at call sites.
Based on conversations I have had with others it seems like having the ability to add debug information to crash logs seems desired. However, I can create an email chain to figure out the exact functionality/implementation we want. It is also unfortunate that currently os_trace does not allow non-scalar format strings so perhaps we should wait until that is added before moving forward.
Created attachment 259970 [details]
Patch
Keith, did you intend to mark the latest patch for review? Comment on attachment 259970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259970&action=review > Source/WTF/wtf/OSRandomSource.cpp:43 > +#if OS(DARWIN) This is not the way we've been doing this recently. A better way to add forward declarations is to put them into a separate SPI header, as that lets us reuse the code, and to reduce the noise at the top of .cpp files. > This is not the way we've been doing this recently. A better way to add
> forward declarations is to put them into a separate SPI header, as that lets
> us reuse the code, and to reduce the noise at the top of .cpp files.
I was thinking that it seemed unlikely anyone would use these in the future so it seemed like file bloat to add an SPI for just one function. If anyone else did want access to the forward declarations then I would agree that creating an SPI file makes sense.
Comment on attachment 259970 [details] Patch Rejecting attachment 259970 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 259970, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WTF/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.webkit.org/results/140765 Created attachment 260648 [details]
Patch
I guess I somehow deleted the reviewer field in the ChangeLog so Commit-Queue got confused. Comment on attachment 260648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260648&action=review > Source/WTF/wtf/OSRandomSource.cpp:47 > +#if OS(DARWIN) > +typedef struct __CCRandom* CCRandomRef; > +extern const CCRandomRef kCCRandomDefault; > +extern "C" int CCRandomCopyBytes(CCRandomRef rnd, void *bytes, size_t count); > +#endif This is not the correct idiom for new uses of SPI in WebKit. See examples like WTF/wtf/spi/darwin/XPCSPI.h and WTF/wtf/spi/cocoa/NSMapTableSPI.h for the way we now do it. Created attachment 261011 [details]
Patch
Created attachment 261013 [details]
Patch for landing
Created attachment 261015 [details]
Patch for landing
Comment on attachment 261015 [details] Patch for landing Rejecting attachment 261015 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 261015, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: a/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors self._run(tool, options, state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run step(tool, options).run(state) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run if changelog_entry.has_valid_reviewer(): AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer' Full output: http://webkit-queues.webkit.org/results/160990 Committed r189633: <http://trac.webkit.org/changeset/189633> |