crash on exit in slp_kill_tasks_with_stacks

[ghost]

Originally reported by: RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew)

---

(originally reported in Trac by mconst on 2013-04-26 23:45:44)

With the current stackless 2.7-slp, the following code crashes on exit:

import stackless

stackless.tasklet(stackless.test_cframe)(1)
stackless.schedule()

It's crashing in slp_kill_tasks_with_stacks, where it calls SLP_CHAIN_REMOVE to remove the task from the runqueue:

/* unlink from runnable queue if it wasn't previously remove()'d */
if (t->next && t->prev) {
    task = t;
    chain = &task;
    SLP_CHAIN_REMOVE(PyTaskletObject, chain, task, next, prev);
}

The problem is that this code is calling SLP_CHAIN_REMOVE incorrectly. "task" is supposed to be an output parameter, not input -- and it can't be an alias of *chain, or else SLP_CHAIN_REMOVE will think the list is empty and crash. The code should be:

/* unlink from runnable queue if it wasn't previously remove()'d */
if (t->next && t->prev) {
    chain = &t;
    SLP_CHAIN_REMOVE(PyTaskletObject, chain, task, next, prev);
    t = task;
}

I've attached a patch. With this patch, the program above works for me.

---

• Bitbucket: https://bitbucket.org/stackless-dev/stackless/issue/21

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@ctismer on 2013-04-28 15:02:19 said:

I was trying to find out when this problem crept in, but it appears to be from
before the move to mercurial.
Wondering why this gave never a problem before.

Can you please provide more info about your environment, compiler, version etc.?
I would like to reproduce the crash.

Thanks for the patch!

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

mconst on 2013-04-28 21:21:17 said:

Sorry, I misread the date on revision cf49004a05e2 -- it was actually a year and three weeks ago. Still in mercurial, though :-)

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

mconst on 2013-04-28 20:52:51 said:

It's not that old -- I believe it was introduced three weeks ago, in revision cf49004a05e2. It looks like that patch fixed another crash bug on shutdown (by adding a missing Py_INCREF, and using st.current instead of st.main), but it also modified the calls to SLP_CHAIN_REMOVE and SLP_CHAIN_INSERT.

The SLP_CHAIN_INSERT change shouldn't hurt anything, although it looks unnecessary to me (SLP_CHAIN_INSERT doesn't actually modify *chain unless it's NULL, and I don't think st.current can ever be NULL). The SLP_CHAIN_REMOVE call caused this bug.

I'm using the latest slp-2.7 from mercurial; I compiled it with gcc 4.7.0, and I'm running it on an x86-64 system with Linux 2.6.35.4. I think this crash should be cross-platform, though -- it's a 100% reliable null-pointer dereference. Is it not happening for you?

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@ctismer on 2013-04-28 22:13:30 said:

In fact, that was quite a bit before we moved to Mercurial. At the same time, Nagare also
build the new Stackless site for us, and I still did not update the Wiki for that....

And I guess some check-ins were a bit quicker than they should have been.
This will not happen any more, we got much more care- and thoughtful ;-)

Keep reporting, it is appreciated.

Please double-check before committing or submitting comments.
I will do my best to adhere to that, too. And I will fail, again.

People are people, for heaven's sake, although they try to be better.
What a contradiction per se!

cheers - chris

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@akruis on 2013-04-29 22:32:39 said:

I can reproduce the crash. The fix looks correct and - more importantly fixes the crash.
Fixed by changeset [d8c20220cf51].

[ghost]

Original comment by RMTEW FULL NAME (Bitbucket: rmtew, GitHub: rmtew):

---

@ctismer on 2013-04-29 23:07:04 said:

Anselm, I agree that this patch is ok, but it was a bit too quick.

I wanted to ask Michel Constant to create the relevant headers on top of the
patch, so it can be checked into mercurial with full user info, as I did
it for the last check-in that I did for you at __PyCon .