Don’t modify tuples that others may already have references to

[cwalther]

Short Version

Due to a bug in Cyan’s code, setting individual entries of a multi-value SDL variable from Python (ptSDL.setIndex()) has only ever worked by chance. All current uses of it happened to work with Python 2.3, but some break with Python 2.7. Symptoms fixed by the attached commit include broken private chat functionality in the neighborhood egg room and inability to enter Teledahn buckets.

Long Version

As D'Lanor discovered, entering an egg room cubicle with the H-uru client (Python 2.7) causes the following error log, and chat is not made private as it should be:

(10/25 14:49:32) cPythConeOfSilence01 - Traceback (most recent call last):
(10/25 14:49:32)   File ".\python\xChatChannelRegion.py", line 130, in OnNotify
(10/25 14:49:32)     self.IAddMember(event[2])
(10/25 14:49:32)   File ".\python\xChatChannelRegion.py", line 149, in IAddMember
(10/25 14:49:32)     PtDebugPrint("xChatChannel: memberID=%d added to SDL." % (memberID),level=kDebugDumpLevel)
(10/25 14:49:32)   File "PlasmaTypes", line 138, in PtDebugPrint
(10/25 14:49:32) SystemError: ..\Objects\tupleobject.c:142: bad argument to internal function

It turns out this backtrace is a bit misleading, the error does not happen in PtDebugPrint(). What fails is the middle one of these lines:

            if self.SDL["intSDLChatMembers"][count] == -1:
                self.SDL.setIndex("intSDLChatMembers",count,memberID)
                PtDebugPrint("xChatChannel: memberID=%d added to SDL." % (memberID),level=kDebugDumpLevel)

SDL["intSDLChatMembers"] is a tuple (8-tuple in this case), and SDL.setIndex() (plPythonSDLModifier::SetItemIdx()) modifies that tuple directly. This is a big no-no, because tuples are supposed to be immutable. The C API to modify tuples only exists because there must be a way to build tuples in the first place, but once they’re out in the wild and someone else might already have references to them, it mustn’t be used anymore. This is Cyan’s code and unchanged between CWE, CWE-ou, and H-uru/Plasma.

The attempt to modify the tuple can have three outcomes, according to my observations. Which one is taken is deterministic but unpredictably dependent on various seemingly unrelated circumstances:

1. It can work: When nobody else has a reference to the tuple, modifying it succeeds, and everything works as intended.
2. It can fail spectacularly: When the tuple has a reference count greater than 1, Python notices the error and refuses to modify it. The error bubbles up through the Python interpreter until it arrives in the Python script, where it appears as an exception (SystemError), causing the observed backtrace, and terminates execution of the script.
3. It can fail silently: When the tuple has a reference count greater than 1, Python notices the error and refuses to modify it. The error, for some reason that I haven’t investigated yet, is somehow swallowed somewhere before it reaches the Python script. The script continues executing and it looks like everything went well, but the SDL value has actually not been changed.

With CWE-ou with Python 2.3 (and probably also the original Cyan MOULa client), normally outcome 1 happens. I can easily provoke outcome 3 however by adding a variable that keeps a reference to the tuple – the added PtDebugPrint confirms that the SDL setting has not taken effect:

            if self.SDL["intSDLChatMembers"][count] == -1:
                s = self.SDL["intSDLChatMembers"]
                self.SDL.setIndex("intSDLChatMembers",count,memberID)
                del s
                PtDebugPrint("SDL after: " + repr(self.SDL["intSDLChatMembers"]))
                PtDebugPrint("xChatChannel: memberID=%d added to SDL." % (memberID),level=kDebugDumpLevel)

There may be a way to provoke outcome 2 too, I haven’t tried that.

With H-uru/Plasma with Python 2.7, normally outcome 2 happens. I can easily provoke outcome 3 however by various seemingly innocuous changes to the Python code, e.g. adding another PtDebugPrint (yes, this is after where the error occurs):

            if self.SDL["intSDLChatMembers"][count] == -1:
                self.SDL.setIndex("intSDLChatMembers",count,memberID)
                PtDebugPrint("")
                PtDebugPrint("xChatChannel: memberID=%d added to SDL." % (memberID),level=kDebugDumpLevel)

There may be a way to provoke outcome 1, but I doubt it – the fact that the tuple has reference count 2 in this case probably comes from differences in Python’s garbage collection between 2.3 and 2.7.

The way to fix this is making plPythonSDLModifier::SetItemIdx() create a new tuple rather than modify an existing one. I have checked all uses of ptSDL.setIndex() and unless I missed anything they don’t mind if it replaces the tuple. I have also verified that the egg room SDL setting works now (I didn’t do any multiplayer testing to see if it has the intended effect though), and found that it also fixes entering the Teledahn buckets. The other two uses of ptSDL.setIndex(), the Gahreesen gear niche and remembering the last opened page of every book on the Relto bookshelf, already appear to work without the fix (tuple refcount is 1).

I’ve also found the following comment that seems to refer to this issue:
tldnBucketBrain.py:417:

        riders = None   #Need to release variable otherwise the SDL refuses to update (thus why I've moved it ouside of the 'for' loop)
        ageSDL.setIndex(kStringAgeSDLRiders,index,-1)

[Hoikas]

This looks like some very good stuff to me. However, I am I am hesitant to merge your changes myself because I'm too unfamiliar with the CPython API.

[cwalther]

That’s fine, doesn’t hurt to have some more eyes look over it.

There’s a formal review going on over at OpenUru.org, if you want to keep an eye on that: http://foundry.openuru.org/fisheye/cru/CWE-3

[branan]

This looks good. I also did a quick glance at the scripts and I concur with @cwalther that this won't cause any issues. If it does end up causing any issues with scripts, it should be fixed python-side - this is the correct way to handle this on the C side of things.

Merging!