EVP_PKEY_asn1_{new,copy}(): pointless zeroing + a tiny bug

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

EVP_PKEY_asn1_{new,copy}(): pointless zeroing + a tiny bug

Theo Buehler-4
Very minor polishing, really:

Since we converted EVP_PKEY_asn1_new() to using calloc(), there's no
need to set the structure to zero manually with an odd spelling of NULL.
Besides, the list of zeroed members is incomplete: it misses sig_print.

Copy-pasting this list is likely the reason why EVP_PKEY_asn1_copy()
forgets to copy that member. Luckily, this copy function is rarely used
and the omitted callback is only used once (in X509_signature_print()).

Index: lib/libcrypto/asn1/ameth_lib.c
===================================================================
RCS file: /var/cvs/src/lib/libcrypto/asn1/ameth_lib.c,v
retrieving revision 1.17
diff -u -p -r1.17 ameth_lib.c
--- lib/libcrypto/asn1/ameth_lib.c 13 May 2018 06:40:55 -0000 1.17
+++ lib/libcrypto/asn1/ameth_lib.c 23 May 2018 06:42:11 -0000
@@ -309,59 +309,26 @@ EVP_PKEY_asn1_new(int id, int flags, con
 {
  EVP_PKEY_ASN1_METHOD *ameth;
 
- ameth = calloc(1, sizeof(EVP_PKEY_ASN1_METHOD));
- if (!ameth)
+ if ((ameth = calloc(1, sizeof(EVP_PKEY_ASN1_METHOD))) == NULL)
  return NULL;
 
  ameth->pkey_id = id;
  ameth->pkey_base_id = id;
  ameth->pkey_flags = flags | ASN1_PKEY_DYNAMIC;
 
- if (info) {
- ameth->info = strdup(info);
- if (!ameth->info)
+ if (info != NULL) {
+ if ((ameth->info = strdup(info)) == NULL)
  goto err;
- } else
- ameth->info = NULL;
+ }
 
- if (pem_str) {
- ameth->pem_str = strdup(pem_str);
- if (!ameth->pem_str)
+ if (pem_str != NULL) {
+ if ((ameth->pem_str = strdup(pem_str)) == NULL)
  goto err;
- } else
- ameth->pem_str = NULL;
-
- ameth->pub_decode = 0;
- ameth->pub_encode = 0;
- ameth->pub_cmp = 0;
- ameth->pub_print = 0;
-
- ameth->priv_decode = 0;
- ameth->priv_encode = 0;
- ameth->priv_print = 0;
-
- ameth->old_priv_encode = 0;
- ameth->old_priv_decode = 0;
-
- ameth->item_verify = 0;
- ameth->item_sign = 0;
-
- ameth->pkey_size = 0;
- ameth->pkey_bits = 0;
-
- ameth->param_decode = 0;
- ameth->param_encode = 0;
- ameth->param_missing = 0;
- ameth->param_copy = 0;
- ameth->param_cmp = 0;
- ameth->param_print = 0;
-
- ameth->pkey_free = 0;
- ameth->pkey_ctrl = 0;
+ }
 
  return ameth;
 
-err:
+ err:
  EVP_PKEY_asn1_free(ameth);
  return NULL;
 }
@@ -390,6 +357,7 @@ EVP_PKEY_asn1_copy(EVP_PKEY_ASN1_METHOD
  dst->param_copy = src->param_copy;
  dst->param_cmp = src->param_cmp;
  dst->param_print = src->param_print;
+ dst->sig_print = src->sig_print;
 
  dst->pkey_free = src->pkey_free;
  dst->pkey_ctrl = src->pkey_ctrl;

Reply | Threaded
Open this post in threaded view
|

Re: EVP_PKEY_asn1_{new,copy}(): pointless zeroing + a tiny bug

Brent Cook
ok bcook@

On Wed, May 23, 2018 at 2:11 AM, Theo Buehler <[hidden email]> wrote:

> Very minor polishing, really:
>
> Since we converted EVP_PKEY_asn1_new() to using calloc(), there's no
> need to set the structure to zero manually with an odd spelling of NULL.
> Besides, the list of zeroed members is incomplete: it misses sig_print.
>
> Copy-pasting this list is likely the reason why EVP_PKEY_asn1_copy()
> forgets to copy that member. Luckily, this copy function is rarely used
> and the omitted callback is only used once (in X509_signature_print()).
>
> Index: lib/libcrypto/asn1/ameth_lib.c
> ===================================================================
> RCS file: /var/cvs/src/lib/libcrypto/asn1/ameth_lib.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 ameth_lib.c
> --- lib/libcrypto/asn1/ameth_lib.c      13 May 2018 06:40:55 -0000
> 1.17
> +++ lib/libcrypto/asn1/ameth_lib.c      23 May 2018 06:42:11 -0000
> @@ -309,59 +309,26 @@ EVP_PKEY_asn1_new(int id, int flags, con
>  {
>         EVP_PKEY_ASN1_METHOD *ameth;
>
> -       ameth = calloc(1, sizeof(EVP_PKEY_ASN1_METHOD));
> -       if (!ameth)
> +       if ((ameth = calloc(1, sizeof(EVP_PKEY_ASN1_METHOD))) == NULL)
>                 return NULL;
>
>         ameth->pkey_id = id;
>         ameth->pkey_base_id = id;
>         ameth->pkey_flags = flags | ASN1_PKEY_DYNAMIC;
>
> -       if (info) {
> -               ameth->info = strdup(info);
> -               if (!ameth->info)
> +       if (info != NULL) {
> +               if ((ameth->info = strdup(info)) == NULL)
>                         goto err;
> -       } else
> -               ameth->info = NULL;
> +       }
>
> -       if (pem_str) {
> -               ameth->pem_str = strdup(pem_str);
> -               if (!ameth->pem_str)
> +       if (pem_str != NULL) {
> +               if ((ameth->pem_str = strdup(pem_str)) == NULL)
>                         goto err;
> -       } else
> -               ameth->pem_str = NULL;
> -
> -       ameth->pub_decode = 0;
> -       ameth->pub_encode = 0;
> -       ameth->pub_cmp = 0;
> -       ameth->pub_print = 0;
> -
> -       ameth->priv_decode = 0;
> -       ameth->priv_encode = 0;
> -       ameth->priv_print = 0;
> -
> -       ameth->old_priv_encode = 0;
> -       ameth->old_priv_decode = 0;
> -
> -       ameth->item_verify = 0;
> -       ameth->item_sign = 0;
> -
> -       ameth->pkey_size = 0;
> -       ameth->pkey_bits = 0;
> -
> -       ameth->param_decode = 0;
> -       ameth->param_encode = 0;
> -       ameth->param_missing = 0;
> -       ameth->param_copy = 0;
> -       ameth->param_cmp = 0;
> -       ameth->param_print = 0;
> -
> -       ameth->pkey_free = 0;
> -       ameth->pkey_ctrl = 0;
> +       }
>
>         return ameth;
>
> -err:
> + err:
>         EVP_PKEY_asn1_free(ameth);
>         return NULL;
>  }
> @@ -390,6 +357,7 @@ EVP_PKEY_asn1_copy(EVP_PKEY_ASN1_METHOD
>         dst->param_copy = src->param_copy;
>         dst->param_cmp = src->param_cmp;
>         dst->param_print = src->param_print;
> +       dst->sig_print = src->sig_print;
>
>         dst->pkey_free = src->pkey_free;
>         dst->pkey_ctrl = src->pkey_ctrl;
>