Comment 18 for bug 711225

Revision history for this message
Barry Warsaw (barry) wrote :

So, here's the problem. In python-apt, progress.cc, PyFetchProgress::Pulse(), you have the following code at about line 398:

   if((result == NULL) || (!PyArg_Parse(result, "b", &res)))
   {
      // most of the time the user who subclasses the pulse()
      // method forgot to add a return {True,False} so we just
      // assume he wants a True. There may be a Python exception on the stack
      // that must be cleared.
      PyCbObj_BEGIN_ALLOW_THREADS
      return true;
   }

What this is essentially doing is saying, whatever comes back from the pulse() callback should be either the Python bool value True or False. The PyArg_Parse() call has the effect of trying to turn whatever the callback returns into a short integer. So, if True or False were returned, you'd get 1 and 0. But the pulse() callback can actually return anything, so if it returned something that could be coerced into an integer (deep in Python, by PyInt_AsLong() via the 'b' flag), it would still work (fsvo "work").

The problem happens when pulse() returns something that cannot be coerced to an int, and in the jockey case it's actually getting None returned. python-apt clearly seems this is a common possibility based on the comment in the code. Probably the most likely error situation in client code is falling off the end without returning True or False, and when Python falls off the end of a function, an implicit None is returned. This is exactly what's happening in the jockey case. 'result' is None, and that cannot be coerced to an int, so PyInt_AsLong sets a TypeError, which gets dutifully propagated up the call stack until you get to line 398 in progress.cc.

So, PyFetchProgress::Pulse() wants to ignore a bogus return, but when it enters the 'if' clause at line 400, it's returning a non-error value 'true' *while an exception is pending on Python's stack*. The effect of this is that the next time Python checks for an outstanding exception, it'll see the TypeError and throw it, but as you can tell, it's far away from the *actual* error location. The behavior witnessed should have been a clue that something like this was happening (it did eventually dawn on me :).

The fix is simple. When ignoring any possible exception from PyArg_Parse(), you *also* need to clear the exception, which is what the attached branch does. Note that I don't particularly like ignoring bogus return values here, but that's a yak to shave on some other day.

I've verified the branch fixes the bug using Martin's same reproducible recipe.