Peter Maydell <[email protected]> writes:
> In commit eb3f69cac62670 we removed the dependency of this
> mem plugin on the QEMU headers, but in doing that we introduced
> undefined behaviour when the plugin accesses unaligned memory.
> This shows up if you build with the gcc or clang address
ubsan
> sanitizer and run 'make check-tcg', in numerous warnings like:
>
> ../../tests/tcg/plugins/mem.c:167:27: runtime error: load of misaligned
> address 0x7f1f300354b1 for type 'uint16_t' (aka 'unsigned short'), which
> requires 2 byte alignment
> 0x7f1f300354b1: note: pointer points here
> 00 00 00 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 14
> 15 16 17 18 19 1a 1b 1c
> ^
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
> ../../tests/tcg/plugins/mem.c:167:27
>
> Fix this by rearranging the data reads and writes to use
> memcpy() instead.
>
> Fixes: eb3f69cac62670 ("tests/tcg/plugins/mem.c: remove dependency on qemu
> headers")
> Signed-off-by: Peter Maydell <[email protected]>
with the extra clarity on the sanitiser:
Tested-by: Alex Bennée <[email protected]>
Reviewed-by: Alex Bennée <[email protected]>
> ---
> tests/tcg/plugins/mem.c | 71 +++++++++++++++++------------------------
> 1 file changed, 30 insertions(+), 41 deletions(-)
>
> diff --git a/tests/tcg/plugins/mem.c b/tests/tcg/plugins/mem.c
> index 7d64e7018f..f3992abc8f 100644
> --- a/tests/tcg/plugins/mem.c
> +++ b/tests/tcg/plugins/mem.c
> @@ -123,6 +123,9 @@ static void update_region_info(uint64_t region, uint64_t
> offset,
> bool is_store = qemu_plugin_mem_is_store(meminfo);
> RegionInfo *ri;
> bool unseen_data = false;
> + void *val_ptr;
> + unsigned int val_size;
> + qemu_plugin_mem_value swapped_value;
>
> g_assert(offset + size <= region_size);
>
> @@ -144,61 +147,46 @@ static void update_region_info(uint64_t region,
> uint64_t offset,
> }
>
> void *ri_data = &ri->data[offset];
> +
> + swapped_value.type = value.type;
> switch (value.type) {
> case QEMU_PLUGIN_MEM_VALUE_U8:
> - {
> - uint8_t val = value.data.u8;
> - uint8_t *p = ri_data;
> - if (is_store) {
> - *p = val;
> - } else {
> - unseen_data = *p != val;
> - }
> + swapped_value.data.u8 = value.data.u8;
> + val_ptr = &swapped_value.data.u8;
> + val_size = 1;
> break;
> - }
> case QEMU_PLUGIN_MEM_VALUE_U16:
> - {
> - uint16_t val = be ? GUINT16_FROM_BE(value.data.u16) :
> - GUINT16_FROM_LE(value.data.u16);
> - uint16_t *p = ri_data;
> - if (is_store) {
> - *p = val;
> - } else {
> - unseen_data = *p != val;
> - }
> + swapped_value.data.u16 = be ? GUINT16_FROM_BE(value.data.u16) :
> + GUINT16_FROM_LE(value.data.u16);
> + val_ptr = &swapped_value.data.u16;
> + val_size = 2;
> break;
> - }
> case QEMU_PLUGIN_MEM_VALUE_U32:
> - {
> - uint32_t val = be ? GUINT32_FROM_BE(value.data.u32) :
> - GUINT32_FROM_LE(value.data.u32);
> - uint32_t *p = ri_data;
> - if (is_store) {
> - *p = val;
> - } else {
> - unseen_data = *p != val;
> - }
> + swapped_value.data.u32 = be ? GUINT32_FROM_BE(value.data.u32) :
> + GUINT32_FROM_LE(value.data.u32);
> + val_ptr = &swapped_value.data.u32;
> + val_size = 4;
> break;
> - }
> case QEMU_PLUGIN_MEM_VALUE_U64:
> - {
> - uint64_t val = be ? GUINT64_FROM_BE(value.data.u64) :
> - GUINT64_FROM_LE(value.data.u64);
> - uint64_t *p = ri_data;
> - if (is_store) {
> - *p = val;
> - } else {
> - unseen_data = *p != val;
> - }
> + swapped_value.data.u64 = be ? GUINT64_FROM_BE(value.data.u64) :
> + GUINT64_FROM_LE(value.data.u64);
> + val_ptr = &swapped_value.data.u64;
> + val_size = 8;
> break;
> - }
> case QEMU_PLUGIN_MEM_VALUE_U128:
> - /* non in test so skip */
> - break;
> + /* none in test so skip */
> + goto done;
> default:
> g_assert_not_reached();
> }
>
> + /* ri_data may not be aligned, so we use memcpy/memcmp */
> + if (is_store) {
> + memcpy(ri_data, val_ptr, val_size);
> + } else {
> + unseen_data = memcmp(ri_data, val_ptr, val_size) != 0;
> + }
> +
> /*
> * This is expected for regions initialised by QEMU (.text etc) but we
> * expect to see all data read and written to the test_data region
> @@ -213,6 +201,7 @@ static void update_region_info(uint64_t region, uint64_t
> offset,
> ri->seen_all = false;
> }
>
> +done:
> g_mutex_unlock(&lock);
> }
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
