Ticket #84 (closed defect: fixed)

Opened 16 years ago

Last modified 10 years ago

xdsc crashes on adding a meeting

Reported by: andersk Owned by:
Priority: trivial Milestone: The Distant Future
Component: -- Keywords:
Cc: Fixed in version:
Upstream bug:


After adding the sipb-ec meeting with xdsc on linerva, it crashed with

*** glibc detected *** corrupted double-linked list: 0xf7de74f8 ***

(Fortunately, the meeting was successfully added.)

Change History

comment:1 Changed 15 years ago by geofft

  • Status changed from new to closed
  • Resolution set to fixed

The crash also happens when removing a meeting.

I've avoided the crash by commenting out three calls to free in reply.c (see r23577). I'm not sure if I'm actually avoiding a double-free, or just avoiding some stupidity and instead leaking memory, but a potential memory leak of a short string every time you add or remove a meeting is very hard for me to care about.

Someone should rewrite the whole thing in Gtk+. Or, you know, write ds2imap or ds2nntp so I can just use my favorite mail/news reader.

comment:2 Changed 15 years ago by andersk

  • Status changed from closed to reopened
  • Resolution fixed deleted

debathena / r23577 / andersk 14:14 (Anders Kaseorg)

It seems pretty clear to me that there are no pointers to those
tempstrings lying around after the free()s you commented. I’m
guessing that something else thinks it’s using the same space. Did
you try valgrind?

debathena / r23577 / andersk 14:15 (Anders Kaseorg)

Replacing an easy-to-reproduce crash with a hard-to-reproduce subtle
memory corruption bug is not an improvement.

comment:3 Changed 15 years ago by broder

  • Keywords proposed added

Tagging as "proposed" because there currently is a package in -proposed.

However, I'm convinced by what Anders says that this patch doesn't understand and fully fix the issue, so unless someone is planning to fix this in the near future, I think we should revert r23577 and upload another release that we can move into production.

comment:4 Changed 15 years ago by broder

r23643 reverts r23577, and is sitting in -proposed. Barring objections, I'll move it to production late Tuesday afternoon.

Someone confirming that it's no more broken than what's currently in production would be nice.

comment:5 Changed 15 years ago by broder

  • Keywords proposed removed
  • Component set to default

I moved the reverted package to production, so I'm stripping the proposed tag.

comment:6 Changed 15 years ago by jdreed

  • Priority changed from minor to low
  • Component set to --
  • Milestone set to The Distant Future

comment:7 Changed 13 years ago by jdreed

So, I went and ran valgrind on xdsc, and based on my (possibly naive) interpretation of the output, it looks like Geoff's patch was right?

==21275== Invalid free() / delete / delete[]
==21275==    at 0x4C282ED: free (vg_replace_malloc.c:366)

==21275==  Address 0x701e750 is 0 bytes inside a block of size 17 free'd
==21275==    at 0x4C282ED: free (vg_replace_malloc.c:366)
==21275==    by 0x4043E4: myfree (xdsc.c:1225)
==21275==    by 0x4071AD: AddCB (reply.c:715)

Memory management in C is definitely not my thing, though. Unless there's any real interest in fixing this, I'm just going to go write a pygtk version.

comment:8 Changed 11 years ago by jdreed

Out of curiousity, I poked at this some more. I am not convinced geofft's patch was wrong. Not only does valgrind back me up, but buried deep within the Motif programming manual is a note that "An application should free compound strings retrieved by XtGetValues?, but in general it should not free values of other types unless the documentation for the particular resource [...] says the application must free that value." And in fact the documentation does not say the address passed to XtSetArg? must be free'd. (We're not using Motif compound strings here).

comment:10 Changed 11 years ago by jdreed

  • Status changed from reopened to review

Note that this requires the latest changes to python-discuss.

comment:11 Changed 10 years ago by jdreed

OK, added Debian packaging for this, and merged master into debian on my checkout.


comment:12 Changed 10 years ago by jdreed

  • Status changed from review to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.