Audio: Fix Memory Leaks in Buffer Component Release

[tmleman]

This pull request resolves memory leak issues in the buffer component release processes across various audio and IPC components. Each commit ensures the complete deallocation of buffer structures by adding necessary rfree calls following buffer_free. This prevents potential memory leaks and enhances the stability and reliability of the system.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[lyakh]

looks wrong to me, instead of fixing this seems to add a double-free bug

[lyakh]

this looks wrong to me. Looking at buffer_free()

sof/src/include/sof/audio/buffer.h

Lines 236 to 239 in 97f869c

	static inline void buffer_free(struct comp_buffer *buffer)
	{
	audio_buffer_free(&buffer->audio_buffer);
	}

and then in audio_buffer_free()

sof/src/audio/buffers/audio_buffer.c

Line 97 in 97f869c

rfree(buffer);

and taking into account the definition of struct comp_buffer

sof/src/include/sof/audio/buffer.h

Lines 126 to 128 in 97f869c

	struct comp_buffer {
	/* data buffer */
	struct sof_audio_buffer audio_buffer;

we come to several conclusions: (1) calling rfree(buffer) in audio_buffer_free() isn't a good idea, because audio_buffer_free() isn't called on a whole object, that can be freed, but on a member of an object. (2) that does however happen to work because that member is currently the first in the object, so its address is equal to the address of the object, so it ends up freeing the object, (3) these fixes aren't correct because the buffer is indeed freed by buffer_free() because of the above. So, this PR isn't fixing a problem but in fact adding a double free bug, instead audio_buffer_free() and / or buffer_free() should be fixed to clarify which one frees the underlying buffer memory.

[marcinszkudlinski]

isn't called on a whole object, that can be freed, but on a member of an object.

not really. The main object is struct sof_audio_buffer, a base class for all buffers (currently comp_buffer and ring buffer)
in c++ it would be simple and clear

class comp_buffer : public sof_audio_buffer

class ring_buffer : public sof_audio_buffer

also freeing would be obvious by using virtual destructors

but we're using an obsolete C, so we need to put sof_audio_buffer as a member :(

this part:
static inline void buffer_free(struct comp_buffer *buffer) { audio_buffer_free(&buffer->audio_buffer); }

is just a wrapper. Although sof_audio_buffer should be used (what would make possible using comp_buffer and ring_buffer - and other buffers - directly in a pipeline), yet some code is still using comp_buffer as a main structure (with unfortunate double buffering when ring_buffer is needed)

[marcinszkudlinski]

@tmleman
the change is not needed at all

see:

static inline void buffer_free(struct comp_buffer *buffer)  
{  
	audio_buffer_free(&buffer->audio_buffer);  
} 


void audio_buffer_free(struct sof_audio_buffer *buffer)
{
	if (!buffer)
		return;

	CORE_CHECK_STRUCT(buffer);
#if CONFIG_PIPELINE_2_0
	audio_buffer_free(buffer->secondary_buffer_sink);
	audio_buffer_free(buffer->secondary_buffer_source);
#endif /* CONFIG_PIPELINE_2_0 */
	if (buffer->ops->free)
		buffer->ops->free(buffer);
	rfree(buffer);
}

as you see, the buffer itself is freed

BUT
this requires that struct sof_audio_buffer is the very first member of all structures that "inherite" from it.
This is enforced by exception - rfree will fail if freed pointer is not the same that was allocated before.

HOWEVER - it may be changed to more secure code:

switch(buffer->buffer_type) {
case BUFFER_TYPE_LEGACY_BUFFER: rfree(container_of(audio_buffer, struct comp_buffer, audio_buffer);
case BUFFER_TYPE_RING_BUFFER: rfree(container_of(ring_buffer, struct ring_buffer, audio_buffer);
}

(above may have incorrect syntax, just a quick demo code)

I'm not convinced it is worth, will cause more code doing the very very same operation

Maybe better a compilation time assert, enforcing sof_audio_buffer to be a first member?

@lyakh ?

[marcinszkudlinski]

OR we can make ops->free obligatory and put the actual call to rfree there

[tmleman]

the change is not needed at all

Yes, that's correct. The tests also clearly show this. I reached the wrong conclusions while analyzing the code.