patch review for Bug#13174, "SHA2 function"

[Chad MILLER]

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 <m_ctype.h>
> #ifdef HAVE_OPENSSL
> #include <openssl/des.h>
> +#include <openssl/sha.h>
> #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: [phone]                         sip:6740@stripped



Attachment: [application/pgp-signature] This is a digitally signed message part PGP.sig