Mailing List Archive

[bug report] ALSA: xen-front: Use Xen common shared buffer implementation
Hello Oleksandr Andrushchenko,

The patch 58f9d806d16a: "ALSA: xen-front: Use Xen common shared
buffer implementation" from Nov 30, 2018, leads to the following
static checker warning:

sound/xen/xen_snd_front_alsa.c:495 alsa_hw_params()
warn: 'stream->shbuf.directory' double freed
sound/xen/xen_snd_front_alsa.c:495 alsa_hw_params()
warn: 'stream->shbuf.grefs' double freed

sound/xen/xen_snd_front_alsa.c
461 static int alsa_hw_params(struct snd_pcm_substream *substream,
462 struct snd_pcm_hw_params *params)
463 {
464 struct xen_snd_front_pcm_stream_info *stream = stream_get(substream);
465 struct xen_snd_front_info *front_info = stream->front_info;
466 struct xen_front_pgdir_shbuf_cfg buf_cfg;
467 int ret;
468
469 /*
470 * This callback may be called multiple times,
471 * so free the previously allocated shared buffer if any.
472 */
473 stream_free(stream);
^^^^^^^^^^^^^^^^^^^
This is freed here.

474 ret = shbuf_setup_backstore(stream, params_buffer_bytes(params));
475 if (ret < 0)
476 goto fail;
^^^^^^^^^^
This leads to some double frees. Probably more double frees than Smatch
is detecting.

477
478 memset(&buf_cfg, 0, sizeof(buf_cfg));
479 buf_cfg.xb_dev = front_info->xb_dev;
480 buf_cfg.pgdir = &stream->shbuf;
481 buf_cfg.num_pages = stream->num_pages;
482 buf_cfg.pages = stream->pages;
483
484 ret = xen_front_pgdir_shbuf_alloc(&buf_cfg);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is where "stream->shbuf.directory" is re-allocated on the success
path.

485 if (ret < 0)
486 goto fail;
487
488 ret = xen_front_pgdir_shbuf_map(&stream->shbuf);
489 if (ret < 0)
490 goto fail;
491
492 return 0;
493
494 fail:
495 stream_free(stream);
^^^^^^^^^^^^^^^^^^^^
Double free.

496 dev_err(&front_info->xb_dev->dev,
497 "Failed to allocate buffers for stream with index %d\n",
498 stream->index);
499 return ret;
500 }

regards,
dan carpenter
Re: [bug report] ALSA: xen-front: Use Xen common shared buffer implementation [ In reply to ]
Hello, Dan!

On 10/21/20 1:50 PM, Dan Carpenter wrote:
> Hello Oleksandr Andrushchenko,
>
> The patch 58f9d806d16a: "ALSA: xen-front: Use Xen common shared
> buffer implementation" from Nov 30, 2018, leads to the following
> static checker warning:
>
> sound/xen/xen_snd_front_alsa.c:495 alsa_hw_params()
> warn: 'stream->shbuf.directory' double freed
> sound/xen/xen_snd_front_alsa.c:495 alsa_hw_params()
> warn: 'stream->shbuf.grefs' double freed
>
> sound/xen/xen_snd_front_alsa.c
> 461 static int alsa_hw_params(struct snd_pcm_substream *substream,
> 462 struct snd_pcm_hw_params *params)
> 463 {
> 464 struct xen_snd_front_pcm_stream_info *stream = stream_get(substream);
> 465 struct xen_snd_front_info *front_info = stream->front_info;
> 466 struct xen_front_pgdir_shbuf_cfg buf_cfg;
> 467 int ret;
> 468
> 469 /*
> 470 * This callback may be called multiple times,
> 471 * so free the previously allocated shared buffer if any.
> 472 */
> 473 stream_free(stream);
> ^^^^^^^^^^^^^^^^^^^
> This is freed here.
>
> 474 ret = shbuf_setup_backstore(stream, params_buffer_bytes(params));
> 475 if (ret < 0)
> 476 goto fail;
> ^^^^^^^^^^
> This leads to some double frees. Probably more double frees than Smatch
> is detecting.
>
> 477
> 478 memset(&buf_cfg, 0, sizeof(buf_cfg));
> 479 buf_cfg.xb_dev = front_info->xb_dev;
> 480 buf_cfg.pgdir = &stream->shbuf;
> 481 buf_cfg.num_pages = stream->num_pages;
> 482 buf_cfg.pages = stream->pages;
> 483
> 484 ret = xen_front_pgdir_shbuf_alloc(&buf_cfg);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is where "stream->shbuf.directory" is re-allocated on the success
> path.
>
> 485 if (ret < 0)
> 486 goto fail;
> 487
> 488 ret = xen_front_pgdir_shbuf_map(&stream->shbuf);
> 489 if (ret < 0)
> 490 goto fail;
> 491
> 492 return 0;
> 493
> 494 fail:
> 495 stream_free(stream);
> ^^^^^^^^^^^^^^^^^^^^
> Double free.
>
> 496 dev_err(&front_info->xb_dev->dev,
> 497 "Failed to allocate buffers for stream with index %d\n",
> 498 stream->index);
> 499 return ret;
> 500 }
>
> regards,
> dan carpenter

Thank you for reporting this,

I'll try to look at it closely and prepare a patch.

Thank you,

Oleksandr