On 02.07.19 21:13, Peter Maydell wrote:
> On Mon, 24 Jun 2019 at 15:48, Max Reitz <mre...@redhat.com> wrote:
>>
>> From: Sam Eiderman <shmuel.eider...@oracle.com>
>>
>> Until ESXi 6.5 VMware used the vmfsSparse format for snapshots (VMDK3 in
>> QEMU).
>>
>> This format was lacking in the following:
>>
>> * Grain directory (L1) and grain table (L2) entries were 32-bit,
>> allowing access to only 2TB (slightly less) of data.
>> * The grain size (default) was 512 bytes - leading to data
>> fragmentation and many grain tables.
>> * For space reclamation purposes, it was necessary to find all the
>> grains which are not pointed to by any grain table - so a reverse
>> mapping of "offset of grain in vmdk" to "grain table" must be
>> constructed - which takes large amounts of CPU/RAM.
>>
>
> Hi; this change has confused Coverity a bit (CID 1402786):
>
>> @@ -481,7 +529,7 @@ static int vmdk_init_tables(BlockDriverState *bs,
>> VmdkExtent *extent,
>> int i;
>>
>> /* read the L1 table */
>> - l1_size = extent->l1_size * sizeof(uint32_t);
>> + l1_size = extent->l1_size * extent->entry_size;
>> extent->l1_table = g_try_malloc(l1_size);
>> if (l1_size && extent->l1_table == NULL) {
>> return -ENOMEM;
>
>> }
>>
>> if (extent->l1_backup_table_offset) {
>> + assert(!extent->sesparse);
>> extent->l1_backup_table = g_try_malloc(l1_size);
>> if (l1_size && extent->l1_backup_table == NULL) {
>> ret = -ENOMEM;
>
> Here we allocate extent->l1_backup_table via g_try_malloc(),
> and our "ENOMEM" guard checks l1_size. However later on we do:
>
> for (i = 0; i < extent->l1_size; i++) {
> le32_to_cpus(&extent->l1_backup_table[i]);
> }
>
> which is a dereference of l1_backup_table whose guard
> is looking at extent->l1_size. Previously Coverity could
> (probably) see this was OK because we were setting
> l1_size = extent->l1_size * sizeof(uint32_t)
> so l1_size is 0 if and only if extent->l1_size is zero.
> But now there's another possibility because we have changed
> the initialization to
> l1_size = extent->l1_size * extent->entry_size;
> so if extent->entry_size is zero then l1_size could be 0
> (getting us past the ENOMEM check) but extent->l1_size
> non-zero (causing us to try to dereference a NULL l1_backup_table
> pointer).
>
> This can't in fact happen because if extent->l1_size is
> non-zero we'll assert that extent->entry_size is
> either 8 or 4, but the logic has become sufficiently
> convoluted that it's no longer clear to Coverity that this
> is an impossible case. I could mark this as a false positive,
> but maybe there's a way of rephrasing this code that makes
> the logic a little clearer to both humans and robots?
There is also the fact that entry_size is initialized to 4
(sizeof(uint32_t)) and updated to 8 (sizeof(uint64_t)) when the image is
in the seSparse subformat. So entry_size can only ever be 4 or 8.
The only substantial thing I could imagine that might in some way make
it clearer would be to drop entry_size and always decide based on
extent->sesparse (because extent->entry_size == (extent->sesparse ? 8 : 4)).
But to me personally, that would not really be clearer. I think it’s
reasonable to store the size of L1 entries in an extent attribute, and
remember that it’s always 4 or 8.
Of course, there is also the less substantial thing, which is adding a
comment “Size of L1 entries, can only be sizeof(uint32_t) or
sizeof(uint64_t)” above the entry_size definition.
I think Coverity is actually confused by aliasing. The report says this:
> 1. Condition l1_size, taking false branch.
So l1_size must be 0, which means either extent->l1_size or
extent->entry_size must be 0.
It then says:
> 3. Condition i < extent->l1_size, taking true branch.
So extent->l1_size must be greater than 0. (To be exact, it prints this
two times, so extent->l1_size must be 2.)
This is followed by:
> 4. Condition extent->entry_size == 8UL /* sizeof (uint64_t) */, taking true
> branch.
Er, so, extent->entry_size is 8?
That actually makes l1_size 16. So somewhere between conditions 1 and
3/4, *extent must have changed its contents.
So it looks to me like Coverity just thinks that *extent may be used
concurrently. Short of adding a “restrict”, I don’t know what to do
this but to close the report as a false positive.
> Supplementary:
>
> (1) this code:
> for (i = 0; i < extent->l1_size; i++) {
> if (extent->entry_size == sizeof(uint64_t)) {
> le64_to_cpus((uint64_t *)extent->l1_table + i);
> } else {
> assert(extent->entry_size == sizeof(uint32_t));
> le32_to_cpus((uint32_t *)extent->l1_table + i);
> }
> }
> only asserts that extent->entry_size is sane if the l1_size
> is non-zero, and it does it inside the loop so we assert
> once for each entry. Hiding the assert like this might be
> part of what's confusing Coverity.
>
> (2) we allocate the l1_backup_table() with a size of l1_size,
> which will differ depending on extent->entry_size; but then
> we do the endianness conversion on it using
> for (i = 0; i < extent->l1_size; i++) {
> le32_to_cpus(&extent->l1_backup_table[i]);
> }
> which assumes always 32-bit entries. Is that correct?
Yes, because 64 bit entries only occur for the seSparse format. At
least our current (RO) implementation does not use a backup table when
accessing seSparse images. (Hence the assertion against that at the
beginning of the enclosing if block.)
Max
signature.asc
Description: OpenPGP digital signature
