Re: [Qemu-devel] [PATCH v3 5/5] crypto: support multiple threads accessing one QCryptoBlock

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Dec 10, 2018 at 04:09:31PM +0100, Alberto Garcia wrote:
> On Mon 10 Dec 2018 03:52:03 PM CET, Daniel P. Berrangé wrote:
> >> > +int qcrypto_block_init_cipher(QCryptoBlock *block,
> >> > +                              QCryptoCipherAlgorithm alg,
> >> > +                              QCryptoCipherMode mode,
> >> > +                              const uint8_t *key, size_t nkey,
> >> > +                              size_t n_threads, Error **errp)
> >> > +{
> >> > +    size_t i;
> >> > +
> >> > +    assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
> >> > +
> >> > +    block->ciphers = g_new0(QCryptoCipher *, n_threads);
> >> 
> >> You can use g_new() instead of g_new0() because you're anyway
> >> overwriting all elements of the array.
> >
> > I'd rather have it initialized to zero upfront, so if creating any
> > cipher in the array fails, we don't have uninitialized array elements
> > during later cleanup code.
> 
> But it is the value of block->n_ciphers that determines the size of the
> array, and that is only incremented after each successful iteration:
> 
>   for (i = 0; i < n_threads; i++) {
>       block->ciphers[i] = qcrypto_cipher_new(alg, mode, key, nkey, errp);
>       /* ... error handling ... */
>       block->n_ciphers++;
>       block->n_free_ciphers++;
>   }
> 
> In other words, the cleanup code won't touch uninitialized elements
> because it cannot even tell the difference between an index that points
> to an uninitialized element of the array and an index that points beyond
> the allocated memory.

.../correctly written/ cleanup code won't touch uninitialized elements...

I prefer to see the code be hardened against mistakes and so would always
zero-initialize everything unless there's compelling performance reason
not to in certain specific places.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|