TONY: Engine crashes on strict-alignment platforms

[dwatteau]

Current Git HEAD immediately crashes with tony-demo.zip 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.

[dwatteau]

GDB backtrace on the mips64el system

[dwatteau]

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

[eriktorbjorn]

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)?

[eriktorbjorn]

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...

[eriktorbjorn]

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.

[lephilousophe]

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

[lephilousophe]

The PR has now landed in master and 2.7.1