406007 - MMGC_GET_STACK_EXENTS doesn't safely stash registers on some platforms

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Jason Orendorff [:jorendorff]]

Attachment: v1 - remove do/while blocks from MMGC_GET_STACK_EXENTS

On MacIntel, MMGC_GET_STACK_EXENTS looks like this:

	#define MMGC_GET_STACK_EXENTS(_gc, _stack, _size) \
		do { \
		volatile auto int save1,save2,save3,save4,save5,save6,save7;\
		asm("movl %%eax,%0" : "=r" (save1));\
		asm("movl %%ebx,%0" : "=r" (save2));\
		...
		asm("movl %%esp,%0" : "=r" (_stack));\
		_size = (uintptr)_gc->GetStackTop() - (uintptr)_stack;	} while (0)

Note that all those volatile locals are declared inside the do-while block.  Here's how this is used:

	GCWorkItem item;
	MMGC_GET_STACK_EXENTS(this, item.ptr, item._size);
	...
	PushWorkItem(work, item);
	Mark(work);

The region of the stack occupied by save1...saveN is included in `item`, but then the variables go out of scope, and that memory could be overwritten (in fact it seems quite likely) before Mark() gets to them.

Comment 1

[Jason Orendorff [:jorendorff]]

Incidentally, is there a reason EXENTS is spelled that way?  Can I change it to EXTENTS?

Comment 2

[Edwin Smith]

nice find!  sure, s/EXENTS/EXTENTS sounds right on.

Comment 3

[Tommy Reilly]

nice, I'm wondering where the do {} whiles came from in the first place, maybe there was a reason for it?    Maybe somebody tried to use the macro twice in one function and made this change to allow that?   Anyway, should be removed.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: rename EXENTS to EXTENTS

Pushed v1 and this patch.