Compiled _dirstate_helpers causes crash with specified file commands

Bug #582656 reported by Matthew Fuller
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Bazaar
Fix Released
High
John A Meinel
2.0
Fix Released
High
John A Meinel
2.1
Fix Released
High
John A Meinel
bzr (Ubuntu)
Fix Released
Undecided
Unassigned
Karmic
Invalid
Undecided
Unassigned

Bug Description

When files are specified to certain commands (seen with stat, diff, and ci at least), and the C _dirstate_helpers is present, bzr will crash:

bzr: ERROR: exceptions.KeyError: 'foo'
[...]
  File "/home/fullermd/src/bzr/bzr.dev/bzrlib/dirstate.py", line 1203, in _find_block_index_from_key
    cache=self._split_path_cache)
KeyError: 'foo'

Doing a `rm bzrlib/_dirstate_helpers_pyx.so` will make bzr act normally (i.e., not fail).

This error does NOT occur with the 2.1.1 release, but DOES affect bzr.dev, as well as the current heads of the 2.1 and 2.0 branches, and so should block new releases on them :(

Reproduction (alter paths to taste):

#!/bin/sh -x
bzr="/home/fullermd/src/bzr/bzr.dev/bzr --no-aliases --no-plugins"
#bzr="/home/fullermd/src/bzr/2.1/bzr --no-aliases --no-plugins"
#bzr="/home/fullermd/src/bzr/2.0/bzr --no-aliases --no-plugins"
#bzr="/usr/local/bin/bzr --no-aliases --no-plugins"

${bzr} init A
(
 cd A ;
 mkdir foo ;
 touch foo/bar ;
 ${bzr} add ;
 ${bzr} ci -m 'add' ;
 echo "change" > foo/bar ;

 # Works
 ${bzr} stat ;

 # Both fail
 #${bzr} stat foo ;
 ${bzr} stat foo/bar ;
)

Tags: babune sru

Related branches

Revision history for this message
Robert Collins (lifeless) wrote :

 Ouch. Could you perhaps confirm that the full test suite works on your machine? Just-in-casely-yrs.

Revision history for this message
Matthew Fuller (fullermd) wrote :

In some testing, building manually from the bzr-2.1.1 tag FAILS, but building from the bzr-2.1.1 tarball works. The difference appears to be that from the tarball I'm getting the generated files (from pyrex 0.9.8.5), whereas in the scratch build I'm using pyrex 0.9.9.

Revision history for this message
Matthew Fuller (fullermd) wrote :

Downgrade from Critical since it probably won't affect tarballs (until the RM upgrades their pyrex, anyway).

Changed in bzr:
importance: Critical → High
Revision history for this message
Vincent Ladeuil (vila) wrote :

http:/babune.ladeuil.net:24842/job/debug-freebsd/1/#showFailuresLink list all the failing tests when files are generated with pyrex 0.9.9.

The "regular" freebsd8 slave is not affected since it keeps using the files generated with 0.9.8.5 (and will do so until the
corresponding files are modified in trunk).

Revision history for this message
John A Meinel (jameinel) wrote :

So just to confirm, *nothing* has changed in dirstate_helpers for a while now. This is simply caused by a change in behavior with pyrex 0.9.8.5 vs pyrex 0.9.9. Is that correct?

Certainly seems like something we should file upstream, though we can try to debug it here first.

Revision history for this message
Vincent Ladeuil (vila) wrote : Re: [Bug 582656] Re: Compiled _dirstate_helpers causes crash with specified file commands

>>>>> John A Meinel <email address hidden> writes:

    > So just to confirm, *nothing* has changed in dirstate_helpers for a
    > while now. This is simply caused by a change in behavior with pyrex
    > 0.9.8.5 vs pyrex 0.9.9. Is that correct?

That's the current understanding.

    > Certainly seems like something we should file upstream, though we can
    > try to debug it here first.

Sure, I skimmed the diffs but nothing obvious for me, so I'll leave it
to you since you know pyrex far better.

Revision history for this message
Matthew Fuller (fullermd) wrote :

> So just to confirm, *nothing* has changed in dirstate_helpers for a
> while now. This is simply caused by a change in behavior with pyrex
> 0.9.8.5 vs pyrex 0.9.9. Is that correct?

It appears to be (modulo the usual testing-not-showing-absense-of-bugs
deal).

Building the 2.1.1 tarball works. touch'ing the
_dirstate_helpers_pyx.pyx and rebuilding (with 0.9.9) makes it fail.

The diffs between the two generated .c files are pretty sizable, but
there's so much noise it's probably impossible to see what might cause
it from direct inspection; the diff is >6500 lines long (even after
adjusting the source filename comments to match).

Revision history for this message
Matthew Fuller (fullermd) wrote :

In the generated file, several lines into the section for
      /* "/home/mbp/bzr/prepare-2.1/bzrlib/_dirstate_helpers_pyx.pyx":1226 */
putting a pair of printf()'s around the line

      if (PyObject_Cmp(__pyx_v_source_parent_id, __pyx_10, &__pyx_6) < 0) {__pyx_filename = __pyx_f[0]; __pyx_lineno = 1226; goto __pyx_L1;}

shows that that's the line where it gives up and starts unwinding the stack on its way to death. With the tarball's .c file it always gets past that line. A little effort backtracking the args doesn't lead me anywhere obvious.

Revision history for this message
John A Meinel (jameinel) wrote :

I'm quite positive that this is a bug in pyrex itself, and I've got an associated file to demonstrate it.

However, we can workaround the bug for now, and I'll propose a patch to do so.

Basically, it looks like doing:

 try:
  something_that_raises_exception()
 except Exception:
  something_else

Leaves the exception flag set (PyErr_Occurred() returns True), which is tripping up at the:
  if x == y:

line later on. (PyObject_Cmp() probably checks if a side-effect error has occurred). Changing the lines to:

 try:
  something_that_raises_exception()
 except Exception, _:
  something_else

Does the right thing. I grepped the source code, and only found the non-object form in _dirstate_helpers.pyx.

The attached file is just a trivial demonstration. Pyrex 0.9.8.5/Cython should just work, but 0.9.8.6 and 0.9.9 should be raising the trapped KeyError.

Vincent Ladeuil (vila)
tags: added: babune
Changed in bzr:
status: Confirmed → Fix Released
assignee: nobody → John A Meinel (jameinel)
milestone: none → 2.2b3
Revision history for this message
Vincent Ladeuil (vila) wrote :

Dear ~ubuntu-sru, we'd like to get this into karmic-updates.
Per <https://wiki.ubuntu.com/StableReleaseUpdates/MicroReleaseExceptions>, we have an exception to take all of 2.0.6, conditional on running the tests during the package build and on running the tests in 2.0.6.
Some of those will fail in 2.0.6, but they should be fixed in 2.0.7.
But unlike the 2.1 and 2.2 series, it may be harder to get there, we did some changes in the test infrastructure that we considered too invasive to backport so we may have to discuss whether it's better to rely on manual verification than backporting a bigger patch (which may also require upgrading testtools and/or subunit).
For now I propose we build the package, then check there are only the expected failures in the package.

tags: added: sru
Revision history for this message
Martin Pool (mbp) wrote :

I know this bug depends a bit on specific pyrex versions. For it to
be SRU'd, we should probably check it is relevant to the Pyrex in
Karmic.

Revision history for this message
Martin Pitt (pitti) wrote :

I'm doubtful that it makes a lot of sense to fix that in karmic -- few software developers stay with such old versions, they are much more likely using maverick or at least lucid. However, we should fix that in maverick in an SRU indeed.

Revision history for this message
Vincent Ladeuil (vila) wrote :

This fix *is* included in the 2.1 and 2.2 series.

Vincent Ladeuil (vila)
Changed in bzr (Ubuntu):
status: New → Fix Released
Revision history for this message
Matthew Fuller (fullermd) wrote :

Anything having to do with bzr updates in an Ubuntu version almost 3 years out of support really doesn't deserve to still be open...

Changed in bzr (Ubuntu Karmic):
status: New → Invalid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.