Pitfalls of delegates inside ranges

[H. S. Teoh]

On Mon, Sep 02, 2013 at 12:27:44AM +0200, Joseph Rushton Wakeling wrote:
> Hello all,
> 
> I came across an interesting little pitfall of delegates recently
> which I thought I'd share.
> 
> TL;DR: having reference types as struct members can be dangerous. ;-)

Yes.


> Suppose we have a fairly simple range which is supposed to count
> from 0 to some maximum.  The trick is that the count is implemented
> by a function that's accessed by a delegate.
> 
> struct Foo
> {
>     private size_t _count = 0, _max;
>     private void delegate() jump = null;
> 
>     @disable this();
> 
>     this(size_t max)
>     {
>         _max = max;


This:

>         jump = &jump10;
>     }


And this:

[...]
>     void jump10()
>     {
>         _count += 10;
>         writeln("At end of jump, count = ", _count);
>     }
> }

are a recipé for disaster. :)

Well, Adam Ruppe & myself found out the hard way that delegates closing
over struct variables are dangerous, because structs can be moved about
/ copied by value etc.. Consider this innocent-looking code:

	struct S {
		void delegate()[] cleanups;
		void* data;
		this(int) {
			data = malloc(1024);
			cleanups ~= { free(data); };
		}
		~this() {
			foreach_reverse (f; cleanups)
				f();
		}
	}

	S makeS() { return S(123); /* create new instance of S */ }

	void main() {
		auto s = makeS();
		... // do some other stuff here
	} // segfault

Spot the bug. :)

It's very non-obvious, of course. It's ultimately caused by the delegate
that closes over S.data. When the ctor creates the delegate, the
delegate's context pointer is set to the instance of S being initialized
in makeS(). This instance then returns S to main() -- but since structs
are passed *by value*, it gets *copied* into a new location reserved for
the local variable s in main(). The original instance of S, that the
delegate's context pointer is pointing to, is now invalid. If main()
exits right away, it *appears* to be fine -- because the original
instance of S is still sitting on the stack (though technically it is no
longer valid since makeS()'s scope has ended). But as soon as you make
another function call, or allocate new variables on the stack, etc.,
they will begin to overwrite that original copy of S, and so when the
dtor of s is invoked, it calls the delegate which tries to access the
*original* copy of S that has now been overwritten with garbage. Result:
segfault.

The compiler really should reject attempts to close over struct
instances in this way, as it's really dangerous, but looks innocent.
Worst of all, this bug is extremely hard to trace, because by the time
the dtor is invoked, the original cause of the problem may already be
very far away, and code reviews generally fail to catch it because it's
so subtle.


T

-- 
Let's eat some disquits while we format the biskettes.