fluidsynth 2.0.8 - macOS - audio driver crash

[stubbulon5]

Hello, release 2.0.8 seems to have introduced an audio driver crash. (macOS - coreaudio)

2.0.7 works perfectly.

[derselbst]

It fails when it tries to zero out the allocated memory. But looking at the changes, I don't see any relevant change.

[stubbulon5]

The only other clue I can give is that I was Dynamically linking the library bundled with my App.

[derselbst]

You could compile fluidsynth 2.1.0.rc1 , and try if it crashes as well. If it does, recompile with
cmake -Denable-ubsan=1 and post the output that adress sanitizer gives when fluidsynth crashes. At least on my machine, I cannot reproduce any crash, neither with 2.0.8 nor 2.1.0.rc1 .

[stubbulon5]

Thanks Tom, I'll certainly try this, but it will be in a few days time.

[lilyinstarlight]

I'm able to reproduce this on my computer (macOS 10.15.1) and 2.1.0.rc1 no longer crashed where 2.0.8 did. I tried to bisect to find the commit that first caused the crash and the commit where it no longer happened. I also compiled both the broken commit and the v2.1.0.rc1 commit with ASAN and dropped the outputs at the pastebin links since both indicate undefined behaviour.

First Commit With Crash: a94bc82 (Initialize allocated memory to garbage for debug builds.)
First Commit Without Crash: 6e45cfc (Move declaration of fluid_alloc() to fluidsynth_priv.h)

ASAN from commit a94bc82: https://paste.fooster.io/fluidsynth_591_1
ASAN from v2.1.0.rc1 tag: https://paste.fooster.io/fluidsynth_591_2

I didn't go too in-depth in the code to see why this might happen but hopefully this helps! Let me know if you want more data.

[stubbulon5]

Great to know that 2.1.0.rc1 fixes this issue! I'll give th 2.1.0.rc a go. Thanks again! I'll close this one out!

[romanbsd]

Interesting, it loses the higher part of the address of the pointer when using fluid_alloc.

(lldb) n
Process 74195 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x0000000100103ada FluidSynth`fluid_alloc(len=104) at fluid_sys.c:218:12
   215 	        memset(ptr, 0xCC, len);
   216 	    }
   217 	#endif
-> 218 	    return ptr;
   219 	}
   220 	
   221 	/**
Target 0: (fluidsynth) stopped.
(lldb) p ptr
(void *) $20 = 0x000060b000017dd0

(lldb) s
Process 74195 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step in
    frame #0: 0x00000001000eac63 FluidSynth`new_fluid_core_audio_driver2(settings=0x00006070000026a0, func=0x0000000000000000, data=0x0000613000009c00) at fluid_coreaudio.c:170:12
   167 	
   168 	    dev = FLUID_NEW(fluid_core_audio_driver_t);
   169 	
-> 170 	    if(dev == NULL)
   171 	    {
   172 	        FLUID_LOG(FLUID_ERR, "Out of memory");
   173 	        return NULL;
Target 0: (fluidsynth) stopped.
(lldb) p dev
(fluid_core_audio_driver_t *) $21 = 0x0000000000017dd0

What makes it think that it's 32 bit?

[derselbst]

@fkmclane Thanks for the detailed analysis! After you pointed me to 6e45cfc, I remembered the problem. It's one of those nasty implicit-function-delcaration bugs. I added -Werror=implicit-function-declaration on master, but forgot 2.0.x . I'll apply the commit to 2.0.x and release 2.0.9 ASAP.

it loses the higher part of the address of the pointer when using fluid_alloc. What makes it think that it's 32 bit?

In C89, implicitly declared functions default to return an int.

[lilyinstarlight]

Thanks for the quick response!

So I looked into the undefined behaviour a little bit more and I think I see where the issue is. Interestingly, this code has been around and seemingly problematic since 05ee230 (9 years ago). I guess it happened to work well enough that it wasn't an issue or no one noticed when the undefined behaviour did happen but there is a problematic logic issue here. The undefined behaviour happens if there is fewer than sizeof(AudioBufferList)/sizeof(AudioBuffer) outputs for a device. This is 24/16 on my system which corresponds to at least one output which happens to work because of integer math but if a device has fewer than that (i.e. zero outputs), the calculated value for num becomes 8/16 which is zero. I put the struct definitions below after more explanation to perhaps help understand why integer math technically saves it for at least one output but devices with no usable outputs cause undefined behaviour.

When a device does not have usable outputs, the size of the AudioBufferList needs to be 8 on my system (which is what AudioObjectGetPropertyDataSize correctly says -- enough to allocate an aligned mNumberBuffers member and zero buffers), however because this is less than sizeof(AudioBufferList) (24 on my system), the VLA for bufList has zero length after the division -- undefined behaviour by itself since a zero-length VLA doesn't make sense and especially so if data is about to be put in it.

Semantically, the AudioBufferList structure should be allocated and be by itself and not passed in as an array. It should get allocated with enough space to contain a number of buffers then the buffer array and if you allocate the whole thing as an array, you either get the same size anyway due to the definition in the macOS SDK (num = 1), double/triple/etc. up the space for mNumberBuffers even though it should technically only be allocated once (num = 2+), or do not allocate any space, not even for the mNumberBuffers member, (num = 0).

I put a possible patch that does fix the undefined behaviour below but I don't know if it completely follows the style guide and it does add a heap allocation where there wasn't one before (it was a variable length array on the stack -- at least GCC puts them on the stack and I assume Clang does too). Using malloc of the returned size is what I see examples and other software using CoreAudio doing so this seems to be a correct way to do it. I made the patch against v2.1.0.rc1 but it should apply cleanly to master. If this patch looks acceptable and applying to master is good, I would like to make a pull request against the master branch (and let you backport for 2.0.9 if you feel the need to or just let the undefined behaviour stay in 2.0.x (I mean it's been 9 years already) and test the change for 2.1.x).

Thanks again for working on FluidSynth!

Definition of AudioBufferList and AudioBuffer

/*!
    @struct         AudioBuffer
    @abstract       A structure to hold a buffer of audio data.
    @var            mNumberChannels
                        The number of interleaved channels in the buffer.
    @var            mDataByteSize
                        The number of bytes in the buffer pointed at by mData.
    @var            mData
                        A pointer to the buffer of audio data.
*/
struct AudioBuffer
{
    UInt32              mNumberChannels;
    UInt32              mDataByteSize;
    void* __nullable    mData;
};
typedef struct AudioBuffer  AudioBuffer;

/*!
    @struct         AudioBufferList
    @abstract       A variable length array of AudioBuffer structures.
    @var            mNumberBuffers
                        The number of AudioBuffers in the mBuffers array.
    @var            mBuffers
                        A variable length array of AudioBuffers.
*/
struct AudioBufferList
{
    UInt32      mNumberBuffers;
    AudioBuffer mBuffers[1]; // this is a variable length array of mNumberBuffers elements
	
#if defined(__cplusplus) && defined(CA_STRICT) && CA_STRICT
public:
    AudioBufferList() {}
private:
    //  Copying and assigning a variable length struct is problematic; generate a compile error.
    AudioBufferList(const AudioBufferList&);
    AudioBufferList&    operator=(const AudioBufferList&);
#endif

};
typedef struct AudioBufferList  AudioBufferList;

Current src/drivers/fluid_coreaudio.c:get_num_outputs

int
get_num_outputs(AudioDeviceID deviceID)
{
    int i, total = 0;
    UInt32 size;
    AudioObjectPropertyAddress pa;
    pa.mSelector = kAudioDevicePropertyStreamConfiguration;
    pa.mScope = kAudioDevicePropertyScopeOutput;
    pa.mElement = kAudioObjectPropertyElementMaster;

    if(OK(AudioObjectGetPropertyDataSize(deviceID, &pa, 0, 0, &size)))
    {
        int num = size / (int) sizeof(AudioBufferList);
        AudioBufferList bufList[num];

        if(OK(AudioObjectGetPropertyData(deviceID, &pa, 0, 0, &size, bufList)))
        {
            int numStreams = bufList->mNumberBuffers;

            for(i = 0; i < numStreams; ++i)
            {
                AudioBuffer b = bufList->mBuffers[i];
                total += b.mNumberChannels;
            }
        }
    }

    return total;
}

Possible Patch

diff --git a/src/drivers/fluid_coreaudio.c b/src/drivers/fluid_coreaudio.c
index baeada5..b6be555 100644
--- a/src/drivers/fluid_coreaudio.c
+++ b/src/drivers/fluid_coreaudio.c
@@ -79,14 +79,20 @@ get_num_outputs(AudioDeviceID deviceID)
     int i, total = 0;
     UInt32 size;
     AudioObjectPropertyAddress pa;
+    AudioBufferList *bufList;
     pa.mSelector = kAudioDevicePropertyStreamConfiguration;
     pa.mScope = kAudioDevicePropertyScopeOutput;
     pa.mElement = kAudioObjectPropertyElementMaster;
 
     if(OK(AudioObjectGetPropertyDataSize(deviceID, &pa, 0, 0, &size)))
     {
-        int num = size / (int) sizeof(AudioBufferList);
-        AudioBufferList bufList[num];
+        bufList = FLUID_MALLOC(size);
+
+        if(bufList == NULL)
+        {
+            FLUID_LOG(FLUID_ERR, "Out of memory");
+            return 0;
+        }
 
         if(OK(AudioObjectGetPropertyData(deviceID, &pa, 0, 0, &size, bufList)))
         {
@@ -98,6 +104,8 @@ get_num_outputs(AudioDeviceID deviceID)
                 total += b.mNumberChannels;
             }
         }
+
+        FLUID_FREE(bufList);
     }
 
     return total;

[derselbst]

@fkmclane Thanks for your explanations. @romanbsd Just came accross this behaviour as well and tried to fix it: #593 .

So to simplify things: From my understanding, AudioObjectGetPropertyDataSize returns the number of bytes that AudioBufferList::mNumberBuffers plus the VLA member AudioBufferList::mBuffers will allocate. And in case there are no such audio buffers, fluidsynth's current implementation doesn't allocate anything, whereas your implementation at least allocates size bytes, while hoping that

size >= sizeof(AudioBufferList::mNumberBuffers) .

Since this hope might fail apparently, I would say that it is, technically, illegal to cast the memory returned by malloc to type AudioBufferList *, since the allocated memory may be smaller than sizeof(AudioBufferList).

In order to turn this hope into a guarantee, how about

-    if(OK(AudioObjectGetPropertyDataSize(deviceID, &pa, 0, 0, &size)))
+    if(OK(AudioObjectGetPropertyDataSize(deviceID, &pa, 0, 0, &size)) && size >= (sizeof(AudioBufferList) - FLUID_MEMBER_SIZE(AudioBufferList, mBuffers)) )

to avoid that accessing bufList->mNumberBuffers; (or any other, though non-existent, members of that struct) result in a heap-based overflow.

@fkmclane If you can successfully test your patch along with my changes on macOS and make a PR, I will happily include it in 2.0.9 . (Just leave the declaration of AudioBufferList *bufList; in the inner scope ;) )

[lilyinstarlight]

size >= sizeof(AudioBufferList::mNumberBuffers).

Since this hope might fail apparently, I would say that it is, technically, illegal to cast the memory returned by malloc to type AudioBufferList *, since the allocated memory may be smaller than sizeof(AudioBufferList).

If the call succeeds, I'm not finding anywhere that it would be less than the size of the struct with mNumberBuffers and mBuffers being empty.

For casting to AudioBufferList *, technically the definition is fixed size (defined with only 1 element in mBuffers) despite it semantically being variable but it seems functions like AudioObjectGetPropertyData expect that behaviour. I'm actually having trouble finding much good documentation on these besides usage in other software (e.g. OBS, RtAudio, Chromium, etc.). It seems the purpose of AudioObjectGetPropertyDataSize is to give the size of the structure that AudioObjectGetPropertyData will fill, irrelevant of the fixed-size definition, and assuming the call succeeds (indicated by the Ok() macro) the size should be enough for required data (including mNumberBuffers member).

I do notice, however, that a few other projects (not all I found) check that the size is non-zero before allocating in addition to checking that the call succeeded so I'll put that in the PR.

I could very well be misunderstanding so if you do have documentation somewhere on the usage of these functions, I would be appreciative. I have only done minimal macOS dev and the usage of some of these kits (like CoreAudio) from C is still relatively foreign to me. I'll also open a PR and we can do review and comments there if you'd like.

References from other projects:
https://github.com/obsproject/obs-studio/blob/master/plugins/mac-capture/mac-audio.c#L295
https://github.com/thestk/rtaudio/blob/master/RtAudio.cpp#L738
https://chromium.googlesource.com/chromium/src/+/refs/heads/master/media/audio/mac/audio_manager_mac.cc#261

[derselbst]

I'm not finding anywhere that it would be less than the size of the struct with mNumberBuffers and mBuffers being empty.

I also don't think that it would return a size less than sizeof(mNumberBuffers). On the other hand I would have expected it to return at least sizeof(AudioBufferList) (even if that single buffer element would be uninitialized by a subsequent call to AudioObjectGetPropertyData()). So, I was just trying to be paranoid on the size, esp. because I'm lacking documentation as well.

[lilyinstarlight]

Yeah, I think Apple putting a size of 1 for the dynamic array in the struct kinda breaks good programming practice when the number of buffers is zero since the size of memory at the struct pointer will be less than the struct. Conversely, when the number of buffers is more than one, the amount of data being filled will be larger than the size of the struct. I tried to keep it within how Apple expects/requires it to be used judging by other projects that use CoreAudio. I opened the PR at #594 so I'll let you make comments/review there for code changes.

Thanks again!