Issue 1164 - Unordered GC finalization leading to memory bugs
Summary: Unordered GC finalization leading to memory bugs
Status: RESOLVED WONTFIX
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: Other Linux
: P2 enhancement
Assignee: No Owner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-04-19 14:06 UTC by Marcin Kuszczak
Modified: 2019-05-21 00:55 UTC (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Marcin Kuszczak 2007-04-19 14:06:14 UTC
class Storage {
    void sync() {};
}
class Test {
    Storage s;
    this() {s=new Storage;}
    ~this() {s.sync();}
}
void main() {
    new Test;
}

Code above triggers Segmentation fault. Didn't test it on Windows, but probably it is not platform specific issue.
Comment 1 Matti Niemenmaa 2007-04-19 14:10:54 UTC
Invalid. http://www.digitalmars.com/d/class.html#destructors has the following passage:

"When the garbage collector calls a destructor for an object of a class that has members that are references to garbage collected objects, those references are no longer valid. This means that destructors cannot reference sub objects. This rule does not apply to auto objects or objects deleted with the DeleteExpression."
Comment 2 Marcin Kuszczak 2007-04-19 14:35:39 UTC
d-bugmail@puremagic.com wrote:

> 
> "When the garbage collector calls a destructor for an object of a class
> that has members that are references to garbage collected objects, those
> references are no longer valid. This means that destructors cannot
> reference sub objects. This rule does not apply to auto objects or objects
> deleted with the DeleteExpression."
> 

Hmmm... but it doesn't help much with cleaning up on destruction. When
collecting would be done in two phases (1. calling all destructors, 2.
deallocating memory) maybe it would work.

Maybe I should mark it as enhancement?

Comment 3 Marcin Kuszczak 2007-04-20 01:50:04 UTC
Reopened and marked as enhancement.
Comment 4 Derek Parnell 2007-04-20 02:20:33 UTC
It also fails in Windows.

The workaround is to use the 'scope' attribute.

void main()
{
  scope Test t = new Test;
}
Comment 5 Pieter Penninckx 2014-07-04 08:21:11 UTC
Reproduced with DMD version 2.065.


If the destructor cannot reference sub objects, this implies that also an invariant cannot reference sub objects, because an invariant is called just before the destructor. Example below triggers segfault with DMD version 2.065.


class B
{
	double a, b, c, d, e, f, g, h;
	bool fun() const { return true;	}
}

class A
{
	B b;
	invariant() {
		assert(b !is null);
		if (b.fun()) // <- Segfault here, but b is not null.
		{ assert(true);	}
	}
	this() { b = new B(); }
	~this()	{}
}

int main() {
	A a = new A();
	return 0;
}
Comment 6 Andrei Alexandrescu 2016-12-22 15:12:21 UTC
Couldn't reproduce on dmd 2.072.1. Any better code sample?
Comment 7 Pieter Penninckx 2016-12-25 13:57:10 UTC
The following code still segfaults with DMD 2.072.1-0.

class X 
{
	double a, b, c, d, e, f, g, h;
	X sibling;
	bool fun() const { return true;	}
	invariant() {
		if(sibling !is null) {
			if (sibling.fun())
			{ assert(true);	}
		}
	}
	this() {sibling = new X(this); }
	this(X sibling_) {sibling = sibling_;}
	~this() {}
}

int main() {
	X x = new X();
	return 0;
}


Garbage collector + destructor (or finalizer) is a difficult combination. See for instance this comment (https://github.com/WebAssembly/design/issues/238#issuecomment-116877193) that strongly opposes introducing a destructor in Javascript because this combination can lead to object resurrection (objects made reachable again in a destructor call).
Comment 8 Andrei Alexandrescu 2016-12-25 16:23:20 UTC
Cool, was able to repro. Fortunately we have a couple of cards in our sleeve. What we could do is:

* Mark
* For each collectable object:
  * Call dtor
  * (NEW) Obliterate with .init
* Sweep

That way objects used during destruction will at least find objects in a deterministic state.
Comment 9 Pieter Penninckx 2016-12-26 10:22:31 UTC
Just to be sure I understand you correctly.

Am I right that the garbage collector currently works as follows:

* Mark (= mark all reachable objects as reachable)
* For each collectable (= non-reachable) object:
  * Call dtor
  * release memory of the collectable object

Am I right that the steps you are thinking about can also be formulated as follows:

* Mark (= mark all reachable objects as reachable)
* For each collectable (= non-reachable) object:
  * Call dtor
  * Obliterate with .init
* For each collectable object:
  * release memory of the collectable object
Comment 10 Andrei Alexandrescu 2016-12-26 12:41:06 UTC
(In reply to Pieter Penninckx from comment #9)
> Just to be sure I understand you correctly.
> 
> Am I right that the garbage collector currently works as follows:
> 
> * Mark (= mark all reachable objects as reachable)
> * For each collectable (= non-reachable) object:
>   * Call dtor
>   * release memory of the collectable object

I don't know exactly. I am pretty certain, however, that freed objects are currently not overwritten with .init.
Comment 11 safety0ff.bugz 2016-12-26 13:14:07 UTC
(In reply to Pieter Penninckx from comment #7)
> 
> int main() {
> 	X x = new X();
> 	return 0;
> }

Your invariant gets called infinitely:
x.sibling.sibling == x
sibling's invariant() executes before and after sibling.fun() executes.
The invariant has the line: if (sibling.fun())
Comment 12 safety0ff.bugz 2016-12-26 13:37:46 UTC
(In reply to Pieter Penninckx from comment #7)
>
> 	invariant() {
> 		if(sibling !is null) {
> 			if (sibling.fun())
> 			{ assert(true);	}
> 		}
> 	}
>

Also:
https://dlang.org/spec/contracts.html#Invariants "The code in the invariant may not call any public non-static members of the class or struct, either directly or indirectly. Doing so will result in a stack overflow, as the invariant will wind up being called in an infinitely recursive manner."
Comment 13 safety0ff.bugz 2016-12-26 14:03:35 UTC
(In reply to Pieter Penninckx from comment #9)
> 
> Am I right that the garbage collector currently works as follows:


It currently works as follows:

* Mark
* For each unmarked object:
*   Finalize it if necessary
*   If it can be released without overwriting it do so
* For each unmarked unreleased object:
*   release memory of the object

However, you shouldn't rely on this: http://dlang.org/spec/class.html#destructors

If you recompile druntime with the MEMSTOMP option, the GC and it will write arbitrary data to finalized memory.

Therefore it follows that referencing GC managed objects from invariants of other GC managed objects is unadvised.

I think a case could be made for being able to control insertion of invariant calls in destructors.
Comment 14 Pieter Penninckx 2016-12-27 08:01:13 UTC
(In reply to comment #11)

Woops. You're right. The segfault is caused by an infinite recursion and probably not by a GC problem. We can avoid the infinite recursion by marking fun() as private:

class X 
{
	X sibling;
	private bool fun() const { return true;	} // Note: this line changed.
	invariant() {
		if(sibling !is null) {
			if (sibling.fun()) { assert(true); }
		}
	}
	this() {sibling = new X(this); }
	this(X sibling_) {sibling = sibling_;}
	~this() {}
}

int main() {
	X x = new X();
	return 0;
}

This just runs normally for me. But if I understand comment #13 correctly, this is just luck and I shouldn't count on it, especially when the runtime is compiled with the MEMSTOMP option.
Comment 15 safety0ff.bugz 2016-12-27 10:35:05 UTC
(In reply to Pieter Penninckx from comment #14)
>
> This just runs normally for me. But if I understand comment #13 correctly,
> this is just luck and I shouldn't count on it, especially when the runtime
> is compiled with the MEMSTOMP option.

MEMSTOMP is essentially a tool used for debugging memory corruption type issues.
Compliant D code runs correctly regardless whether it is enable and I was using it as an illustration of what is allowed in the runtime.

I don't know what is the official stance on referencing other GC managed memory from invariant code.
Since it is automatically run before the destructor/finalizer, it violates the spec.
This restriction is quite severe, it might be better if invariants aren't called before the destructor.
Comment 16 Mathias LANG 2019-05-21 00:55:19 UTC
We advertise everywhere that you should not access GC-allocated items in destructors, but you can access anything that is allocated with another method. Just like you should not allocate in destructors.

Closing this, as it is not actionable.
Anything addressing this should be a DIP, as we have gathered over a decade of experience with this problem.