pcm: ioplug: Limit transfer size to buffer boundary #103
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aditpape
force-pushed
the
ioplug_2nd_transfer_boundary_check
branch
from
1d7a371
to
fb911b4
Compare
|
updated patch to cover cases with avail being exact multiple of buffer_size. |
aditpape
force-pushed
the
ioplug_2nd_transfer_boundary_check
branch
from
fb911b4
to
2036b86
Compare
Commit 1714332 introduced 2nd transfer() call to transfer all remaining available frames. Unfortunately this valuable fix introduced two regressions: a) If the prior calculated avail value exceeds the buffer size a too large size value is passed to the underlaying plugin and results in memory corruption if not blocked by attached plugin internally. Avail values > buffer size can happen if e.g. xrun detection is disabled, as avail is calculated by pure difference between hw and app position. This patch limits 2nd transfer call to fit into area. b) the 2nd transfer callback is executed with adapted offset+size but without updating pcm->appl_ptr. An underlaying plugin may rely on consistent pointer positions. To maintain consistency the appl_ptr is temporarily forwarded during the 2nd transfer call and rewinded afterwards. Signed-off-by: Andreas Pape <apape@de.adit-jv.com>
aditpape
force-pushed
the
ioplug_2nd_transfer_boundary_check
branch
from
2036b86
to
8cf12b4
Compare
|
Dear @perexg, can you give indication on PR acceptance? |
perexg
added a commit
to perexg/alsa-lib
that referenced
this pull request
Jan 21, 2021
It seems that the commit "pcm: ioplug: Transfer all available data" introduced new regressions (wrong memory access). The avail_update in ioplug does not move appl_ptr nor hw_ptr, so it's possible that the transfers may be repetitive. This patch moves the transfer calls to mmap_begin callback where it should be. Fixes: 1714332 ("pcm: ioplug: Transfer all available data") BugLink: alsa-project#103 Signed-off-by: Jaroslav Kysela <perex@perex.cz>
perexg
added a commit
to perexg/alsa-lib
that referenced
this pull request
Jan 21, 2021
It seems that the commit "pcm: ioplug: Transfer all available data" introduced new regressions (wrong memory access). The second issue is that the avail_update in ioplug does not move appl_ptr nor hw_ptr, so it's possible that the transfers may be repetitive. This patch moves the transfer calls to mmap_begin callback where it should be. The pointer wraps are handled by design now. Fixes: 1714332 ("pcm: ioplug: Transfer all available data") BugLink: alsa-project#103 Signed-off-by: Jaroslav Kysela <perex@perex.cz>
|
Looking to the code, it does not make sense to copy the capture data in snd_pcm_ioplug_avail_update(). It should be copied in mmap_begin callback. Or we should do the dual-buffering like in pcm_plugin.c (it means separate the external plugin hw_ptr and appl_ptr). I created #115 with my version. Could you test? @tiwai - FYI ! |
|
Promising approach... |
|
Tested your #115 -proposal: |
|
Thank you for the feedback. Closing and merging #115 . |
perexg
added a commit
that referenced
this pull request
Jan 27, 2021
It seems that the commit "pcm: ioplug: Transfer all available data" introduced new regressions (wrong memory access). The second issue is that the avail_update in ioplug does not move appl_ptr nor hw_ptr, so it's possible that the transfers may be repetitive. This patch moves the transfer calls to mmap_begin callback where it should be. The pointer wraps are handled by design now. Fixes: 1714332 ("pcm: ioplug: Transfer all available data") BugLink: #103 Tested-by: Andreas Pape <apape@de.adit-jv.com> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
lgirdwood
pushed a commit
to thesofproject/alsa-lib
that referenced
this pull request
May 31, 2021
It seems that the commit "pcm: ioplug: Transfer all available data" introduced new regressions (wrong memory access). The second issue is that the avail_update in ioplug does not move appl_ptr nor hw_ptr, so it's possible that the transfers may be repetitive. This patch moves the transfer calls to mmap_begin callback where it should be. The pointer wraps are handled by design now. Fixes: 1714332 ("pcm: ioplug: Transfer all available data") BugLink: alsa-project#103 Tested-by: Andreas Pape <apape@de.adit-jv.com> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit 1714332 introduced 2nd transfer()
call to transfer all remaining available frames.
If the prior calculated avail value exceeds the buffer size a too large size value
is passed to the underlaying plugin and results in memory corruption if not blocked by plugin internally.
Avail values > buffer size can happen if e.g. xrun detection is disabled,
as avail is calculated by pure difference between hw and app position.
This patch limits 2nd transfer call to remaining rest of a buffer size.
Signed-off-by: Andreas Pape apape@de.adit-jv.com