[alsa-devel] [PATCH v2 alsa-lib 1/6] pcm: hw: add helper functions to map/unmap status/control data for runtime of PCM substream

[Takashi Sakamoto]

Hi,

On Jun 27 2017 17:15, Takashi Iwai wrote:
> On Tue, 27 Jun 2017 10:09:37 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On Jun 27 2017 00:09, Takashi Iwai wrote:
>>> On Sun, 25 Jun 2017 06:41:19 +0200,
>>> Takashi Sakamoto wrote:
>>>>
>>>> Handling mapping operation for status/control data includes some
>>>> supplemental operations for fallback mode. It's better to have helper
>>>> functions for this purpose.
>>>>
>>>> This commit adds the helper functions.
>>>> ---
>>>>    src/pcm/pcm_hw.c | 48 ++++++++++++++++++++++++++++++++++--------------
>>>>    1 file changed, 34 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
>>>> index a648d12c..abf4afe0 100644
>>>> --- a/src/pcm/pcm_hw.c
>>>> +++ b/src/pcm/pcm_hw.c
>>>> @@ -31,6 +31,7 @@
>>>>    #include <stdlib.h>
>>>>    #include <stddef.h>
>>>>    #include <unistd.h>
>>>> +#include <stdbool.h>
>>>>    #include <signal.h>
>>>>    #include <string.h>
>>>>    #include <fcntl.h>
>>>> @@ -866,7 +867,7 @@ static snd_pcm_sframes_t snd_pcm_hw_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_u
>>>>    	return xfern.result;
>>>>    }
>>>>    -static int snd_pcm_hw_mmap_status(snd_pcm_t *pcm)
>>>> +static int map_status_data(snd_pcm_t *pcm)
>>>>    {
>>>>    	snd_pcm_hw_t *hw = pcm->private_data;
>>>>    	struct snd_pcm_sync_ptr sync_ptr;
>>>> @@ -900,7 +901,7 @@ static int snd_pcm_hw_mmap_status(snd_pcm_t *pcm)
>>>>    	return 0;
>>>>    }
>>>>    -static int snd_pcm_hw_mmap_control(snd_pcm_t *pcm)
>>>> +static int map_control_data(snd_pcm_t *pcm)
>>>>    {
>>>>    	snd_pcm_hw_t *hw = pcm->private_data;
>>>>    	void *ptr;
>>>> @@ -922,10 +923,28 @@ static int snd_pcm_hw_mmap_control(snd_pcm_t *pcm)
>>>>    	return 0;
>>>>    }
>>>>    -static int snd_pcm_hw_munmap_status(snd_pcm_t *pcm)
>>>> +static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
>>>>    {
>>>>    	snd_pcm_hw_t *hw = pcm->private_data;
>>>>    	int err;
>>>> +
>>>> +	hw->sync_ptr_ioctl = (int)force_fallback;
>>>> +
>>>> +	err = map_status_data(pcm);
>>>> +	if (err < 0)
>>>> +		return err;
>>>> +
>>>> +	err = map_control_data(pcm);
>>>> +	if (err < 0)
>>>> +		return err;
>>>
>>> This would leave the status mmap in the error path?
>>
>> In the error path, snd_pcm_hw_open_fd() calls snd_pcm_close(), then
>> The status data is going to be unmapped.
>>
>> snd_pcm_hw_open_fd()
>> ->snd_pcm_close()
>>    ->snd_pcm_hw_close()
>>      ->unmap_status_and_control_data()
>>        ->unmap_status_data()
>>          ->munmap(2) or free(3)
> 
> Ah then there was already a bug there.  It free/munmap unconditionally
> for both status and control even for an error path of the mmap of
> control (while status was mmapped).

Yep. These codes were originally written with a loose manner. But it 
would be allowed because both 'mmap(NULL, non-zero)' and 'free(NULL)' 
generates no error and 'sync_ptr_ioctl' works fine, and 'sync_ptr' is 
assigned to NULL after deallocated.

> But OK, it's no big issue and doesn't need to block the whole patchset
> at this point.

For your information.

======== 8< --------
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

#include <string.h>
#include <limits.h>
#include <errno.h>

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#include <sys/ioctl.h>
#include <sys/mman.h>

#include <sound/asound.h>

int main(void)
{
     int fd;
     size_t page_size = sysconf(_SC_PAGESIZE);
     int pages;
     void *status;
     int status_length;
     void *control;
     int control_length;

     fd = open("/dev/snd/pcmC0D0p", O_RDWR);
     if (fd < 0) {
         printf("open(2): %s\n", strerror(errno));
         return EXIT_FAILURE;
     }

     /* Map status data. */
     pages = (sizeof(struct snd_pcm_mmap_status) + page_size - 1) / 
page_size;
     status_length = pages * page_size;
     status = MAP_FAILED;
     status = mmap(NULL, status_length, PROT_READ, MAP_FILE | MAP_SHARED,
                   fd, SNDRV_PCM_MMAP_OFFSET_STATUS);
     if (status == MAP_FAILED || status == NULL) {
         close(fd);
         printf("mmap(2): status: %s\n", strerror(errno));
         return EXIT_FAILURE;
     }

     /* Map control data. */
     pages = (sizeof(struct snd_pcm_mmap_control) + page_size - 1) / 
page_size;
     control_length = pages * page_size;
     control = MAP_FAILED;
     control = mmap(NULL, control_length, PROT_READ | PROT_WRITE,
                    MAP_FILE | MAP_SHARED, fd, 
SNDRV_PCM_MMAP_OFFSET_CONTROL);
     if (status == MAP_FAILED || status == NULL) {
         munmap(status, status_length);
         close(fd);
         printf("mmap(2): control: %s\n", strerror(errno));
         return EXIT_FAILURE;
     }

     /* Not error. */
     if (munmap(NULL, 10000) < 0)
         printf("unexpected: munmap(2): NULL: %s\n", strerror(errno));

     /* Generate error. */
     if (munmap(NULL, 0) < 0)
         printf("expected: munmap(2): NULL: %s\n", strerror(errno));
     /* Ditto. */
     if (munmap(control, 0) < 0)
         printf("expected: munmap(2): control: 0: %s\n", strerror(errno));
     /* Ditto. */
     if (munmap(status, 0) < 0)
         printf("expected: munmap(2): status: 0: %s\n", strerror(errno));

     if (munmap(control, control_length) < 0)
         printf("unexpected: munmap(2): control: 0: %s\n", strerror(errno));
     if (munmap(status, status_length) < 0)
         printf("unexpected: munmap(2): status: 0: %s\n", strerror(errno));
     close(fd);

     /* Not error. */
     free(NULL);

     return EXIT_SUCCESS;
}
======== 8< --------


Regards

Takashi Sakamoto