Skip to content

Audio: Fix Memory Leaks in Buffer Component Release #10052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tmleman wants to merge 5 commits into from
Closed

Audio: Fix Memory Leaks in Buffer Component Release #10052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c1c890f
audio: copier: fix memory leak in copier_dai_free
tmleman Jun 10, 2025
7e70394
ipc3: fix memory leak in ipc_buffer_new
tmleman Jun 10, 2025
98a1151
audio: fix memory leak in chain_dma buffer handling
tmleman Jun 10, 2025
d6d8e98
audio: resolve buffer memory leaks in legacy reset functions
tmleman Jun 10, 2025
bfcaba5
audio: complete dma_buffer deallocation in Zephyr components
tmleman Jun 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/audio/chain_dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,7 @@ __cold static void chain_release(struct comp_dev *dev)

if (cd->dma_buffer) {
buffer_free(cd->dma_buffer);
rfree(cd->dma_buffer);
Copy link
Collaborator

@lyakh lyakh Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@marcinszkudlinski marcinszkudlinski Jun 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

cd->dma_buffer = NULL;
}
}
Expand Down Expand Up @@ -621,6 +622,7 @@ __cold static int chain_task_init(struct comp_dev *dev, uint8_t host_dma_id, uin
ret = chain_init(dev, buff_addr, buff_size);
if (ret < 0) {
buffer_free(cd->dma_buffer);
rfree(cd->dma_buffer);
cd->dma_buffer = NULL;
goto error;
}
Expand Down
5 changes: 4 additions & 1 deletion src/audio/copier/copier_dai.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,11 @@ __cold void copier_dai_free(struct copier_data *cd)
rfree(cd->dd[i]);
}
/* only dai have multi endpoint case */
if (cd->multi_endpoint_buffer)
if (cd->multi_endpoint_buffer) {
buffer_free(cd->multi_endpoint_buffer);
rfree(cd->multi_endpoint_buffer);
cd->multi_endpoint_buffer = NULL;
}
}

int copier_dai_prepare(struct comp_dev *dev, struct copier_data *cd)
Expand Down
1 change: 1 addition & 0 deletions src/audio/dai-legacy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,7 @@ void dai_common_reset(struct dai_data *dd, struct comp_dev *dev)

if (dd->dma_buffer) {
buffer_free(dd->dma_buffer);
rfree(dd->dma_buffer);
dd->dma_buffer = NULL;
}

Expand Down
2 changes: 2 additions & 0 deletions src/audio/dai-zephyr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1105,6 +1105,7 @@ int dai_common_params(struct dai_data *dd, struct comp_dev *dev,
*/
if (err < 0) {
buffer_free(dd->dma_buffer);
rfree(dd->dma_buffer);
dd->dma_buffer = NULL;
dma_sg_free(&config->elem_array);
rfree(dd->z_config);
Expand Down Expand Up @@ -1246,6 +1247,7 @@ void dai_common_reset(struct dai_data *dd, struct comp_dev *dev)

if (dd->dma_buffer) {
buffer_free(dd->dma_buffer);
rfree(dd->dma_buffer);
dd->dma_buffer = NULL;
}

Expand Down
1 change: 1 addition & 0 deletions src/audio/host-legacy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,7 @@ void host_common_reset(struct host_data *hd, uint16_t state)
/* free DMA buffer */
if (hd->dma_buffer) {
buffer_free(hd->dma_buffer);
rfree(hd->dma_buffer);
hd->dma_buffer = NULL;
}

Expand Down
1 change: 1 addition & 0 deletions src/audio/host-zephyr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,6 +1131,7 @@ void host_common_reset(struct host_data *hd, uint16_t state)
/* free DMA buffer */
if (hd->dma_buffer) {
buffer_free(hd->dma_buffer);
rfree(hd->dma_buffer);
hd->dma_buffer = NULL;
}

Expand Down
1 change: 1 addition & 0 deletions src/ipc/ipc3/helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@ int ipc_buffer_new(struct ipc *ipc, const struct sof_ipc_buffer *desc)
sizeof(struct ipc_comp_dev));
if (!ibd) {
buffer_free(buffer);
rfree(buffer);
return -ENOMEM;
}
ibd->cb = buffer;
Expand Down
Loading