Opened 10 months ago

Closed 9 months ago

#14507 closed defect (fixed)

TONY: Engine crashes on strict-alignment platforms

Reported by: dwatteau Owned by: lephilousophe
Priority: normal Component: Engine: Tony
Version: Keywords: strict alignment
Cc: Game: Tony Tough


Current Git HEAD immediately crashes with on the mips64el platform I have here, which has strict-alignment constraints and 64-bit pointers (and is little-endian).

I can't say whether this impact more "serious" strict-alignment platforms we support. Building with UBSan on a regular x86 system also lets one reproduce the issue more easily.

GDB and UBSan logs below.

Attachments (2)

gdb-tony-crash-strict-alignment-mips64el.txt (1.9 KB ) - added by dwatteau 10 months ago.
GDB backtrace on the mips64el system
ubsan-tony-strict-alignment.txt.gz (1.5 KB ) - added by dwatteau 10 months ago.
(gzip'd) UBSan output when running with -fsanitize=alignment on an x86 system

Download all attachments as: .zip

Change History (7)

by dwatteau, 10 months ago

GDB backtrace on the mips64el system

by dwatteau, 10 months ago

(gzip'd) UBSan output when running with -fsanitize=alignment on an x86 system

comment:1 by eriktorbjorn, 9 months ago

It looks to me like it allocates memory blocks (in expr.cpp) for one or more Expressions, plus one byte at the beginning to store the size of the block, measured in number of Expressions.

It's probably that that puts each Expression on an unaligned address.

I'm not really familiar with this engine, but it seems to me that there could be two ways forward:

  • Instead of using a single byte, use a larger type so that the Expressions end up on aligned addresses. Is there any good way to figure out what the alignment is?
  • Instead of storing the size in the memory block, wouldn't it be possible to ask the memory manager for the size with globalSize() / sizeof(Expression)?

comment:2 by eriktorbjorn, 9 months ago

I believe the following functions would have to be changed:

  • duplicateExpession(MpalHandle h)
  • evaluateAndFreeExpression(byte *expr)
  • parseExpression(const byte *lpBuf, const Common::UnalignedPtr<MpalHandle> &h)
  • compareExpressions(MpalHandle h1, MpalHandle h2)
  • freeExpression(MpalHandle h)

If I understand correctly, as long as you have an MpalHandle you can ask the memory manager for the size of the block. Because while MpalHandle is just a void pointer, it should still always point to a MemoryItem. But when there's just a raw byte pointer...? Probably not. The memory manager only keeps some meta data on each block it allocates, but doesn't actually keep track of them afterwards.

So the "ask memory manager for size" option is probably out.

So what about increasing the size of the counter at the beginning of the block? Would it be enough to increase it to the size of intptr_t? We can still have it only use the first byte.

Though there are a couple of places, particularly in parseExpession(), where I'm still not sure exactly what it's doing...

Last edited 9 months ago by eriktorbjorn (previous) (diff)

comment:3 by eriktorbjorn, 9 months ago

I played around a bit with alignof(), but I couldn't get it to work. I suspect it's because of the data passed to expr.cpp from the "outside", so to speak, and again I lack the understanding of the engine.

Maybe option 3 would be accessing the data as if it was a memory buffer, not a struct... but that's going to be ugly, I think.

comment:4 by lephilousophe, 9 months ago

PR #5148 seems to fix the bug. At least with UBsan.

comment:5 by lephilousophe, 9 months ago

Owner: set to lephilousophe
Resolution: fixed
Status: newclosed

The PR has now landed in master and 2.7.1

Note: See TracTickets for help on using tickets.