From: Date: March 9 2007 4:19pm Subject: patch review for Bug#13174, "SHA2 function" List-Archive: http://lists.mysql.com/internals/34381 Message-Id: <0941653C-BFA7-4C5E-AC82-F21CAC545FF4@mysql.com> MIME-Version: 1.0 (Apple Message framework v752.3) Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=pgp-sha1; boundary="Apple-Mail-16--274640022" Content-Transfer-Encoding: 7bit --Apple-Mail-16--274640022 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=US-ASCII; delsp=yes; format=flowed Hi Bill, all. This is my first review of a patch on "internals". I intend to review contributions publicly here from now on, so that readers and future submitters get a feel for our work. -- Against the 02/05/2007 12:30:49 patch, at http://www.karwin.com/ sha2.patch.gz: > diff -urN -x '*.Po' -x '*.Plo' -x Makefile -x mysqlbug -x > gen_lex_hash -x lex_hash.h mysql-5.0.33-clean/mysql-test/r/ > func_digest.result mysql-5.0.33/mysql-test/r/func_digest.result > --- mysql-5.0.33-clean/mysql-test/r/func_digest.result 1969-12-31 > 16:00:00.000000000 -0800 > +++ mysql-5.0.33/mysql-test/r/func_digest.result 2007-01-21 > 23:04:02.000000000 -0800 > @@ -0,0 +1,1344 @@ > +SELECT SHA1( x'a8' ) AS SHA1; > +SHA1 > +99f2aa95e36f95c2acb0eaf23998f030638f3f15 > +SELECT SHA1( x'3000' ) AS SHA1; > +SHA1 > +f944dcd635f9801f7ac90a407fbc479964dec024 > +SELECT SHA1( x'42749e' ) AS SHA1; > +SHA1 > +a444319e9b6cc1e8464c511ec0969c37d6bb2619 [...] > +SELECT SHA2 > ( x'a8f17ae01bd749341b1cea3f73ef22c3a84e4ac38276f497c79ed23f4ae4d4f824 > 88910d4258e8f0206be789d3', 224 ) AS SHA224; > +SHA224 > +026f1a48b71cdddd0335fc736cc9f6877bd67dc31e43f87cc04efb32 > +SELECT SHA2 > ( x'32d5e868b0b48239a6d74799ae8046f4d98faa564f9b80233541c108d4bc8c31f7 > b6b3bc945e796eafbb9aa57c408c8ec4a0059889e4bd29edba758f50103839b9e1e127 > 774fdd9f08f332223971c09e8f4fc7e5e607aa5e585af3fa60e896', 384 ) AS > SHA384; > +SHA384 > +01817497f65899ff7dbd32c4a1c34fb0f6d1c9dc426cdaf6bc42d14c68a6e610aac9f > cc47c02076162f1510777a416a5 [...] Yes, good. > diff -urN -x '*.Po' -x '*.Plo' -x Makefile -x mysqlbug -x > gen_lex_hash -x lex_hash.h mysql-5.0.33-clean/mysql-test/t/ > func_digest.test mysql-5.0.33/mysql-test/t/func_digest.test > --- mysql-5.0.33-clean/mysql-test/t/func_digest.test 1969-12-31 > 16:00:00.000000000 -0800 > +++ mysql-5.0.33/mysql-test/t/func_digest.test 2007-01-21 > 19:53:23.000000000 -0800 > @@ -0,0 +1,461 @@ > + > +-- source include/have_openssl.inc > + > +# These test data are from the NIST SHA Test Vectors for Hashing > +# Byte-Oriented Messages. See http://csrc.nist.gov/cryptval/shs.htm > +# Only the "ShortMsg" test data are used here. > +# Values of x'00' in the test data have been excluded; it is not > clear > +# how to specify a length of 8 bits for a binary value like that. > + > +SELECT SHA1( x'a8' ) AS SHA1; > +SELECT SHA1( x'3000' ) AS SHA1; > +SELECT SHA1( x'42749e' ) AS SHA1; > +SELECT SHA1( x'9fc3fe08' ) AS SHA1; > +SELECT SHA1( x'b5c1c6f1af' ) AS SHA1; [...] > +SELECT SHA2( x'6e5ecbbc6f36ec985c253f7e4bb6b1f8ab6c4942', 224 ) AS > SHA224; > +SELECT SHA2( x'393d8ef3671232dc6efcb3d6426fc88f730e946b5d', 224 ) > AS SHA224; > +SELECT SHA2( x'f222e611d99b3728b5e308f9b9b637b9d493528c2865', > 224 ) AS SHA224; > +SELECT SHA2( x'16cd2320dd785b07b681c86ad39e56549ee4d71aa9e69e', > 224 ) AS SHA224; > +SELECT SHA2( x'ceef92454528483f45a6992d179abff266145f2da2d10eb1', > 224 ) AS SHA224; [...] > +SELECT SHA2( x'bd', 256 ) AS SHA256; > +SELECT SHA2( x'5fd4', 256 ) AS SHA256; This are great results, but you test only that it works for literal strings. Make sure that SELECT SHA2(chardata, intdata) FROM exampletable; works. Show that hash lengths that aren't one of those few values do what you expect. Even include negative numbers to show that the server is stable. Include other character field types and use some "multibyte" charsets. Give it NULLs for both parameters. Give it functions as parameters SHA2(SHA2(SHA2(x'foo', 256), 256), 256) and SHA2(SHA2(SHA2(NULL, 256), 256), 256) . Give it a subselect as a parameter. Prove in the tests that all the marginal cases and edge cases work as one would expect. Add every permutation you can think of. > diff -urN -x '*.Po' -x '*.Plo' -x Makefile -x mysqlbug -x > gen_lex_hash -x lex_hash.h mysql-5.0.33-clean/sql/item_create.cc > mysql-5.0.33/sql/item_create.cc > --- mysql-5.0.33-clean/sql/item_create.cc 2007-01-09 > 04:51:08.000000000 -0800 > +++ mysql-5.0.33/sql/item_create.cc 2007-01-21 19:46:16.000000000 > -0800 > @@ -347,6 +347,11 @@ > return new Item_func_sha(a); > } > +Item *create_func_sha2(Item* a, Item* b) > +{ > + return new Item_func_sha2(a, b); > +} > + > Item *create_func_sleep(Item* a) > { > current_thd->lex->uncacheable(UNCACHEABLE_SIDEEFFECT); > diff -urN -x '*.Po' -x '*.Plo' -x Makefile -x mysqlbug -x > gen_lex_hash -x lex_hash.h mysql-5.0.33-clean/sql/item_create.h > mysql-5.0.33/sql/item_create.h > --- mysql-5.0.33-clean/sql/item_create.h 2007-01-09 > 04:51:06.000000000 -0800 > +++ mysql-5.0.33/sql/item_create.h 2007-01-21 19:46:34.000000000 -0800 > @@ -83,6 +83,7 @@ > Item *create_func_sign(Item* a); > Item *create_func_sin(Item* a); > Item *create_func_sha(Item* a); > +Item *create_func_sha2(Item* a, Item* b); > Item *create_func_sleep(Item* a); > Item *create_func_soundex(Item* a); > Item *create_func_space(Item *); > diff -urN -x '*.Po' -x '*.Plo' -x Makefile -x mysqlbug -x > gen_lex_hash -x lex_hash.h mysql-5.0.33-clean/sql/item_strfunc.cc > mysql-5.0.33/sql/item_strfunc.cc > --- mysql-5.0.33-clean/sql/item_strfunc.cc 2007-01-09 > 04:51:35.000000000 -0800 > +++ mysql-5.0.33/sql/item_strfunc.cc 2007-01-21 20:03:18.000000000 > -0800 > @@ -28,6 +28,7 @@ > #include > #ifdef HAVE_OPENSSL > #include > +#include > #endif /* HAVE_OPENSSL */ Good. > #include "md5.h" > #include "sha1.h" > @@ -230,6 +231,135 @@ > MY_CS_BINSORT,MYF(0)), > DERIVATION_COERCIBLE); > } > +String *Item_func_sha2::val_str(String *str) No space between functions? Also, for all new functions, add a Doxygen-style comment that describes it. At least have a brief statement that tells us what it is and the parameters. > +{ > + DBUG_ASSERT(fixed == 1); > +#ifdef HAVE_OPENSSL Wait, OPENSSL might not be the only source of these routines, and we have our own (GPL compatible) SSL implementation that defines HAVE_OPENSSL [ :( ] and yet doesn't implement SHA-2. Test with configure flags "--without-openssl --with-yassl" . Instead of testing that we HAVE_OPENSSL here, test that we HAVE_SHA2 and add a test in the configure.in script for it. Don't test for a container that you expect to have something; instead test for functionality. > + static unsigned char digest_buf[SHA512_DIGEST_LENGTH]; > + uint i; > + char *p; > + String *input_string; > + unsigned char *input_ptr; > + size_t input_len; > + uint digest_length= 0; > + > + if ((null_value= args[0]->null_value)) > + { > + return (String *) 0; > + } > + input_string= args[0]->val_str(str); > + if (!input_string) > + { > + null_value= 1; > + return (String *) 0; > + } > + input_ptr= (unsigned char *) input_string->ptr(); > + input_len= (size_t) input_string->length(); > + > + switch ((uint) args[1]->val_int()) { > +#ifndef OPENSSL_NO_SHA512 > + case 512: > + digest_length = SHA512_DIGEST_LENGTH; > + (void) SHA512(input_ptr, input_len, digest_buf); Excellent use of (void) here. Cheers! > + break; > + case 384: > + digest_length = SHA384_DIGEST_LENGTH; > + (void) SHA384(input_ptr, input_len, digest_buf); > + break; > +#endif > +#ifndef OPENSSL_NO_SHA256 Instead, perhaps test that SHA224_DIGEST_LENGTH is defined? Just a suggestion. I don't know openssl standards for that. > + case 224: > + digest_length = SHA224_DIGEST_LENGTH; > + (void) SHA224(input_ptr, input_len, digest_buf); > + break; > + case 256: // SHA-256 is the default > + case 0: > + digest_length = SHA256_DIGEST_LENGTH; > + (void) SHA256(input_ptr, input_len, digest_buf); > + break; > +#endif > + default: > + push_warning_printf(current_thd, > + MYSQL_ERROR::WARN_LEVEL_ERROR, > + ER_WRONG_PARAMETERS_TO_PROCEDURE, > + ER(ER_WRONG_PARAMETERS_TO_PROCEDURE), > + "sha2", "--with-openssl"); > + null_value= 1; > + return (String *) 0; > + } > + > + for (i= 0, p= (char *) str->ptr(); i < digest_length; ++i, p+=2) > + { > + sprintf(p, "%02x", digest_buf[i]); > + } > + str->length((uint) digest_length*2); > + null_value= 0; > + return str; > + > +#else > + push_warning_printf(current_thd, > + MYSQL_ERROR::WARN_LEVEL_ERROR, > + ER_FEATURE_DISABLED, > + ER(ER_FEATURE_DISABLED), > + "sha2", "--with-openssl"); > + null_value= 1; > + return (String *) 0; > +#endif > +} > + > + > +void Item_func_sha2::fix_length_and_dec() > +{ > + maybe_null = 1; > + max_length = 0; > + > +#ifdef HAVE_OPENSSL > + switch ((uint) args[1]->val_int()) { > +#ifndef OPENSSL_NO_SHA512 > + case 512: > + max_length= SHA512_DIGEST_LENGTH*2; n * 2 because of the hexadecimal encoding of bytes? Comment that please. > + break; > + case 384: > + max_length= SHA384_DIGEST_LENGTH*2; > + break; > +#endif > +#ifndef OPENSSL_NO_SHA256 > + case 256: > + case 0: // SHA-256 is the default > + max_length= SHA256_DIGEST_LENGTH*2; > + break; > + case 224: > + max_length= SHA224_DIGEST_LENGTH*2; > + break; > +#endif > + default: > + push_warning_printf(current_thd, > + MYSQL_ERROR::WARN_LEVEL_ERROR, > + ER_WRONG_PARAMETERS_TO_PROCEDURE, > + ER(ER_WRONG_PARAMETERS_TO_PROCEDURE), > + "sha2", "--with-openssl"); > + } > + > + /* > + The SHA2() function treats its parameter as being a case > sensitive. > + Thus we set binary collation on it so different instances of > SHA2() > + will be compared properly. > + */ > + args[0]->collation.set( > + get_charset_by_csname( > + args[0]->collation.collation->csname, > + MY_CS_BINSORT, > + MYF(0)), > + DERIVATION_COERCIBLE); > +#else > + push_warning_printf(current_thd, > + MYSQL_ERROR::WARN_LEVEL_ERROR, > + ER_FEATURE_DISABLED, > + ER(ER_FEATURE_DISABLED), > + "sha2", "--with-openssl"); > +#endif > +} > + > /* Implementation of AES encryption routines */ > diff -urN -x '*.Po' -x '*.Plo' -x Makefile -x mysqlbug -x > gen_lex_hash -x lex_hash.h mysql-5.0.33-clean/sql/item_strfunc.h > mysql-5.0.33/sql/item_strfunc.h > --- mysql-5.0.33-clean/sql/item_strfunc.h 2007-01-09 > 04:51:08.000000000 -0800 > +++ mysql-5.0.33/sql/item_strfunc.h 2007-01-21 19:50:55.000000000 > -0800 > @@ -66,6 +66,19 @@ > const char *func_name() const { return "sha"; } > }; > +class Item_func_sha2 :public Item_str_func > +{ > +public: > + Item_func_sha2(Item *a, Item *b) :Item_str_func(a, b) I don't know if this is a documented convention, but we like the superclass on a separate line, leading with the colon. Not a big deal, in any case. > + { > + collation.set(&my_charset_bin); > + } > + String *val_str(String *); > + void fix_length_and_dec(); > + const char *func_name() const { return "sha2"; } > + bool check_partition_func_processor(byte *bool_arg) { return 0; } > +}; > + > class Item_func_aes_encrypt :public Item_str_func > { > public: > diff -urN -x '*.Po' -x '*.Plo' -x Makefile -x mysqlbug -x > gen_lex_hash -x lex_hash.h mysql-5.0.33-clean/sql/lex.h > mysql-5.0.33/sql/lex.h > --- mysql-5.0.33-clean/sql/lex.h 2007-01-09 04:51:49.000000000 -0800 > +++ mysql-5.0.33/sql/lex.h 2007-01-21 22:59:04.000000000 -0800 > @@ -730,6 +730,7 @@ > { "SIN", F_SYM(FUNC_ARG1),0,CREATE_FUNC(create_func_sin)}, > { "SHA", F_SYM(FUNC_ARG1),0,CREATE_FUNC > (create_func_sha)}, > { "SHA1", F_SYM(FUNC_ARG1),0,CREATE_FUNC > (create_func_sha)}, > + { "SHA2", F_SYM(FUNC_ARG2),0,CREATE_FUNC > (create_func_sha2)}, > { "SLEEP", F_SYM(FUNC_ARG1),0,CREATE_FUNC > (create_func_sleep)}, > { "SOUNDEX", F_SYM(FUNC_ARG1),0,CREATE_FUNC(create_func_soundex)}, > { "SPACE", F_SYM(FUNC_ARG1),0,CREATE_FUNC(create_func_space)}, EOF. I'd like to see this go in. Please fix these and resubmit it. - chad -- Chad Miller, Software Developer chad@stripped MySQL Inc., www.mysql.com Orlando, Florida, USA 13-20z, UTC-0500 Office: +1 408 213 6740 sip:6740@stripped --Apple-Mail-16--274640022 content-type: application/pgp-signature; x-mac-type=70674453; name=PGP.sig content-description: This is a digitally signed message part content-disposition: inline; filename=PGP.sig content-transfer-encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (Darwin) iD8DBQFF8XsE/peCpMTxrLsRApgAAJ90qZGmf04kPGuuS3hjf23W06MRwwCfWV5z ooQf4wSuvyt//CgdMqd4oK0= =BrHs -----END PGP SIGNATURE----- --Apple-Mail-16--274640022--