test-verify.t failure after 8d44b5a2974f

[Matt Mackall]

On Mon, 2012-05-07 at 02:00 +0200, Mads Kiilerich wrote:
> I have been trying to make brendan's windows buildbut pass cleanly. But 
> starting with "the" 2.2.1 changeset test-verify.t has started to fail.
> 
> On line 76 of test-verify.t "hg verify" will pause and show a message 
> box saying "python.exe has stopped working" and APPCRASH.
> 
> I have no clue what the exact problem is - but I also haven't dived into 
> the code. A quick linux valgrind shows no problem on that 'hg verify'.
> 
> I assume it can be reproduced by following 
> http://mercurial.selenic.com/wiki/HackableMercurial#Running_the_test_suite_under_MSYS 
> - or Brendan can give access to the buildbut where there is a 
> w2k8/hg/verifyfail.cmd that can reproduce the problem.

Managed to track this down on Wine. The issue seems to be that the new
index object is not expecting the destructor to get called when the
initializer fails. So the failing initializer avoids incref on
self->data, but then unconditionally decrefs it in the destructor. 

Apparently, by some quirk of implementation[1], on 32-bit Windows the
data object gets freed before the index, thus leading to a decref on
unallocated memory and a segfault.

So the simplest fix seems to be to unconditionally incref:

diff -r 871e24e9361e mercurial/parsers.c
--- a/mercurial/parsers.c	Sun May 06 14:37:51 2012 -0500
+++ b/mercurial/parsers.c	Mon May 07 15:25:02 2012 -0500
@@ -983,6 +983,7 @@
 	self->ntdepth = self->ntsplits = 0;
 	self->ntlookups = self->ntmisses = 0;
 	self->ntrev = -1;
+	Py_INCREF(self->data);
 
 	if (self->inlined) {
 		long len = inline_scan(self, NULL);
@@ -998,7 +999,6 @@
 		self->raw_length = size / 64;
 		self->length = self->raw_length + 1;
 	}
-	Py_INCREF(self->data);
 
 	return 0;
 bail:

Notably, this is the reverse of this change here, which now proves to be
unrelated to the actual refcount fix in that patch:

 http://www.selenic.com/hg/rev/8d44b5a2974f#l1.38

If this had been two patches, bisect would have told us which was to
blame. So the moral is: don't roll multiple changes together!

I've queued a fix for stable. I'm pretty sure this is not vulnerable to
heap exploits and only affects damaged revlogs, so we'll hold off on
2.2.2 until June 1.

[1] Python probably frees local objects in hash order, and the hash
order changes.

-- 
Mathematics is the supreme nostalgia of our time.