pkg/41171: audio/timidity Sun audio bug

[mag%netherworld.org@localhost]

>Number:         41171
>Category:       pkg
>Synopsis:       mismatched calling convention with Sun audio device
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    pkg-manager
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Apr 08 08:05:00 +0000 2009
>Originator:     John E. Krokes
>Release:        NetBSD 5.0_RC3 200903200521Z
>Organization:
>Environment:
System: NetBSD test3.netherworld.org 5.0_RC3 NetBSD 5.0_RC3 (GENERIC.MP) #0: 
Fri Mar 20 18:16:50 UTC 2009 
builds%b4.netbsd.org@localhost:/home/builds/ab/netbsd-5-0-RC3/sparc64/200903200521Z-obj/home/builds/ab/netbsd-5-0-RC3/src/sys/arch/sparc64/compile/GENERIC.MP
 sparc64
Architecture: sparc64
Machine: sparc64
>Description:
        In timidity/aq.c, in aq_setup(), there is a function call to acntl().

            if(play_mode->acntl(PM_REQ_GETFRAGSIZ, &bucket_size) == -1)
                bucket_size = audio_buffer_size * Bps;
            bucket_time = (double)bucket_size / Bps / play_mode->rate;

        As I read this, it says:
                Call acntl, passing in a constant and a variable by reference.

                If acntl() returns -1 (for failure),
                        set bucket_size to a default value,
                else
                        ignore the return value and do not set bucket_size.
                
                Perform a calculation based on the value of bucket_size.

        So, one presumes that acntl() is supposed to modify bucket_size in
        place if it succeeds.


        When using a Sun audio device, the relevant code is in
        timidity/sun_a.c, in acntl().

                case PM_REQ_GETFRAGSIZ:
                  if(ioctl(audioctl_fd, AUDIO_GETINFO, &auinfo) < 0)
                      return -1;
                  return auinfo.blocksize;

        Clearly, if the ioctl() succeeds, it will return a value to the
        caller instead of modifying a variable.


        Thus, bucket_size is not set to a meaningful value but is
        instead used uninitialized in subsequent calculations. 

        Further on in the code, this leads to division-by-zero, and eventually
        timidity exits after printing this error message:

                Strange, I feel like allocating -24 bytes. This must be a bug.
        

        
>How-To-Repeat:
        timidity -id -Od anyfile.mid
>Fix:
        workaround:
                timidity -id -Ou -o anyfile.au anyfile.mid
                cat anyfile.au > /dev/audio


        Or, apply this patch:


-----cut-here-----
*** timidity/sun_a.c.orig       Tue Apr  7 17:04:26 2009
--- timidity/sun_a.c    Tue Apr  7 17:05:53 2009
***************
*** 345,356 ****
        case PM_REQ_GETQSIZ:
        if(ioctl(audioctl_fd, AUDIO_GETINFO, &auinfo) < 0)
            return -1;
!       return auinfo.play.buffer_size;
  
        case PM_REQ_GETFRAGSIZ:
        if(ioctl(audioctl_fd, AUDIO_GETINFO, &auinfo) < 0)
            return -1;
!       return auinfo.blocksize;
  
        case PM_REQ_OUTPUT_FINISH:
        return ioctl(audioctl_fd, AUDIO_DRAIN, NULL);
--- 345,358 ----
        case PM_REQ_GETQSIZ:
        if(ioctl(audioctl_fd, AUDIO_GETINFO, &auinfo) < 0)
            return -1;
!       *((int *)arg) = auinfo.play.buffer_size;
!       return 0;
  
        case PM_REQ_GETFRAGSIZ:
        if(ioctl(audioctl_fd, AUDIO_GETINFO, &auinfo) < 0)
            return -1;
!       *((int *)arg) = auinfo.blocksize;
!       return 0;
  
        case PM_REQ_OUTPUT_FINISH:
        return ioctl(audioctl_fd, AUDIO_DRAIN, NULL);
-----cut-here-----



        This patch modifies code added by patch-aa in pkgsrc-2008Q4. Perhaps
        it would be smarter to modify the patch rather than patch the same
        code twice.