im.pidgin.pidgin: d77adf1b9a4b44f121620f20e2643602e3f6776e

[Evan Schoenberg]

(Apologies to Mark for receiving this twice - this is the reply to  
which Mark just replied).


On Jan 21, 2008, at 2:09 AM, Mark Doliner wrote:
> I disapproved this.  I guess there are two reasons:
>
> 1. Checking the validity of a FlapConnection by checking whether the  
> memory
> address of the struct exists in a GList is ugly.  I imagine it will  
> work
> 99.99999999% of the time, but it is theoretically possible for the  
> same memory
> address to be reused for a new and different FlapConnection, which  
> means you
> would end up free'ing something you hadn't intended.

I don't think that's right.

1. Every time a FlapConnection is made, it is immediately added to the  
GList.  There is only one entry point for creating a FlapConnection,  
flap_connection_new()
2. Every time a FlapConnection is destroyed, it is immediately removed  
from the GList.  There is only one entry point for destroying  a  
FlapConnection, flap_connection_destroy_cb().
3. The same memory could certainly be used for a new FlapConnection  
which was at some point in the past used for a FlapConnection, but  
that's not relevant to this code. That could never happen while a  
FlapConnection is valid (has not yet been freed) -- which is to say  
that a FlapConnection could never be created which makes use of memory  
currently pointed-to by the flap+connections list.
4. The list is used to check validity to ensure that a double-free  
doesn't occur; at no point is it iterated and universally freed (which  
would still never fail since the only way a FlapConnection is freed  
will remove that pointer from the list at the same time).

Did I miss something?

Yes, it's ugly.  The same sad-but-necessary technique is used to check  
for PurpleConnection objects' validities, for the same reason: If a  
callback is registered and is not canceled when the object is freed,  
once it is called, we will crash.  GObjectification of the objects in  
question (with reference counting) -- or implementation of manual  
reference counting -- would fix the problem.

> 2. This totally should not be necessary.  If the FlapConnection is  
> destroyed
> by flap_connection_destroy before the timeout is called then the  
> timeout is
> removed.  See the first two lines of flap_connection_destroy().

I see that, and I saw that.  Nevertheless, I have the following back  
trace, from libpurple 2.3.1 in Adium 1.2:
-----
Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000c23eab8b
Crashed Thread:  0

Thread 0 Crashed:
0   libpurple                     	0x08a0cf4d  
flap_connection_destroy_cb + 31
1   com.adiumX.AdiumPurple        	0x08773d8c callTimerFunc + 25
2   com.apple.CoreFoundation      	0x96018b7e CFRunLoopRunSpecific +  
4494
3   com.apple.CoreFoundation      	0x96018d38 CFRunLoopRunInMode + 88
4   com.apple.HIToolbox           	0x931638a4  
RunCurrentEventLoopInMode + 283
5   com.apple.HIToolbox           	0x931636bd ReceiveNextEventCommon +  
374
6   com.apple.HIToolbox           	0x93163531  
BlockUntilNextEventMatchingListInMode + 106
7   com.apple.AppKit              	0x964fdd5b _DPSNextEvent + 657
8   com.apple.AppKit              	0x964fd6a0 -[NSApplication  
nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
9   com.apple.AppKit              	0x964f66d1 -[NSApplication run] + 795
10  com.apple.AppKit              	0x964c39ba NSApplicationMain + 574
11  com.adiumX.adiumX             	0x00002fc2 _start + 216
12  com.adiumX.adiumX             	0x00002ee9 start + 41
----

disassembly of the compiled libpurple 2.3.1 binary shipping with Adium  
1.2 shows that flap_connection_destroy_cb + 31 is:
_flap_connection_destroy_cb:
	+0	00100f2a  55					  pushl		  %ebp
	+1	00100f2b  89e5					  movl		  %esp,%ebp
	+3	00100f2d  56					  pushl		  %esi
	+4	00100f2e  53					  pushl		  %ebx
	+5	00100f2f  83ec50				  subl		  $0x50,%esp
	+8	00100f32  e800000000			  calll		  0x00100f37
    +13	00100f37  5b					  popl		  %ebx
    +14	00100f38  8b4508				  movl		  0x08(%ebp),%eax
    +17	00100f3b  8945dc				  movl		  %eax,0xdc(%ebp)
    +20	00100f3e  8b45dc				  movl		  0xdc(%ebp),%eax
    +23	00100f41  8b00					  movl		  (%eax),%eax
    +25	00100f43  8945e0				  movl		  %eax,0xe0(%ebp)
    +28	00100f46  8b45e0				  movl		  0xe0(%ebp),%eax
    +31	00100f49  8bb0c8000000			  movl		  0x000000c8(%eax),%esi
    +37	00100f4f  e8eac1f1ff			  calll		  _purple_connections_get_all
    +42	00100f54  89742404				  movl		  %esi,0x04(%esp)
    +46	00100f58  890424				  movl		  %eax,(%esp)
    +49	00100f5b  e8d9b40d00			  calll		  0x001dc439				    _g_list_find



Note that +37, _purple_connections_get_all, is the function call made  
by PURPLE_CONNECTION_IS_VALID().  We crash before that. The only thing  
which happens before that call is made is:
	od = conn->od;
so dereferencing conn is causing a crash.

The explanation that came with the crash was, as is typical for  
crashes in callback methods (there are several others which are  
longtime offenders), "I woke my computer from sleep, and Adium crashed."

> For the record I absolutely hate PURPLE_CONNECTION_IS_VALID and  
> think we
> should get rid of it.

Getting rid of it without a replacement would be crashtastic.  I agree  
that it's a hack of a solution which only catches those cases we have  
the foresight to consider, and I'd love to see it replaced by an  
actual solution.  I've held off for a long time now on giving it  
reference counting, as each time I return to consider doing so the  
GObjectification discussion comes back... and GObjectification would  
of course obsolete any such solution.

Cheers,
Evan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://pidgin.im/pipermail/devel/attachments/20080123/ee6d4cba/attachment.html>