Memory leak on macro expansion into function

[nyankers]

This took a while to narrow down and recreate.

Suppose you try to compile the following invalid LPC file:

#define A(x) static int a() { return (x); }
#define B(x) static int b() { return (x); }

A(1,2)

B(1)

This will fail to compile, which is itself correct, but then upon rebooting the server (e.g. hotboot as a System user, reboot as any user, etc.), the following message will pop up on the server (which causes a hotboot to fail):

Jul  1 00:03:07 ** System halted.
FREE(5570c053f3f8/40), token.cpp line 1305:
 09 's 't 'a 't 'i 'c '  'i 'n 't '  'b '( ') '  '{ '  'r 'e 't 'u 'r 'n '  '(
free(): invalid pointer
Aborted

I've only just been able to reproduce it, so I've not done further analysis yet. At first, I expected A(1,2) to be the problem element, but it appears it's failing on B(1) (a correct macro expansion into function) following the incorrect one. I also thought reaching the function limit with macros could cause this, but if so, I haven't been able to reproduce that so far. This issue has been pretty insidious to track down, since I won't find out for hours or days later, long after I solved the LPC issue that caused it.

[nyankers]

From debugging this, it looks like this may be happening because PP::clear() will call Macro::clear() before TokenBuf::clear(). TokenBuf::pop() only frees the buffer under certain circumstances, and in this case (where fd < -1), that requires the macro to be intact, but because Macro::clear() is called first in this scenario, mc->narg will contain junk data.

I don't know if there's any reason for this ordering, but considering TokenBuf uses Macro and not the other way around, I'd think it makes logical sense to swap the two statements.

[dworkin]

Nice work!