Opened 3 years ago

Closed 3 years ago

Last modified 7 months ago

#7039 closed defect (fixed)

Indeo3 unaligned accesses

Reported by: wjp Owned by: wjp
Priority: normal Component: Video
Keywords: Cc:
Game: Beavis & Butthead Virtual Stupidity

Description

The indeo3 codec reads from the unaligned pointer 'ref_frm_pos' without using the appropriate memory access macros (READ_UINT16, READ_UINT32).

This causes crashes at least for BBVS on MIPS. Reported and tracked down by joostp.

Ticket imported from: bugs/7039.

Attachments (1)

indeo3_force_align.patch (1.1 KB) - added by joostp 3 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 3 years ago by wjp

NB: There are also derived pointers such as 'ref_lp' that need attention.

Changed 3 years ago by joostp

Attachment: indeo3_force_align.patch added

comment:2 Changed 3 years ago by joostp

Attached is a patch that enables alignment checking in Indeo3Decoder::decodeChunk() on x86/x86_64, so that the issue can be reproduced more easily.

After applying the patch, make sure to compile ScummVM with SCUMM_NEED_ALIGNMENT defined:

CXXFLAGS="-DSCUMM_NEED_ALIGNMENT" ./configure ...

comment:3 Changed 3 years ago by wjp

I don't think that patch will work, since it won't force the compiler to generate unaligned read instructions.

I have an attempt at a fix at https://github.com/wjp/scummvm/commits/indeo3_align .
Could you test if it works for you?

comment:4 Changed 3 years ago by wjp

If I disable optimizations, and force common/endian.h to fall back to the block of single-byte reads, I can use this patch to test it on x86 it seems, and with my patch to indeo3 it then no longer crashes.

comment:5 Changed 3 years ago by joostp

Your patch fixes the crashes for me as well, nice. :)

comment:6 Changed 3 years ago by lordhoto

Sounds good. Did we convince ourselves that the writes really are no issue and are always aligned?

comment:7 Changed 3 years ago by joostp

I didn't check extensively, but the pointers that were written to that I looked at seemed to be aligned (don't know if that was due to luck or 'design', though).

comment:8 Changed 3 years ago by wjp

Owner: set to wjp
Resolution: fixed
Status: newclosed

comment:9 Changed 7 months ago by digitall

Component: Video
Game: Beavis & Butthead Virtual Stupidity
Note: See TracTickets for help on using tickets.