Opened 4 years ago

Closed 4 years ago

Last modified 13 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 4 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 by wjp, 4 years ago

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

by joostp, 4 years ago

Attachment: indeo3_force_align.patch added

comment:2 by joostp, 4 years ago

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 by wjp, 4 years ago

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 by wjp, 4 years ago

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 by joostp, 4 years ago

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

comment:6 by lordhoto, 4 years ago

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

comment:7 by joostp, 4 years ago

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 by wjp, 4 years ago

Owner: set to wjp
Resolution: fixed
Status: newclosed

comment:9 by digitall, 13 months ago

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