Bug 173647

Created attachment 313513 [details] WIP patch

Attachment 313513 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] 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 313517 [details] Patch Ready for review

Attachment 313517 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] 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 313518 [details] Patch Filled out the WebCore ChangeLog entry.

Comment on attachment 313518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313518&action=review Looks good to me. Please address the following comments. > Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:353 > + if (version->size() != 1 || version->at(0) != 0x00) We should have 0x00 as a constant as well. Will memcpy be more straightforward for readability? > Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:403 > + if (version->size() != 1 || version->at(0) != 0x01) Ditto. > Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:410 > + if (parameters) { I wonder if this part of code ever gets executed.

Comment on attachment 313518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313518&action=review >> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:353 >> + if (version->size() != 1 || version->at(0) != 0x00) > > We should have 0x00 as a constant as well. Will memcpy be more straightforward for readability? You probably mean memcmp. Problem with using that is if an empty Vector is (somehow) returned. If that happens, we'd be testing uninitialized memory. >> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:410 >> + if (parameters) { > > I wonder if this part of code ever gets executed. Probably not, I don't think there's any test cases covering it, in WebKit or web-platform-tests. Same I think for the optional `privateKey.publicKey` member. I've opened bug #174420 to add such a test.

(In reply to Jiewen Tan from comment #6) > Comment on attachment 313518 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313518&action=review > > Looks good to me. Please address the following comments. > Please r+ the patch if you're OK with the replies to the comments.

Comment on attachment 313518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313518&action=review >>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:353 >>> + if (version->size() != 1 || version->at(0) != 0x00) >> >> We should have 0x00 as a constant as well. Will memcpy be more straightforward for readability? > > You probably mean memcmp. Problem with using that is if an empty Vector is (somehow) returned. If that happens, we'd be testing uninitialized memory. ... otherwise yes, after the size is validated we'd be able to call memcmp and compare the vector data against an array constant.

Comment on attachment 313518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313518&action=review >>>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:353 >>>> + if (version->size() != 1 || version->at(0) != 0x00) >>> >>> We should have 0x00 as a constant as well. Will memcpy be more straightforward for readability? >> >> You probably mean memcmp. Problem with using that is if an empty Vector is (somehow) returned. If that happens, we'd be testing uninitialized memory. > > ... otherwise yes, after the size is validated we'd be able to call memcmp and compare the vector data against an array constant. I would prefer your second comment. >>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:410 >>> + if (parameters) { >> >> I wonder if this part of code ever gets executed. > > Probably not, I don't think there's any test cases covering it, in WebKit or web-platform-tests. Same I think for the optional `privateKey.publicKey` member. > > I've opened bug #174420 to add such a test. Awesome! Please enable them just for GTK+ now.

Comment on attachment 313518 [details] Patch Any plans on this patch?

(In reply to Jiewen Tan from comment #11) > Comment on attachment 313518 [details] > Patch > > Any plans on this patch? The constants patch is ready for review in bug #17409. I'm setting this bug to depend on it, after it lands this patch can be updated to properly test for specific constants. I'll also make this bug depend on bug #174420 so that the test case is provided first, and then this patch can simply unskip it for the GTK+ and WPE ports.

(In reply to Zan Dobersek from comment #12) > (In reply to Jiewen Tan from comment #11) > > Comment on attachment 313518 [details] > > Patch > > > > Any plans on this patch? > > The constants patch is ready for review in bug #17409. Bug #174091 actually.

Created attachment 316765 [details] Patch Corrected implementation after the test in bug #174420 revealed some issues. Still depends on #174091 to land, will be updated after that.

Created attachment 316845 [details] WIP

Created attachment 316884 [details] Patch

Comment on attachment 316884 [details] Patch Attachment 316884 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4239273 New failing tests: fast/dom/StyleSheet/detached-sheet-owner-node-link.html

Created attachment 316953 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 316884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316884&action=review > Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:468 > + // Retrieve the `q` point. Since only `d` is provided, libgcrypt will compute it on-the-fly. Isn't the public key provided in the previous step if any? Why is computation still needed for that case?

Comment on attachment 316884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316884&action=review >> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:468 >> + // Retrieve the `q` point. Since only `d` is provided, libgcrypt will compute it on-the-fly. > > Isn't the public key provided in the previous step if any? Why is computation still needed for that case? Pubic key is optional. If it's present and of valid form, it's set above, using the gcry_mpi_t object. Here it's retrieved into a gcry_mpi_point_t object, which is the type used to represent EC points, but it has essentially the same value as publicKeyMPI if that was provided, or libgcrypt will have to compute it from scratch for the specific EC curve. I can improve the comment upon landing, or upon next upload if there's anything more substantial to polish.

Comment on attachment 316884 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316884&action=review Looks good to me. r=me. >>> Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp:468 >>> + // Retrieve the `q` point. Since only `d` is provided, libgcrypt will compute it on-the-fly. >> >> Isn't the public key provided in the previous step if any? Why is computation still needed for that case? > > Pubic key is optional. If it's present and of valid form, it's set above, using the gcry_mpi_t object. Here it's retrieved into a gcry_mpi_point_t object, which is the type used to represent EC points, but it has essentially the same value as publicKeyMPI if that was provided, or libgcrypt will have to compute it from scratch for the specific EC curve. > > I can improve the comment upon landing, or upon next upload if there's anything more substantial to polish. Sure, please improve the comments.

Committed r220253: <http://trac.webkit.org/changeset/220253>

<rdar://problem/33717915>