Bug 433756 - timidity++ -d0 crashes
Summary: timidity++ -d0 crashes
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: timidity++
Version: rawhide
Hardware: All
OS: Linux
high
high
Target Milestone: ---
Assignee: Jindrich Novy
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-02-21 10:16 UTC by Andrew Bartlett
Modified: 2013-07-02 23:26 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2008-02-21 12:13:23 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Patch to fix the segfault (335 bytes, patch)
2008-02-21 10:16 UTC, Andrew Bartlett
no flags Details | Diff
patch for the spec file too! (1.08 KB, patch)
2008-02-21 10:17 UTC, Andrew Bartlett
no flags Details | Diff

Description Andrew Bartlett 2008-02-21 10:16:50 UTC
Description of problem:
timidity crashes due to free() of constant pointer

Version-Release number of selected component (if applicable):
TiMidity++-2.13.2

How reproducible:
Every time

Steps to Reproduce:
1. timidity -d0 foo.midi
2. 
3.
  
Actual results:
glibc detects double-free

Expected results:
midi playing

Additional info:
Perhaps an audit to find other cases might be worth while.

I've chosen to 'leak' a very small amount of memory in the rare case that -d is
specified twice.

Comment 1 Andrew Bartlett 2008-02-21 10:16:50 UTC
Created attachment 295494 [details]
Patch to fix the segfault

Comment 2 Andrew Bartlett 2008-02-21 10:17:47 UTC
Created attachment 295495 [details]
patch for the spec file too!

Comment 3 Jindrich Novy 2008-02-21 12:00:34 UTC
The spec file patch was not cleanly applicable due to recent changes to
timidity++ so I modified it.

Maybe a better solution would be to originally strdup the constant string
otherwise the dynamic_lib_root value would be leaked.

I rewrote your patch to accomodate that.

Thanks!

Comment 4 Andrew Bartlett 2008-02-21 21:21:13 UTC
Commenting only in the interests of computer science:

How would we have a leak?  We can certainly use more memory - if we always keep
two copies of the string - the static const default string, and the strdup()
default copy. 

The only 'leak' I can see is if a user specifies -d twice, and if they do that,
they get what they deserve :-)

Comment 5 Jindrich Novy 2008-02-22 09:35:36 UTC
It's better to add code that is safe by principle than code that is safe only
after some assumptions if it is not hard or performance problematic. Consider
that upstream may change the code in a way that it uses dynamic_lib_root on some
other places than now, the problem you describe is still fixed, but leaks could
have been silently introduced.


Note You need to log in before you can comment on or make changes to this bug.