Re: [Xen-devel] [PATCH for-4.7 3/7] tools/blktap2: Fix array initialisers for tapdisk_disk_{types, drivers}[]

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Apr 27, 2016 at 06:01:22PM +0100, Andrew Cooper wrote:
> Clang points out:
> 
>   tapdisk-disktype.c:117:2: error: initializer overrides prior initialization
>   of this subobject [-Werror,-Winitializer-overrides]
>           0,
>           ^
>   tapdisk-disktype.c:115:23: note: previous initialization is here
>           [DISK_TYPE_VINDEX]      = &vhd_index_disk,
>                                     ^~~~~~~~~~~~~~~
> 
> Mixing different initialiser styles should be avoided; The actual behaviour is
> different to the expected behaviour.  This specific example has been broken
> since its introduction in c/s 7b4dea554 "blktap2: Fix tapdisk disktype issues"
> in 2010, and is caused by the '#if 0' block removing &tapdisk_{sync,vmdk}.
> 
> First of all, remove what were intended to be trailing NULL entries in
> tapdisk_disk_{types,drivers}[], making consistent use of Designated
> Initialisers for the initialisation.
> 
> This requires changing the loop in tapdisk_disktype_find() to be based on the
> number of elements in tapdisk_disk_types[], rather than looking for the first
> NULL.  This fixes a latent bug, as the use of Designated Initializers causes
> to intermediate zero entries if not all indices are explicitly specified.
> 
> There is a second latent bug where tapdisk_disktype_find() assumes that
> tapdisk_disk_drivers[] has at least as many entries as tapdisk_disk_types[].
> This is not the case and tapdisk_disk_drivers[] had one entry fewer than
> tapdisk_disk_types[], but the NULL loop bound prevented an out-of-bounds read
> of tapdisk_disk_drivers[].  Fix the issue by explicitly declaring
> tapdisk_disk_drivers[] to have the same number of entries as
> tapdisk_disk_types[].
> 
> Finally, this leads to a linker error.  It turns out that tapdisk_vhd_index
> doesn't exist, and I can't find any evidence in the source history to suggest
> that it ever did.  I can only presume that it would have been #if 0'd out like
> tapdisk_sync and tapdisk_vmdk had it not been for this bug preventing a build
> failure.  Drop all three.
> 
> No functional change, but only because of the specific layout of
> tapdisk_disk_types[].
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>

> ---
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> Please can someone carefully check my "No functional change" assertion?  I am
> fairly sure it is correct, but this is a gnarly.

You assertion should be correct.

> ---
>  tools/blktap2/drivers/tapdisk-disktype.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/blktap2/drivers/tapdisk-disktype.c 
> b/tools/blktap2/drivers/tapdisk-disktype.c
> index e9a6890..e89d364 100644
> --- a/tools/blktap2/drivers/tapdisk-disktype.c
> +++ b/tools/blktap2/drivers/tapdisk-disktype.c
> @@ -33,6 +33,8 @@
>  #include "tapdisk-disktype.h"
>  #include "tapdisk-message.h"
>  
> +#define ARRAY_SIZE(a) (sizeof (a) / sizeof *(a))
> +
>  static const disk_info_t aio_disk = {
>         "aio",
>         "raw image (aio)",
> @@ -112,35 +114,25 @@ const disk_info_t *tapdisk_disk_types[] = {
>       [DISK_TYPE_LOG] = &log_disk,
>       [DISK_TYPE_VINDEX]      = &vhd_index_disk,
>       [DISK_TYPE_REMUS]       = &remus_disk,
> -     0,
>  };
>  
>  extern struct tap_disk tapdisk_aio;
> -extern struct tap_disk tapdisk_sync;
> -extern struct tap_disk tapdisk_vmdk;
>  extern struct tap_disk tapdisk_vhdsync;
>  extern struct tap_disk tapdisk_vhd;
>  extern struct tap_disk tapdisk_ram;
>  extern struct tap_disk tapdisk_qcow;
>  extern struct tap_disk tapdisk_block_cache;
> -extern struct tap_disk tapdisk_vhd_index;
>  extern struct tap_disk tapdisk_log;
>  extern struct tap_disk tapdisk_remus;
>  
> -const struct tap_disk *tapdisk_disk_drivers[] = {
> +const struct tap_disk *tapdisk_disk_drivers[ARRAY_SIZE(tapdisk_disk_types)] 
> = {
>       [DISK_TYPE_AIO]         = &tapdisk_aio,
> -#if 0
> -     [DISK_TYPE_SYNC]        = &tapdisk_sync,
> -     [DISK_TYPE_VMDK]        = &tapdisk_vmdk,
> -#endif
>       [DISK_TYPE_VHD]         = &tapdisk_vhd,
>       [DISK_TYPE_RAM]         = &tapdisk_ram,
>       [DISK_TYPE_QCOW]        = &tapdisk_qcow,
>       [DISK_TYPE_BLOCK_CACHE] = &tapdisk_block_cache,
> -     [DISK_TYPE_VINDEX]      = &tapdisk_vhd_index,
>       [DISK_TYPE_LOG]         = &tapdisk_log,
>       [DISK_TYPE_REMUS]       = &tapdisk_remus,
> -     0,
>  };
>  
>  int
> @@ -149,7 +141,11 @@ tapdisk_disktype_find(const char *name)
>       const disk_info_t *info;
>       int i;
>  
> -     for (i = 0; info = tapdisk_disk_types[i], info != NULL; ++i) {
> +     for (i = 0; i < ARRAY_SIZE(tapdisk_disk_types); ++i) {
> +             info = tapdisk_disk_types[i];
> +             if (!info)
> +                     continue;
> +
>               if (strcmp(name, info->name))
>                       continue;
>  
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel