Opened 5 years ago

Closed 23 months ago

Last modified 16 months ago

#15755 closed patch (fixed)

Adium crash as a result of bug in purple_xfer_set_local_filename

Reported by: fain Owned by:
Milestone: 2.13.0 Component: libpurple
Version: 2.10.7 Keywords:
Cc:

Description (last modified by Robby)

purple_xfer_set_local_filename (in libpurple/ft.c) frees the "xfer->local_filename" before setting it to the passed-in value. However, if the passed-in "filename" is the same pointer as local_filename, the string will be nil and the app will crash under libgmalloc (see: https://trac.adium.im/ticket/16352#no1 ). The fix is simple: just check if the pointer values are equal, and if so, don't free the old string and copy the new one.

basically the new function should look like:

void
purple_xfer_set_local_filename(PurpleXfer *xfer, const char *filename)
{
	g_return_if_fail(xfer != NULL);

	if (filename != xfer->local_filename) {
		g_free(xfer->local_filename);
		xfer->local_filename = g_strdup(filename);
	}
}

A lot of the functions in this file have this problem, so it might be worth running through it.

Attachments (1)

15755-patch.diff (1.5 KB) - added by fain 5 years ago.
Here's the patch that fixes the free() calls in ft.c

Download all attachments as: .zip

Change History (9)

comment:1 Changed 5 years ago by fain

P.S. The backtrace for the adium crash is:

thread #1: tid = 0x2503, 0x00007fff96b466b0 libsystem_c.dylib`strlen + 16, stop reason = EXC_BAD_ACCESS (code=1, address=0x5d6b9bfc0)
    frame #0: 0x00007fff96b466b0 libsystem_c.dylib`strlen + 16
    frame #1: 0x0000000100a219dd libglib`g_strdup + 49
    frame #2: 0x0000000100bc1f16 libpurple`purple_xfer_set_local_filename + 52
    frame #3: 0x0000000100bc1b0e libpurple`purple_xfer_request_accepted + 438
    frame #4: 0x0000000100bc4225 libpurple`purple_xfer_choose_file_ok_cb + 506
    frame #5: 0x00000001006ef4b0 AdiumLibpurple`adiumPurpleRequestFile(title=0x0000000000000000, filename=0x00000005d6ba1fd0, savedialog=0, ok_cb=0x0000000100bc402b, cancel_cb=0x0000000100bc42a3, account=0x00000002ce687f50, who=0x00000005d6b8afe0, conv=0x0000000000000000, user_data=0x00000005d6b88f10) + 1008 at adiumPurpleRequest.m:390
    frame #6: 0x0000000100be1329 libpurple`purple_request_file + 154
    frame #7: 0x0000000100bc18f4 libpurple`purple_xfer_choose_file + 113
    frame #8: 0x0000000100bc1294 libpurple`purple_xfer_request + 273
    frame #9: 0x00000001006e69ed AdiumLibpurple`-[SLPurpleCocoaAdapter xferRequest:](self=0x00000002c23ffff0, _cmd=0x0000000100765e74, xfer=0x00000005d6b88f10) + 29 at SLPurpleCocoaAdapter.m:1496
    frame #10: 0x00000001006fa2a2 AdiumLibpurple`-[CBPurpleAccount _beginSendOfFileTransfer:](self=0x00000001e5d4be90, _cmd=0x0000000100768910, fileTransfer=0x00000005d6b1af30) + 354 at CBPurpleAccount.m:1681
    frame #11: 0x00000001007062d9 AdiumLibpurple`-[ESPurpleJabberAccount beginSendOfFileTransfer:](self=0x00000001e5d4be90, _cmd=0x00000001002754c3, fileTransfer=0x00000005d6b1af30) + 73 at ESPurpleJabberAccount.m:509
    frame #12: 0x000000010019e06e Adium`-[ESFileTransferController sendFile:toListContact:](self=0x00000001d324afc0, _cmd=0x00000001008e80cb, inPath=0x00000005cf127fa0, listContact=0x00000003c04b2ef0) + 558 at ESFileTransferController.m:393
    frame #13: 0x000000010018f2b2 Adium`-[AIContentController handleFileSendsForContentMessage:](self=0x00000001d311cfc0, _cmd=0x0000000100290480, inContentMessage=0x00000005d6a3cf90) + 1810 at AIContentController.m:664
    frame #14: 0x000000010018f542 Adium`-[AIContentController processAndSendContentObject:](self=0x00000001d311cfc0, _cmd=0x00000001002902f5, inContentObject=0x00000005d6a3cf90) + 274 at AIContentController.m:719
    frame #15: 0x000000010018db3b Adium`-[AIContentController finishSendContentObject:](self=0x00000001d311cfc0, _cmd=0x00000001002902a9, inObject=0x00000005d6a3cf90) + 251 at AIContentController.m:360
    frame #16: 0x000000010018d9d9 Adium`-[AIContentController didFilterAttributedString:contentSendingContext:](self=0x00000001d311cfc0, _cmd=0x0000000100290278, filteredString=0x00000005d6a54fe0, inObject=0x00000005d6a3cf90) + 345 at AIContentController.m:335
    frame #17: 0x00007fff8ec3709c CoreFoundation`__invoking___ + 140
    frame #18: 0x00007fff8ec36f37 CoreFoundation`-[NSInvocation invoke] + 263
    frame #19: 0x00000001001b4a8f Adium`-[AdiumContentFiltering filterAttributedString:usingFilterType:direction:filterContext:notifyingTarget:selector:context:](self=0x00000001d31fcf10, _cmd=0x00000001008e304e, attributedString=0x00000005d6a54fe0, type=AIFilterContent, direction=AIFilterOutgoing, filterContext=0x00000005d6a3cf90, target=0x00000001d311cfc0, selector=0x0000000100290278, context=0x00000005d6a3cf90) + 1407 at AdiumContentFiltering.m:368
    frame #20: 0x000000010018d2b1 Adium`-[AIContentController filterAttributedString:usingFilterType:direction:filterContext:notifyingTarget:selector:context:](self=0x00000001d311cfc0, _cmd=0x00000001008e304e, attributedString=0x00000005d6a32fe0, type=AIFilterContent, direction=AIFilterOutgoing, filterContext=0x00000005d6a3cf90, target=0x00000001d311cfc0, selector=0x0000000100290278, context=0x00000005d6a3cf90) + 145 at AIContentController.m:207
    frame #21: 0x000000010018d851 Adium`-[AIContentController sendContentObject:](self=0x00000001d311cfc0, _cmd=0x00000001008e804c, inObject=0x00000005d6a3cf90) + 273 at AIContentController.m:303
    frame #22: 0x000000010005f6bc Adium`-[AIMessageViewController sendMessage:](self=0x0000000599ed9f20, _cmd=0x00007fff90ab0816, sender=0x000000059a60ee80) + 1116 at AIMessageViewController.m:389
    frame #23: 0x00000001005fe96d AIUtilities`-[AISendingTextView sendContent:](self=0x000000059a60ee80, _cmd=0x00000001008f09ab, sender=0x0000000000000000) + 77 at AISendingTextView.m:159
    frame #24: 0x00000001008d26eb Adium`-[AIMessageEntryTextView sendContent:](self=0x000000059a60ee80, _cmd=0x00000001008f09ab, sender=0x0000000000000000) + 283 at AIMessageEntryTextView.m:902
    frame #25: 0x00000001005fe779 AIUtilities`-[AISendingTextView insertText:](self=0x000000059a60ee80, _cmd=0x00007fff964b9eeb, aString=0x00007fff7d39ac60) + 889 at AISendingTextView.m:126
    frame #26: 0x00000001008d8689 Adium`-[AIMessageEntryTextView insertText:](self=0x000000059a60ee80, _cmd=0x00007fff964b9eeb, aString=0x00007fff7d39ac60) + 73 at AIMessageEntryTextView.m:1773
    frame #27: 0x00007fff95b5b296 AppKit`-[NSTextView(NSKeyBindingCommands) insertNewline:] + 555
    frame #28: 0x00007fff95b5af6c AppKit`-[NSResponder doCommandBySelector:] + 75
    frame #29: 0x00007fff95b5adce AppKit`-[NSTextView doCommandBySelector:] + 197
    frame #30: 0x00007fff95bdde6e AppKit`-[NSKeyBindingManager(NSKeyBindingManager_MultiClients) interpretEventAsCommand:forClient:] + 2200
    frame #31: 0x00007fff95bdd2db AppKit`-[NSTextInputContext handleEvent:] + 939
    frame #32: 0x00007fff95bdcea7 AppKit`-[NSView interpretKeyEvents:] + 183
    frame #33: 0x00000001005fe910 AIUtilities`-[AISendingTextView interpretKeyEvents:](self=0x000000059a60ee80, _cmd=0x00007fff964abad5, eventArray=0x00000005d6a0afd0) + 336 at AISendingTextView.m:153
    frame #34: 0x00007fff95b29c57 AppKit`-[NSTextView keyDown:] + 723
    frame #35: 0x00000001008cf42d Adium`-[AIMessageEntryTextView keyDown:](self=0x000000059a60ee80, _cmd=0x00007fff9649b8e3, inEvent=0x00000005d69f8f60) + 2253 at AIMessageEntryTextView.m:344
    frame #36: 0x00007fff95d45020 AppKit`-[NSWindow sendEvent:] + 9687
    frame #37: 0x00007fff95d40644 AppKit`-[NSApplication sendEvent:] + 5761
    frame #38: 0x00007fff95c5621a AppKit`-[NSApplication run] + 636
    frame #39: 0x00007fff95bfabd6 AppKit`NSApplicationMain + 869
    frame #40: 0x0000000100047e12 Adium`main(argc=3, argv=0x00007fff5fbff868) + 34 at main.m:3
    frame #41: 0x0000000100002bd4 Adium`start + 52
Last edited 5 years ago by Robby (previous) (diff)

comment:2 Changed 5 years ago by datallah

  • Status changed from new to pending

This seems like a misuse of the API - why would the same pointer be passed in?

It doesn't seem worthwhile to make a single fix to this one location (and it also doesn't seem like it should be necessary to do this everywhere we have a setter function).

comment:3 Changed 5 years ago by Robby

  • Description modified (diff)

comment:4 Changed 5 years ago by fain

  • Status changed from pending to new

This setter pattern is actually very common in Objective-C (and probably other languages). Here's someone describing why it's a problem to free an object before copying it without checking if they're the same: http://stackoverflow.com/questions/12369787/correctly-override-setter-in-objective-c

The API doesn't specify that the argument can't be the same as the existing value. We *could* update the header files to include that (IMHO needless) restriction, or, just as easily, we could fix the setter implementations. And in doing so, not force Adium and other clients to perform that check themselves whenever calling a setter.

If it makes things easier for integration I could just create a patch for this entire file and attach it to the ticket; let me know.

Changed 5 years ago by fain

Here's the patch that fixes the free() calls in ft.c

comment:5 Changed 5 years ago by fain

Upon closer inspection, Adium is unable to fix this, since the problem is completely in libpurple code (the call to purple_xfer_set_local_filename comes from the "purple_xfer_request_accepted" callback within ft.c, at a much lower level than Adium has access to).

comment:6 Changed 2 years ago by Robby

  • Milestone set to Patches Needing Review

comment:7 Changed 23 months ago by Robby

  • Milestone changed from Patches Needing Review to 2.12.1
  • Resolution set to fixed
  • Status changed from new to closed

Fixed in 41ce9e8.

comment:8 Changed 16 months ago by Robby

  • Milestone changed from 2.12.1 to 2.13.0
Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!