Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

On 10/12/21 10:27 AM, Takashi Iwai wrote:
> On Tue, 12 Oct 2021 17:15:56 +0200,
> Pierre-Louis Bossart wrote:
>>
>>> In either way, the new_ptr has to be validated beforehand that it's
>>> within 0..boundary-1.  (old_ptr is assumed to be valid.)
>>
>> In the 3 of the calls to pcm_lib_apply_appl_ptr(), the check is done
>> already prior to calling that function
>> 	if (appl_ptr >= runtime->boundary)
>> 		appl_ptr -= runtime->boundary;
>> 	err = pcm_lib_apply_appl_ptr(substream, appl_ptr);
>>
>>
>> it's rather unclear to me why the same check is not done for sync_ptr, e.g.
>>
>> if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
>> 	err = pcm_lib_apply_appl_ptr(substream,	
>> 			     sync_ptr.c.control.appl_ptr);
>>
>> if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL)) {
>> 	err = pcm_lib_apply_appl_ptr(substream,
>> 				scontrol.appl_ptr);
>>
>> Should I add a check there, or add a check inside of
>> pcm_lib_apply_appl_ptr() which would be a duplicate in the majority of
>> cases?
> 
> I guess adding in pcm_lib_appl_appl_ptr() would be easier and safer.
> There is even one more place that is calling pcm_lib_apply_appl_ptr()
> in the very latest commit (a fix for a buggy 32bit compat ioctl).

ok, here's the code I'll start testing. Thanks a lot Takashi for your help.

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index a144a3f68e9e..e839459916ca 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2127,11 +2127,30 @@ int pcm_lib_apply_appl_ptr(struct
snd_pcm_substream *substream,
 {
        struct snd_pcm_runtime *runtime = substream->runtime;
        snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
+       snd_pcm_sframes_t diff;
        int ret;

        if (old_appl_ptr == appl_ptr)
                return 0;

+       /*
+        * check if a rewind is requested by the application, after
+        * verifying the new appl_ptr is in the 0..boundary range
+        */
+       if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
+               if (appl_ptr >= runtime->boundary)
+                       appl_ptr -= runtime->boundary;
+
+               diff = appl_ptr - old_appl_ptr;
+               if (diff >= 0) {
+                       if (diff > runtime->buffer_size)
+                               return 0;
+               } else {
+                       if (runtime->boundary + diff > runtime->buffer_size)
+                               return 0;
+               }
+       }
+
        runtime->control->appl_ptr = appl_ptr;
        if (substream->ops->ack) {
                ret = substream->ops->ack(substream);