maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Julius!

Just a couple of comments, see below:

On Dec 04, Julius Goryavsky wrote:
> revision-id: 387839e44cc (mariadb-10.4.10-179-g387839e44cc)
> parent(s): 8115a02283d
> author: Julius Goryavsky <julius.goryavsky@xxxxxxxxxxx>
> committer: Julius Goryavsky <julius.goryavsky@xxxxxxxxxxx>
> timestamp: 2019-11-15 13:37:35 +0100
> message:
> 
> Plugin for Hashicorp Vault Key Management System


> diff --git a/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc
> new file mode 100644
> index 00000000000..32c0e6417a3
> --- /dev/null
> +++ b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc
...
> +  if (tolen_len > max_token_size)
> +  {
> +    my_printf_error(ER_UNKNOWN_ERROR,
> +                    "Maximum allowed token length exceeded",
> +                    0);
> +    return true;
> +  }
> +  size_t buf_len = x_vault_token_len + tolen_len + 1;

typo: "token_len" I think

> +#ifdef _WIN32
> +  char *token_header = (char *) _alloca(sizeof(char) * buf_len);
> +#else
> +  char *token_header = (char *) alloca(sizeof(char) * buf_len);
> +#endif

Please, remove all these ifdefs and add one at the very top of the file:

  #ifdef _WIN32
  #define alloca _alloca
  #endif

Here I presume that you've tested it in buildbot and alloca is indeed
present on all builders and we don't need a fallback here.

Also remove sizeof(char) everywhere, it's always 1.  C99 standard says

  6.5.3.4 The sizeof operator
  ...
  When applied to an operand that has type char, unsigned char, or signed
  char, (or a qualified version thereof) the result is 1.

> +  snprintf(token_header, buf_len, "%s%s", x_vault_token, token);
> +  curl_errbuf[0] = '\0';
> +  if ((list= curl_slist_append(list, token_header)) == NULL ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errbuf)) !=
> +          CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION,
> +                                  write_response_memory)) != CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEDATA,
> +                                  &read_data_stream)) !=
> +          CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_HTTPHEADER, list)) !=
> +          CURLE_OK ||
...
> +  /*
> +    If the result of the conversion does not fit into an
> +    unsigned integer or if an error (for example, overflow)
> +    occurred during the conversion, then we need to return
> +    an error:
> +  */
> +#if ULONG_MAX > UINT_MAX
> +  if (version > UINT_MAX || errno)
> +#else
> +  if (errno)
> +#endif

did you verify that otherwise it makes compilation to fail?
better to keep the number of ifdefs in the code to a minimum, so don't
add this one unless absolutely necessary

> +  {
> +    return ENCRYPTION_KEY_VERSION_INVALID;
> +  }
> +  return (unsigned int) version;
> +}

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx