Re: lua-5.1 restorestack memory handling bug

[Martin Guy <martinwguy@...>]

On 7 February 2012 08:57, Marko Lindqvist <cazfi74@gmail.com> wrote:
> Freeciv ( http://www.freeciv.org/ ) source tree includes copy of
> lua-5.1. We got a bug report about build failure, and this might be a
> real bug (at least on some platforms and compiler options)

> dependencies/lua-5.1/src/ldebug.c:620:21: error: cast
> from 'char ' to 'TValue ' (aka 'struct lua_TValue *') increases required
> alignment from 1 to 8 [-Werror,-Wcast-align]
> StkId errfunc = restorestack(L, L->errfunc);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from
> ../../../../../src.patched/dependencies/lua-5.1/src/ldebug.c:21:
> ../../../../../src.patched/dependencies/lua-5.1/src/ldo.h:25:28: note:
> instantiated from:
> #define restorestack(L,n) ((TValue )((char )L->stack + (n)))
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
> So, "L->stack + (n)" is calculated as char (1 byte space reserved),
> but then cast to pointer (8 bytes). In theory that pointer now has 7*8
> of its bits in undefined state.

The value your are worried about is calculated and stored as a pointer
difference using ((char*)(L) - (char *)(p)) where L and P are both
(TValue *), then the old value is restored using the reverse operation
- but the difference will always be a multiple of sizeof(TValue *).

If the compiler warning is giving you build failures (why?), you can
rewrite the macros to store the difference as calculated between
(TValue *)'s instead of (char *)'s, which eliminates the compiler
warning, and with a quick smoke test, the interpreter still seems to
work:

-#define savestack(L,p)         ((char *)(p) - (char *)L->stack)
-#define restorestack(L,n)      ((TValue *)((char *)L->stack + (n)))
+#define savestack(L,p)         ((p) - (L)->stack)
+#define restorestack(L,n)      ((L)->stack + (n))

But a bug is usually defined as something that causes a runtime
failure, not something that makes a compiler give warning messages, so
I'd tend to apply the rule "don't fix what ain't broke".

    M