[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/6] util: string: Introduce virStringEncodeBase64




On 05/13/2016 12:04 PM, Peter Krempa wrote:
> Add a new helper that sanitizes error semantics of base64_encode_alloc.
> ---
>  src/conf/virsecretobj.c           |  7 ++-----
>  src/libvirt_private.syms          |  1 +
>  src/qemu/qemu_agent.c             |  6 ++----
>  src/storage/storage_backend_rbd.c | 17 ++++-------------
>  src/util/virstring.c              | 24 ++++++++++++++++++++++++
>  src/util/virstring.h              |  2 ++
>  tools/virsh-secret.c              |  6 +++---
>  7 files changed, 38 insertions(+), 25 deletions(-)
> 

probably means #include "base64.h" is no longer necessary in places
where the call has been removed.

> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index 721e0b4..4babd31 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -30,6 +30,7 @@
>  #include "virfile.h"
>  #include "virhash.h"
>  #include "virlog.h"
> +#include "virstring.h"
>  #include "base64.h"
> 
>  #define VIR_FROM_THIS VIR_FROM_SECRET
> @@ -730,12 +731,8 @@ virSecretObjSaveData(virSecretObjPtr secret)
>      if (!secret->value)
>          return 0;
> 
> -    base64_encode_alloc((const char *)secret->value, secret->value_size,
> -                        &base64);
> -    if (base64 == NULL) {
> -        virReportOOMError();
> +    if (!(base64 = virStringEncodeBase64(secret->value, secret->value_size)))


>          goto cleanup;
> -    }
> 
>      if (virFileRewrite(secret->base64File, S_IRUSR | S_IWUSR,
>                         virSecretRewriteFile, base64) < 0)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 9c1abbb..ca2b6b8 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2302,6 +2302,7 @@ virSkipSpacesBackwards;
>  virStrcpy;
>  virStrdup;
>  virStringArrayHasString;
> +virStringEncodeBase64;
>  virStringFreeList;
>  virStringFreeListCount;
>  virStringGetFirstWithPrefix;
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index c55f304..cbc0995 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -2142,11 +2142,9 @@ qemuAgentSetUserPassword(qemuAgentPtr mon,
>      virJSONValuePtr reply = NULL;
>      char *password64 = NULL;
> 
> -    base64_encode_alloc(password, strlen(password), &password64);
> -    if (!password64) {
> -        virReportOOMError();
> +    if (!(password64 = virStringEncodeBase64((unsigned char *) password,
> +                                             strlen(password))))
>          goto cleanup;
> -    }
> 
>      if (!(cmd = qemuAgentMakeCommand("guest-set-user-password",
>                                       "b:crypted", crypted,
> diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
> index 6e92ff7..ac46b9d 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -59,7 +59,7 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>      int r = 0;
>      virStorageAuthDefPtr authdef = source->auth;
>      unsigned char *secret_value = NULL;
> -    size_t secret_value_size;
> +    size_t secret_value_size = 0;
>      char *rados_key = NULL;
>      virBuffer mon_host = VIR_BUFFER_INITIALIZER;
>      virSecretPtr secret = NULL;
> @@ -129,15 +129,8 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>              goto cleanup;
>          }
> 
> -        base64_encode_alloc((char *)secret_value,
> -                            secret_value_size, &rados_key);
> -        memset(secret_value, 0, secret_value_size);
> -
> -        if (rados_key == NULL) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("failed to decode the RADOS key"));
> +        if (!(rados_key = virStringEncodeBase64(secret_value, secret_value_size)))
>              goto cleanup;
> -        }
> 
>          VIR_DEBUG("Found cephx key: %s", rados_key);
>          if (rados_conf_set(ptr->cluster, "key", rados_key) < 0) {
> @@ -147,8 +140,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>              goto cleanup;
>          }
> 
> -        memset(rados_key, 0, strlen(rados_key));
> -
>          if (rados_conf_set(ptr->cluster, "auth_supported", "cephx") < 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("failed to set RADOS option: %s"),
> @@ -233,8 +224,8 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>      ret = 0;
> 
>   cleanup:
> -    VIR_FREE(secret_value);
> -    VIR_FREE(rados_key);
> +    VIR_DISPOSE_N(secret_value, secret_value_size);
> +    VIR_DISPOSE_STRING(rados_key);
> 
>      virObjectUnref(secret);
> 
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index 735e65b..2702cec 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -25,6 +25,7 @@
>  #include <stdio.h>
>  #include <regex.h>
> 
> +#include "base64.h"
>  #include "c-ctype.h"
>  #include "virstring.h"
>  #include "viralloc.h"
> @@ -1066,3 +1067,26 @@ virStringIsPrintable(const char *str)
> 
>      return true;
>  }
> +
> +
> +/**
> + * virStringEncodeBase64:
> + * @buf: buffer of bytes to encode
> + * @buflen: number of bytes to encode
> + *
> + * Encodes @buf to base 64 and returns the resulting string. The caller is
> + * responsible for freeing the result.
> + */
> +char *
> +virStringEncodeBase64(const uint8_t *buf, size_t buflen)
> +{
> +    char *ret;
> +
> +    base64_encode_alloc((const char *) buf, buflen, &ret);
> +    if (!ret) {
> +        virReportOOMError();
> +        return NULL;
> +    }

Don't think the return NULL would be necessary...  just return ret

> +
> +    return ret;
> +}
> diff --git a/src/util/virstring.h b/src/util/virstring.h
> index fd2868a..6bc2726 100644
> --- a/src/util/virstring.h
> +++ b/src/util/virstring.h
> @@ -277,4 +277,6 @@ void virStringStripControlChars(char *str);
> 
>  bool virStringIsPrintable(const char *str);
> 
> +char *virStringEncodeBase64(const uint8_t *buf, size_t buflen);
> +
>  #endif /* __VIR_STRING_H__ */
> diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c
> index 087c2ed..532c4d5 100644
> --- a/tools/virsh-secret.c
> +++ b/tools/virsh-secret.c
> @@ -32,6 +32,7 @@
>  #include "viralloc.h"
>  #include "virfile.h"
>  #include "virutil.h"
> +#include "virstring.h"
>  #include "conf/secret_conf.h"
> 
>  static virSecretPtr
> @@ -265,9 +266,8 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd)
>      if (value == NULL)
>          goto cleanup;
> 
> -    base64_encode_alloc((char *)value, value_size, &base64);
> -    memset(value, 0, value_size);
> -    VIR_FREE(value);

Lost the VIR_FREE(value).... Could be VIR_DISPOSE(value) too.


John
> +    if (!(base64 = virStringEncodeBase64(value, value_size)))
> +        goto cleanup;
> 
>      if (base64 == NULL) {
>          vshError(ctl, "%s", _("Failed to allocate memory"));
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]