Re: [Qemu-trivial] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

Ping.
Patchwork link: http://patchwork.ozlabs.org/patch/866732/

Patchew link: http://patchew.org/QEMU/20180127134908.24095-1-anatoly.address@hidden/

The code in tcg/tci.c reads some data from TCI bytecode through
pointer dereferencing. As far as I know unaligned reads in such a way are
undefined behavior and compiling with -fsanitize=undefined enumerated
them as such at run-time.

I have replaced such reads with invocations of ld{l,q}_he_p.
A comment in include/qemu/bswap.h:310 suggests they should be properly
translated by the compiler. I didn't added signed/unsigned casts
since bswap.h does contain separate signed/unsigned versions
for 16-bit integers but does not for 32- and 64-bit ones, so I supposed
the developers of the bswap.h already arranged everything so
integer promotions don't mess things up. I can add casts in case I'm
not right about it.
http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg06516.html

2018-01-28 9:42 GMT+03:00 Anatoly Trosinenko <address@hidden>:

My patch is kind of trivial quick fix that just eliminates these unaligned reads and doesn't seem to require complicated testing supposing my code properly handles integer promotion (and hope it will not slow the interpreter down).

Aligning everything, on the other hand, can not only remove the UB but also speed things up, but if I get it right, requires O(opcode count) manual work and subsequent less trivial testing that every opcode's argument layout match on generation and interpretation side (errors should be significantly localized due to present assertion on operation size, though). So my patch may be considered as temporary solution.

In fact, I had to similarly make these unaligned reads explicit when porting QEMU to _javascript_ because Emscripten hugely relies on absence of some kinds of UB such as "implicit" unaligned accesses, and such fix seemed to resolve this issue for me on "host with special alignment requirement".

2018-01-27 19:38 GMT+03:00 Stefan Weil <address@hidden>:

Am 27.01.2018 um 14:49 schrieb Anatoly Trosinenko:
> Use ldl_he_p / ldq_he_p functions instead of a plain memory access
> through pointer.
>
> Signed-off-by: Anatoly Trosinenko <address@hidden>
> ---
>  tcg/tci.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)

A better alternative might be aligning the relevant data when generating
the bytecode. See also my comment on alignment in tcg/tci/README.

Stefan

--

С уважением,
Анатолий Тросиненко
e-mail: address@hidden

--

С уважением,
Анатолий Тросиненко
e-mail: address@hidden