On 11/27/2015 09:30 AM, Daniel P. Berrange wrote: > Introduce a new QCryptoSecret object class which will be used > for providing passwords and keys to other objects which need > sensitive credentials. >
> More examples are shown in the updated docs. > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > --- > +++ b/crypto/secret.c > +static void > +qcrypto_secret_load_data(QCryptoSecret *secret, > + uint8_t **output, > + size_t *outputlen, > + Error **errp) > +{ > + if (!g_file_get_contents(secret->file, &data, &length, &gerr)) { > + error_setg(errp, > + "Unable to read %s: %s", > + secret->file, gerr->message); > + g_error_free(gerr); > + return; > + } > + if (length) { > + /* Even though data is raw 8-bit, so may contain > + * arbitrary NULs, ensure it is explicitly NUL > + * terminated */ > + *output = g_renew(uint8_t, data, length + 1); > + (*output)[length] = '\0'; These two lines are dead code. g_file_get_contents() guarantees that on success, contents is malloc'd large enough and that contents[length] == 0. https://developer.gnome.org/glib/stable/glib-File-Utilities.html#g-file-get-contents > + *outputlen = length; > + } else { > + error_setg(errp, "Secret file %s is empty", > + secret->file); Is there any technical reason why we must forbid a 0-length password? (Sometimes, having the empty string as a password can be a useful for development tests). I'm not opposed to rejecting it, especially if doing so now avoids a more cryptic error message later because there is indeed a technical reason; but just want to make sure it is not an arbitrary limitation. > + g_free(data); > + } > + } else if (secret->data) { > + *outputlen = strlen(secret->data); > + *output = g_new(uint8_t, *outputlen + 1); > + memcpy(*output, secret->data, *outputlen + 1); These two lines could be shortened to: *output = g_strdup(secret->data); > + > +static void qcrypto_secret_decrypt(QCryptoSecret *secret, > + const uint8_t *input, > + size_t inputlen, > + uint8_t **output, > + size_t *outputlen, > + Error **errp) > +{ > + if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) { > + ciphertext = qbase64_decode((const gchar*)input, > + inputlen, > + &ciphertextlen, > + errp); > + if (!ciphertext) { > + goto cleanup; > + } > + plaintext = g_new0(uint8_t, ciphertextlen + 1); > + } else { > + ciphertextlen = inputlen; > + plaintext = g_new0(uint8_t, inputlen + 1); g_new0(uint8_t, value) is the same as g_malloc0(value); I don't know if it is worth the distinction. But not worth a respin on its own. I found some style or efficiency things you might want to touch up, but no actual bugs that would prevent this patch from being usable as-is. Reviewed-by: Eric Blake <ebl...@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature