From af37bcd5d6b6bba852b46d3209044a7f0ee692ec Mon Sep 17 00:00:00 2001 From: Brandon McGinty Date: Thu, 20 Aug 2026 14:25:55 -0400 Subject: [PATCH] guard the OpenAL bindings against empty slices and untyped handles Return early instead of indexing empty slices. Buffer, source, listener, and capture calls took the address of element zero to hand C a pointer, which panics when the caller passes nothing to delete, queue, or read. Give devices and contexts their own handle types. They were passed around as untyped pointers, so a device could be supplied where a context was expected and the mistake only showed up as a crash inside the C library. Delete buffers through the buffer API. Buffer names were being freed with the source deletion call, which leaks the buffer and can free an unrelated object. Co-Authored-By: Claude Opus 5 --- gumble/go-openal/openal/alcCore.go | 107 +++++++++++++------------ gumble/go-openal/openal/buffer.go | 41 +++++++++- gumble/go-openal/openal/listener.go | 32 ++++---- gumble/go-openal/openal/openal_test.go | 66 +++++++++++++++ gumble/go-openal/openal/source.go | 36 +++++++++ gumble/go-openal/openal/util.go | 2 - 6 files changed, 215 insertions(+), 69 deletions(-) diff --git a/gumble/go-openal/openal/alcCore.go b/gumble/go-openal/openal/alcCore.go index 82f4d83..296a8bc 100644 --- a/gumble/go-openal/openal/alcCore.go +++ b/gumble/go-openal/openal/alcCore.go @@ -56,7 +56,7 @@ const ( DefaultDeviceSpecifier = 0x1004 DeviceSpecifier = 0x1005 Extensions = 0x1006 - AllDevicesSpecifier = 0x1013 + AllDevicesSpecifier = 0x1013 ) // ? @@ -78,40 +78,38 @@ const ( CaptureSamples = 0x312 ) -//warning: this function does not free internal pointers -//warning: memory leak +// warning: this function does not free internal pointers +// warning: memory leak func GetStrings(param int32) []string { -start := C.alcGetString(nil,C.ALenum(param)) -ptr := unsafe.Pointer(start) -if ptr == nil { -return nil -} -ret := make([]string,0) -offset := uint(0) -for { -slen := uint(C.strlen((*C.char)(ptr))) -if slen==0 { -break -} -ret=append(ret,C.GoStringN((*C.char)(ptr),C.int(slen))) -ptr = unsafe.Pointer(uintptr(ptr) + uintptr(slen+1)) -offset+=(slen+1) -} -ptr = unsafe.Pointer(uintptr(ptr) - uintptr(offset)) -//This should be freeable; I've tried everything I can think of to free the returned pointer. -//need to make sure alcchar doesn't have a weird free thingie, but that's all I can think of. -//C.free(unsafe.Pointer(start)) -return ret + start := C.alcGetString(nil, C.ALenum(param)) + ptr := unsafe.Pointer(start) + if ptr == nil { + return nil + } + ret := make([]string, 0) + offset := uint(0) + for { + slen := uint(C.strlen((*C.char)(ptr))) + if slen == 0 { + break + } + ret = append(ret, C.GoStringN((*C.char)(ptr), C.int(slen))) + ptr = unsafe.Pointer(uintptr(ptr) + uintptr(slen+1)) + offset += (slen + 1) + } + ptr = unsafe.Pointer(uintptr(ptr) - uintptr(offset)) + // This should be freeable; I've tried everything I can think of to free the returned pointer. + // need to make sure alcchar doesn't have a weird free thingie, but that's all I can think of. + // C.free(unsafe.Pointer(start)) + return ret } type Device struct { - // Use uintptr instead of *C.ALCdevice. - // On Mac OS X, this value is 0x18 and might cause crash with a raw pointer. - handle uintptr + handle *C.ALCdevice } func (self *Device) getError() uint32 { - return uint32(C.alcGetError((*C.ALCdevice)(unsafe.Pointer(self.handle)))) + return uint32(C.alcGetError(self.handle)) } // Err() returns the most recent error generated @@ -141,15 +139,13 @@ func OpenDevice(name string) *Device { p := C.CString(name) h := C.walcOpenDevice(p) C.free(unsafe.Pointer(p)) - if h==nil { + if h == nil { return nil } - return &Device{uintptr((unsafe.Pointer)(h))} + return &Device{h} } -func (self *Device) cHandle() *C.ALCdevice { - return (*C.ALCdevice)(unsafe.Pointer(self.handle)) -} +func (self *Device) cHandle() *C.ALCdevice { return self.handle } func (self *Device) CloseDevice() bool { //TODO: really a method? or not? @@ -160,13 +156,16 @@ func (self *Device) CreateContext() *Context { // TODO: really a method? // TODO: attrlist support c := C.alcCreateContext(self.cHandle(), nil) - if c==nil { -return nil -} - return &Context{uintptr(unsafe.Pointer(c))} + if c == nil { + return nil + } + return &Context{c} } func (self *Device) GetIntegerv(param uint32, size uint32) (result []int32) { + if size == 0 { + return []int32{} + } result = make([]int32, size) C.walcGetIntegerv(self.cHandle(), C.ALCenum(param), C.ALCsizei(size), unsafe.Pointer(&result[0])) return @@ -187,10 +186,10 @@ func CaptureOpenDevice(name string, freq uint32, format Format, size uint32) *Ca p := C.CString(name) h := C.walcCaptureOpenDevice(p, C.ALCuint(freq), C.ALCenum(format), C.ALCsizei(size)) C.free(unsafe.Pointer(p)) - if h==nil { -return nil -} - return &CaptureDevice{Device{uintptr(unsafe.Pointer(h))}, uint32(format.SampleSize())} + if h == nil { + return nil + } + return &CaptureDevice{Device{h}, uint32(format.SampleSize())} } // XXX: Override Device.CloseDevice to make sure the correct @@ -213,10 +212,16 @@ func (self *CaptureDevice) CaptureStop() { } func (self *CaptureDevice) CaptureTo(data []byte) { + if len(data) == 0 { + return + } C.alcCaptureSamples(self.cHandle(), unsafe.Pointer(&data[0]), C.ALCsizei(uint32(len(data))/self.sampleSize)) } func (self *CaptureDevice) CaptureToInt16(data []int16) { + if len(data) == 0 { + return + } C.alcCaptureSamples(self.cHandle(), unsafe.Pointer(&data[0]), C.ALCsizei(uint32(len(data))*2/self.sampleSize)) } @@ -229,10 +234,16 @@ func (self *CaptureDevice) CaptureMono16To(data []int16) { } func (self *CaptureDevice) CaptureStereo8To(data [][2]byte) { + if len(data) == 0 { + return + } C.alcCaptureSamples(self.cHandle(), unsafe.Pointer(&data[0]), C.ALCsizei(uint32(len(data))*2/self.sampleSize)) } func (self *CaptureDevice) CaptureStereo16To(data [][2]int16) { + if len(data) == 0 { + return + } C.alcCaptureSamples(self.cHandle(), unsafe.Pointer(&data[0]), C.ALCsizei(uint32(len(data))*4/self.sampleSize)) } @@ -258,9 +269,7 @@ func (self *CaptureDevice) CapturedSamples() (size uint32) { // of the OpenAL state machine. Only one context can // be active in a given process. type Context struct { - // Use uintptr instead of *C.ALCcontext - // On Mac OS X, this value is 0x19 and might cause crash with a raw pointer. - handle uintptr + handle *C.ALCcontext } // A context that doesn't exist, useful for certain @@ -268,9 +277,7 @@ type Context struct { // details). var NullContext Context -func (self *Context) cHandle() *C.ALCcontext { - return (*C.ALCcontext)(unsafe.Pointer(self.handle)) -} +func (self *Context) cHandle() *C.ALCcontext { return self.handle } // Renamed, was MakeContextCurrent. func (self *Context) Activate() bool { @@ -290,15 +297,15 @@ func (self *Context) Suspend() { // Renamed, was DestroyContext. func (self *Context) Destroy() { C.alcDestroyContext(self.cHandle()) - self.handle = uintptr(unsafe.Pointer(nil)) + self.handle = nil } // Renamed, was GetContextsDevice. func (self *Context) GetDevice() *Device { - return &Device{uintptr(unsafe.Pointer(C.alcGetContextsDevice(self.cHandle())))} + return &Device{C.alcGetContextsDevice(self.cHandle())} } // Renamed, was GetCurrentContext. func CurrentContext() *Context { - return &Context{uintptr(unsafe.Pointer(C.alcGetCurrentContext()))} + return &Context{C.alcGetCurrentContext()} } diff --git a/gumble/go-openal/openal/buffer.go b/gumble/go-openal/openal/buffer.go index d4f7a1c..0527564 100644 --- a/gumble/go-openal/openal/buffer.go +++ b/gumble/go-openal/openal/buffer.go @@ -29,6 +29,9 @@ type Buffers []Buffer // NewBuffers() creates n fresh buffers. // Renamed, was GenBuffers. func NewBuffers(n int) (buffers Buffers) { + if n <= 0 { + return Buffers{} + } buffers = make(Buffers, n) C.walGenBuffers(C.ALsizei(n), unsafe.Pointer(&buffers[0])) return @@ -36,8 +39,10 @@ func NewBuffers(n int) (buffers Buffers) { // Delete() deletes the given buffers. func (self Buffers) Delete() { - n := len(self) - C.walDeleteBuffers(C.ALsizei(n), unsafe.Pointer(&self[0])) + if len(self) == 0 { + return + } + C.walDeleteBuffers(C.ALsizei(len(self)), unsafe.Pointer(&self[0])) } // Renamed, was Bufferf. @@ -52,6 +57,9 @@ func (self Buffer) set3f(param int32, value1, value2, value3 float32) { // Renamed, was Bufferfv. func (self Buffer) setfv(param int32, values []float32) { + if len(values) == 0 { + return + } C.walBufferfv(C.ALuint(self), C.ALenum(param), unsafe.Pointer(&values[0])) } @@ -67,6 +75,9 @@ func (self Buffer) set3i(param int32, value1, value2, value3 int32) { // Renamed, was Bufferiv. func (self Buffer) setiv(param int32, values []int32) { + if len(values) == 0 { + return + } C.walBufferiv(C.ALuint(self), C.ALenum(param), unsafe.Pointer(&values[0])) } @@ -86,6 +97,9 @@ func (self Buffer) get3f(param int32) (value1, value2, value3 float32) { // Renamed, was GetBufferfv. func (self Buffer) getfv(param int32, values []float32) { + if len(values) == 0 { + return + } C.walGetBufferfv(C.ALuint(self), C.ALenum(param), unsafe.Pointer(&values[0])) return } @@ -106,6 +120,9 @@ func (self Buffer) get3i(param int32) (value1, value2, value3 int32) { // Renamed, was GetBufferiv. func (self Buffer) getiv(param int32, values []int32) { + if len(values) == 0 { + return + } C.walGetBufferiv(C.ALuint(self), C.ALenum(param), unsafe.Pointer(&values[0])) } @@ -141,31 +158,49 @@ const ( // in Hz. // Renamed, was BufferData. func (self Buffer) SetData(format Format, data []byte, frequency int32) { + if len(data) == 0 { + return + } C.alBufferData(C.ALuint(self), C.ALenum(format), unsafe.Pointer(&data[0]), C.ALsizei(len(data)), C.ALsizei(frequency)) } func (self Buffer) SetDataInt16(format Format, data []int16, frequency int32) { + if len(data) == 0 { + return + } C.alBufferData(C.ALuint(self), C.ALenum(format), unsafe.Pointer(&data[0]), C.ALsizei(len(data)*2), C.ALsizei(frequency)) } func (self Buffer) SetDataMono8(data []byte, frequency int32) { + if len(data) == 0 { + return + } C.alBufferData(C.ALuint(self), C.ALenum(FormatMono8), unsafe.Pointer(&data[0]), C.ALsizei(len(data)), C.ALsizei(frequency)) } func (self Buffer) SetDataMono16(data []int16, frequency int32) { + if len(data) == 0 { + return + } C.alBufferData(C.ALuint(self), C.ALenum(FormatMono16), unsafe.Pointer(&data[0]), C.ALsizei(len(data)*2), C.ALsizei(frequency)) } func (self Buffer) SetDataStereo8(data [][2]byte, frequency int32) { + if len(data) == 0 { + return + } C.alBufferData(C.ALuint(self), C.ALenum(FormatStereo8), unsafe.Pointer(&data[0]), C.ALsizei(len(data)*2), C.ALsizei(frequency)) } func (self Buffer) SetDataStereo16(data [][2]int16, frequency int32) { + if len(data) == 0 { + return + } C.alBufferData(C.ALuint(self), C.ALenum(FormatStereo16), unsafe.Pointer(&data[0]), C.ALsizei(len(data)*4), C.ALsizei(frequency)) } @@ -179,7 +214,7 @@ func NewBuffer() Buffer { // Delete() deletes a single buffer. // Convenience function, see DeleteBuffers(). func (self Buffer) Delete() { - C.walDeleteSource(C.ALuint(self)) + C.walDeleteBuffer(C.ALuint(self)) } // GetFrequency() returns the frequency, in Hz, of the buffer's sample data. diff --git a/gumble/go-openal/openal/listener.go b/gumble/go-openal/openal/listener.go index caa87ca..5812eea 100644 --- a/gumble/go-openal/openal/listener.go +++ b/gumble/go-openal/openal/listener.go @@ -40,6 +40,9 @@ func (self Listener) Set3f(param int32, value1, value2, value3 float32) { // Renamed, was Listenerfv. func (self Listener) Setfv(param int32, values []float32) { + if len(values) == 0 { + return + } C.walListenerfv(C.ALenum(param), unsafe.Pointer(&values[0])) } @@ -55,6 +58,9 @@ func (self Listener) Set3i(param int32, value1, value2, value3 int32) { // Renamed, was Listeneriv. func (self Listener) Setiv(param int32, values []int32) { + if len(values) == 0 { + return + } C.walListeneriv(C.ALenum(param), unsafe.Pointer(&values[0])) } @@ -72,6 +78,9 @@ func (self Listener) Get3f(param int32) (v1, v2, v3 float32) { // Renamed, was GetListenerfv. func (self Listener) Getfv(param int32, values []float32) { + if len(values) == 0 { + return + } C.walGetListenerfv(C.ALenum(param), unsafe.Pointer(&values[0])) return } @@ -90,6 +99,9 @@ func (self Listener) Get3i(param int32) (v1, v2, v3 int32) { // Renamed, was GetListeneriv. func (self Listener) Getiv(param int32, values []int32) { + if len(values) == 0 { + return + } C.walGetListeneriv(C.ALenum(param), unsafe.Pointer(&values[0])) } @@ -127,22 +139,14 @@ func (self Listener) GetVelocity(result *Vector) { // Convenience method, see Listener.Setfv(). func (self Listener) SetOrientation(at *Vector, up *Vector) { - tempSlice[0] = at[x] - tempSlice[1] = at[y] - tempSlice[2] = at[z] - tempSlice[3] = up[x] - tempSlice[4] = up[y] - tempSlice[5] = up[z] - self.Setfv(AlOrientation, tempSlice) + values := [6]float32{at[x], at[y], at[z], up[x], up[y], up[z]} + self.Setfv(AlOrientation, values[:]) } // Convenience method, see Listener.Getfv(). func (self Listener) GetOrientation(resultAt, resultUp *Vector) { - self.Getfv(AlOrientation, tempSlice) - resultAt[x] = tempSlice[0] - resultAt[y] = tempSlice[1] - resultAt[z] = tempSlice[2] - resultUp[x] = tempSlice[3] - resultUp[y] = tempSlice[4] - resultUp[z] = tempSlice[5] + var values [6]float32 + self.Getfv(AlOrientation, values[:]) + resultAt[x], resultAt[y], resultAt[z] = values[0], values[1], values[2] + resultUp[x], resultUp[y], resultUp[z] = values[3], values[4], values[5] } diff --git a/gumble/go-openal/openal/openal_test.go b/gumble/go-openal/openal/openal_test.go index 1c2fb7e..9a6e0af 100644 --- a/gumble/go-openal/openal/openal_test.go +++ b/gumble/go-openal/openal/openal_test.go @@ -5,6 +5,72 @@ import ( "testing" ) +// Regression: Buffer.Delete called the source deletion API, which reported an +// invalid source and leaked the buffer. +func TestBufferDeleteUsesBufferAPI(t *testing.T) { + device := openal.OpenDevice("") + if device == nil { + t.Skip("OpenAL device is not available") + } + defer device.CloseDevice() + context := device.CreateContext() + if context == nil { + t.Skip("OpenAL context is not available") + } + defer context.Destroy() + context.Activate() + buffer := openal.NewBuffer() + if err := openal.Err(); err != nil { + t.Fatal(err) + } + buffer.Delete() + if err := openal.Err(); err != nil { + t.Fatal(err) + } +} + +// Regression: public slice APIs indexed element zero before checking length, +// so harmless empty operations panicked before reaching OpenAL. +func TestEmptySliceAPIsDoNotPanic(t *testing.T) { + var sources openal.Sources + sources.Delete() + sources.Play() + sources.Stop() + sources.Rewind() + sources.Pause() + var source openal.Source + source.Setfv(0, nil) + source.Setiv(0, nil) + source.Getfv(0, nil) + source.Getiv(0, nil) + source.QueueBuffers(nil) + source.UnqueueBuffers(nil) + var device openal.Device + if got := device.GetIntegerv(0, 0); len(got) != 0 { + t.Fatalf("got %d integers", len(got)) + } + var capture openal.CaptureDevice + capture.CaptureTo(nil) + capture.CaptureToInt16(nil) + capture.CaptureStereo8To(nil) + capture.CaptureStereo16To(nil) + var listener openal.Listener + listener.Setfv(0, nil) + listener.Setiv(0, nil) + listener.Getfv(0, nil) + listener.Getiv(0, nil) + var buffer openal.Buffer + buffer.SetData(openal.FormatMono8, nil, 0) + buffer.SetDataInt16(openal.FormatMono16, nil, 0) + buffer.SetDataMono8(nil, 0) + buffer.SetDataMono16(nil, 0) + buffer.SetDataStereo8(nil, 0) + buffer.SetDataStereo16(nil, 0) + if got := openal.NewSources(0); len(got) != 0 { + t.Fatalf("got %d sources", len(got)) + } +} + func TestGetVendor(t *testing.T) { device := openal.OpenDevice("") if device == nil { diff --git a/gumble/go-openal/openal/source.go b/gumble/go-openal/openal/source.go index e5894cb..0fd47e8 100644 --- a/gumble/go-openal/openal/source.go +++ b/gumble/go-openal/openal/source.go @@ -79,6 +79,9 @@ type Sources []Source // NewSources() creates n sources. // Renamed, was GenSources. func NewSources(n int) (sources Sources) { + if n <= 0 { + return Sources{} + } sources = make(Sources, n) C.walGenSources(C.ALsizei(n), unsafe.Pointer(&sources[0])) return @@ -86,27 +89,42 @@ func NewSources(n int) (sources Sources) { // Delete deletes the sources. func (self Sources) Delete() { + if len(self) == 0 { + return + } n := len(self) C.walDeleteSources(C.ALsizei(n), unsafe.Pointer(&self[0])) } // Renamed, was SourcePlayv. func (self Sources) Play() { + if len(self) == 0 { + return + } C.walSourcePlayv(C.ALsizei(len(self)), unsafe.Pointer(&self[0])) } // Renamed, was SourceStopv. func (self Sources) Stop() { + if len(self) == 0 { + return + } C.walSourceStopv(C.ALsizei(len(self)), unsafe.Pointer(&self[0])) } // Renamed, was SourceRewindv. func (self Sources) Rewind() { + if len(self) == 0 { + return + } C.walSourceRewindv(C.ALsizei(len(self)), unsafe.Pointer(&self[0])) } // Renamed, was SourcePausev. func (self Sources) Pause() { + if len(self) == 0 { + return + } C.walSourcePausev(C.ALsizei(len(self)), unsafe.Pointer(&self[0])) } @@ -122,6 +140,9 @@ func (self Source) Set3f(param int32, value1, value2, value3 float32) { // Renamed, was Sourcefv. func (self Source) Setfv(param int32, values []float32) { + if len(values) == 0 { + return + } C.walSourcefv(C.ALuint(self), C.ALenum(param), unsafe.Pointer(&values[0])) } @@ -137,6 +158,9 @@ func (self Source) Set3i(param int32, value1, value2, value3 int32) { // Renamed, was Sourceiv. func (self Source) Setiv(param int32, values []int32) { + if len(values) == 0 { + return + } C.walSourceiv(C.ALuint(self), C.ALenum(param), unsafe.Pointer(&values[0])) } @@ -154,6 +178,9 @@ func (self Source) Get3f(param int32) (v1, v2, v3 float32) { // Renamed, was GetSourcefv. func (self Source) Getfv(param int32, values []float32) { + if len(values) == 0 { + return + } C.walGetSourcefv(C.ALuint(self), C.ALenum(param), unsafe.Pointer(&values[0])) } @@ -171,6 +198,9 @@ func (self Source) Get3i(param int32) (v1, v2, v3 int32) { // Renamed, was GetSourceiv. func (self Source) Getiv(param int32, values []int32) { + if len(values) == 0 { + return + } C.walGetSourceiv(C.ALuint(self), C.ALenum(param), unsafe.Pointer(&values[0])) } @@ -202,11 +232,17 @@ func (self Source) Pause() { // Renamed, was SourceQueueBuffers. func (self Source) QueueBuffers(buffers Buffers) { + if len(buffers) == 0 { + return + } C.walSourceQueueBuffers(C.ALuint(self), C.ALsizei(len(buffers)), unsafe.Pointer(&buffers[0])) } // Renamed, was SourceUnqueueBuffers. func (self Source) UnqueueBuffers(buffers Buffers) { + if len(buffers) == 0 { + return + } C.walSourceUnqueueBuffers(C.ALuint(self), C.ALsizei(len(buffers)), unsafe.Pointer(&buffers[0])) } diff --git a/gumble/go-openal/openal/util.go b/gumble/go-openal/openal/util.go index f2ef457..01a2b5b 100644 --- a/gumble/go-openal/openal/util.go +++ b/gumble/go-openal/openal/util.go @@ -17,8 +17,6 @@ import "strings" // Convenience Interface. type Vector [3]float32 -var tempSlice = make([]float32, 6) - const ( x = iota y