| Message ID | 1461776486-31484-4-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@citrix.com> Acked-by: Wei Liu <wei.liu2@citrix.com> > --- > CC: Ian Jackson <Ian.Jackson@eu.citrix.com> > CC: Wei Liu <wei.liu2@citrix.com> > > 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 >
On 4/27/16 12:01 PM, 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@citrix.com> > --- Reviewed-by: Doug Goldstein <cardoe@cardoe.com> Your assertion appears correct to me. Its the same conclusion I came to and same solution I came to independently.
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;
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@citrix.com> --- CC: Ian Jackson <Ian.Jackson@eu.citrix.com> CC: Wei Liu <wei.liu2@citrix.com> Please can someone carefully check my "No functional change" assertion? I am fairly sure it is correct, but this is a gnarly. --- tools/blktap2/drivers/tapdisk-disktype.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)