[tarantool-patches] Re: [PATCH v2 2/3] crypto: make exported methods conform code style

[Konstantin Osipov <kostja@tarantool.org>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/04/29 14:09]:

Please solicit a code review from the owner of htis code, Georgy.

Thanks!
> Tarantool has a strict rule for naming methods of libraries - use
> the library name as a prefix. For crypto lib methods it should be
> 'crypto_', not 'tnt_'.
> ---
>  extra/exports           | 13 ++++++-------
>  src/lib/crypto/crypto.c | 31 +++++++++++++++++++++---------
>  src/lib/crypto/crypto.h |  6 +++++-
>  src/lua/crypto.lua      | 42 ++++++++++++++++++++---------------------
>  src/main.cc             |  3 +++
>  5 files changed, 56 insertions(+), 39 deletions(-)
> 
> diff --git a/extra/exports b/extra/exports
> index 21fa9bf00..6fabc4d92 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -77,13 +77,12 @@ space_run_triggers
>  space_bsize
>  box_schema_version
>  
> -tnt_openssl_init
> -tnt_EVP_CIPHER_key_length
> -tnt_EVP_CIPHER_iv_length
> -tnt_EVP_MD_CTX_new
> -tnt_EVP_MD_CTX_free
> -tnt_HMAC_CTX_new
> -tnt_HMAC_CTX_free
> +crypto_EVP_CIPHER_key_length
> +crypto_EVP_CIPHER_iv_length
> +crypto_EVP_MD_CTX_new
> +crypto_EVP_MD_CTX_free
> +crypto_HMAC_CTX_new
> +crypto_HMAC_CTX_free
>  
>  # Module API
>  
> diff --git a/src/lib/crypto/crypto.c b/src/lib/crypto/crypto.c
> index 28dbc71dd..4b192ba2d 100644
> --- a/src/lib/crypto/crypto.c
> +++ b/src/lib/crypto/crypto.c
> @@ -35,8 +35,8 @@
>  #include <openssl/ssl.h>
>  #include <openssl/hmac.h>
>  
> -int
> -tnt_openssl_init(void)
> +void
> +crypto_init(void)
>  {
>  #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)
>  	OpenSSL_add_all_digests();
> @@ -46,20 +46,30 @@ tnt_openssl_init(void)
>  	OPENSSL_init_crypto(0, NULL);
>  	OPENSSL_init_ssl(0, NULL);
>  #endif
> -	return 0;
>  }
>  
> -int tnt_EVP_CIPHER_key_length(const EVP_CIPHER *cipher)
> +void
> +crypto_free(void)
> +{
> +#ifdef OPENSSL_cleanup
> +	OPENSSL_cleanup();
> +#endif
> +}
> +
> +int
> +crypto_EVP_CIPHER_key_length(const EVP_CIPHER *cipher)
>  {
>  	return EVP_CIPHER_key_length(cipher);
>  }
>  
> -int tnt_EVP_CIPHER_iv_length(const EVP_CIPHER *cipher)
> +int
> +crypto_EVP_CIPHER_iv_length(const EVP_CIPHER *cipher)
>  {
>  	return EVP_CIPHER_iv_length(cipher);
>  }
>  
> -EVP_MD_CTX *tnt_EVP_MD_CTX_new(void)
> +EVP_MD_CTX *
> +crypto_EVP_MD_CTX_new(void)
>  {
>  #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)
>  	return EVP_MD_CTX_create();
> @@ -68,7 +78,8 @@ EVP_MD_CTX *tnt_EVP_MD_CTX_new(void)
>  #endif
>  };
>  
> -void tnt_EVP_MD_CTX_free(EVP_MD_CTX *ctx)
> +void
> +crypto_EVP_MD_CTX_free(EVP_MD_CTX *ctx)
>  {
>  #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)
>  	return EVP_MD_CTX_destroy(ctx);
> @@ -77,7 +88,8 @@ void tnt_EVP_MD_CTX_free(EVP_MD_CTX *ctx)
>  #endif
>  }
>  
> -HMAC_CTX *tnt_HMAC_CTX_new(void)
> +HMAC_CTX *
> +crypto_HMAC_CTX_new(void)
>  {
>  #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)
>  	HMAC_CTX *ctx = (HMAC_CTX *)OPENSSL_malloc(sizeof(HMAC_CTX));
> @@ -92,7 +104,8 @@ HMAC_CTX *tnt_HMAC_CTX_new(void)
>  
>  }
>  
> -void tnt_HMAC_CTX_free(HMAC_CTX *ctx)
> +void
> +crypto_HMAC_CTX_free(HMAC_CTX *ctx)
>  {
>  #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)
>  	HMAC_cleanup(ctx); /* Remove key from memory */
> diff --git a/src/lib/crypto/crypto.h b/src/lib/crypto/crypto.h
> index 9026db322..bc807e1a2 100644
> --- a/src/lib/crypto/crypto.h
> +++ b/src/lib/crypto/crypto.h
> @@ -35,7 +35,11 @@
>  extern "C" {
>  #endif
>  
> -int tnt_openssl_init();
> +void
> +crypto_init(void);
> +
> +void
> +crypto_free(void);
>  
>  #if defined(__cplusplus)
>  }
> diff --git a/src/lua/crypto.lua b/src/lua/crypto.lua
> index e76370517..30504b1de 100644
> --- a/src/lua/crypto.lua
> +++ b/src/lua/crypto.lua
> @@ -4,7 +4,6 @@ local ffi = require('ffi')
>  local buffer = require('buffer')
>  
>  ffi.cdef[[
> -    int tnt_openssl_init(void);
>      /* from openssl/err.h */
>      unsigned long ERR_get_error(void);
>      char *ERR_error_string(unsigned long e, char *buf);
> @@ -14,16 +13,16 @@ ffi.cdef[[
>  
>      typedef struct {} EVP_MD_CTX;
>      typedef struct {} EVP_MD;
> -    EVP_MD_CTX *tnt_EVP_MD_CTX_new(void);
> -    void tnt_EVP_MD_CTX_free(EVP_MD_CTX *ctx);
> +    EVP_MD_CTX *crypto_EVP_MD_CTX_new(void);
> +    void crypto_EVP_MD_CTX_free(EVP_MD_CTX *ctx);
>      int EVP_DigestInit_ex(EVP_MD_CTX *ctx, const EVP_MD *type, ENGINE *impl);
>      int EVP_DigestUpdate(EVP_MD_CTX *ctx, const void *d, size_t cnt);
>      int EVP_DigestFinal_ex(EVP_MD_CTX *ctx, unsigned char *md, unsigned int *s);
>      const EVP_MD *EVP_get_digestbyname(const char *name);
>  
>      typedef struct {} HMAC_CTX;
> -    HMAC_CTX *tnt_HMAC_CTX_new(void);
> -    void tnt_HMAC_CTX_free(HMAC_CTX *ctx);
> +    HMAC_CTX *crypto_HMAC_CTX_new(void);
> +    void crypto_HMAC_CTX_free(HMAC_CTX *ctx);
>      int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, int len,
>                   const EVP_MD *md, ENGINE *impl);
>      int HMAC_Update(HMAC_CTX *ctx, const unsigned char *data, size_t len);
> @@ -40,17 +39,14 @@ ffi.cdef[[
>      int EVP_CipherUpdate(EVP_CIPHER_CTX *ctx, unsigned char *out, int *outl,
>                       const unsigned char *in, int inl);
>      int EVP_CipherFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *out, int *outl);
> -    int EVP_CIPHER_CTX_cleanup(EVP_CIPHER_CTX *ctx);
>  
> -    int tnt_EVP_CIPHER_iv_length(const EVP_CIPHER *cipher);
> -    int tnt_EVP_CIPHER_key_length(const EVP_CIPHER *cipher);
> +    int crypto_EVP_CIPHER_iv_length(const EVP_CIPHER *cipher);
> +    int crypto_EVP_CIPHER_key_length(const EVP_CIPHER *cipher);
>  
>      int EVP_CIPHER_block_size(const EVP_CIPHER *cipher);
>      const EVP_CIPHER *EVP_get_cipherbyname(const char *name);
>  ]]
>  
> -ffi.C.tnt_openssl_init();
> -
>  local function openssl_err_str()
>    return ffi.string(ffi.C.ERR_error_string(ffi.C.ERR_get_error(), nil))
>  end
> @@ -70,11 +66,11 @@ end
>  local digest_mt = {}
>  
>  local function digest_gc(ctx)
> -    ffi.C.tnt_EVP_MD_CTX_free(ctx)
> +    ffi.C.crypto_EVP_MD_CTX_free(ctx)
>  end
>  
>  local function digest_new(digest)
> -    local ctx = ffi.C.tnt_EVP_MD_CTX_new()
> +    local ctx = ffi.C.crypto_EVP_MD_CTX_new()
>      if ctx == nil then
>          return error('Can\'t create digest ctx: ' .. openssl_err_str())
>      end
> @@ -121,7 +117,7 @@ local function digest_final(self)
>  end
>  
>  local function digest_free(self)
> -    ffi.C.tnt_EVP_MD_CTX_free(self.ctx)
> +    ffi.C.crypto_EVP_MD_CTX_free(self.ctx)
>      ffi.gc(self.ctx, nil)
>      self.ctx = nil
>      self.initialized = false
> @@ -141,14 +137,14 @@ local hmacs = digests
>  local hmac_mt = {}
>  
>  local function hmac_gc(ctx)
> -    ffi.C.tnt_HMAC_CTX_free(ctx)
> +    ffi.C.crypto_HMAC_CTX_free(ctx)
>  end
>  
>  local function hmac_new(digest, key)
>      if key == nil then
>          return error('Key should be specified for HMAC operations')
>      end
> -    local ctx = ffi.C.tnt_HMAC_CTX_new()
> +    local ctx = ffi.C.crypto_HMAC_CTX_new()
>      if ctx == nil then
>          return error('Can\'t create HMAC ctx: ' .. openssl_err_str())
>      end
> @@ -195,7 +191,7 @@ local function hmac_final(self)
>  end
>  
>  local function hmac_free(self)
> -    ffi.C.tnt_HMAC_CTX_free(self.ctx)
> +    ffi.C.crypto_HMAC_CTX_free(self.ctx)
>      ffi.gc(self.ctx, nil)
>      self.ctx = nil
>      self.initialized = false
> @@ -234,13 +230,15 @@ local function cipher_gc(ctx)
>  end
>  
>  local function cipher_new(cipher, key, iv, direction)
> -    if key == nil or key:len() ~= ffi.C.tnt_EVP_CIPHER_key_length(cipher) then
> -        return error('Key length should be equal to cipher key length ('
> -            .. tostring(ffi.C.tnt_EVP_CIPHER_key_length(cipher)) .. ' bytes)')
> +    local needed_len = ffi.C.crypto_EVP_CIPHER_key_length(cipher)
> +    if key == nil or key:len() ~= needed_len then
> +        return error('Key length should be equal to cipher key length ('..
> +                     tostring(needed_len)..' bytes)')
>      end
> -    if iv == nil or iv:len() ~= ffi.C.tnt_EVP_CIPHER_iv_length(cipher) then
> -        return error('Initial vector length should be equal to cipher iv length ('
> -            .. tostring(ffi.C.tnt_EVP_CIPHER_iv_length(cipher)) .. ' bytes)')
> +    needed_len = ffi.C.crypto_EVP_CIPHER_iv_length(cipher)
> +    if iv == nil or iv:len() ~= needed_len then
> +        return error('Initial vector length should be equal to cipher iv '..
> +                     'length ('..tostring(needed_len)..' bytes)')
>      end
>      local ctx = ffi.C.EVP_CIPHER_CTX_new()
>      if ctx == nil then
> diff --git a/src/main.cc b/src/main.cc
> index 5ded83223..606c64c13 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -75,6 +75,7 @@
>  #include "box/lua/init.h" /* box_lua_init() */
>  #include "box/session.h"
>  #include "systemd.h"
> +#include "crypto/crypto.h"
>  
>  static pid_t master_pid = getpid();
>  static struct pidfh *pid_file_handle;
> @@ -621,6 +622,7 @@ tarantool_free(void)
>  	memory_free();
>  	random_free();
>  #endif
> +	crypto_free();
>  	coll_free();
>  	systemd_free();
>  	say_logger_free();
> @@ -780,6 +782,7 @@ main(int argc, char **argv)
>  	signal_init();
>  	cbus_init();
>  	coll_init();
> +	crypto_init();
>  	tarantool_lua_init(tarantool_bin, main_argc, main_argv);
>  
>  	start_time = ev_monotonic_time();
> -- 
> 2.20.1 (Apple Git-117)
> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32