Bug 433756

Summary: timidity++ -d0 crashes
Product: [Fedora] Fedora Reporter: Andrew Bartlett <abartlet>
Component: timidity++Assignee: Jindrich Novy <jnovy>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: high Docs Contact:
Priority: high    
Version: rawhideCC: hdegoede, pknirsch
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-02-21 12:13:23 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Patch to fix the segfault
none
patch for the spec file too! none

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.