Hi,
On Wed, Jun 19, 2019 at 01:16:39PM +0300, Dan Carpenter wrote:
> Hello Takashi Sakamoto,
>
> The patch 4f380d007052: "ALSA: oxfw: configure packet format in
> pcm.hw_params callback" from Jun 12, 2019, leads to the following
> static checker warning:
>
> sound/firewire/oxfw/oxfw-stream.c:357 snd_oxfw_stream_start_duplex()
> warn: 'oxfw->rx_stream.buffer.packets' double freed
>
> sound/firewire/oxfw/oxfw-stream.c
> 317 int snd_oxfw_stream_start_duplex(struct snd_oxfw *oxfw)
> 318 {
> 319 int err;
> 320
> 321 if (oxfw->substreams_count == 0)
> 322 return -EIO;
> 323
> 324 if (amdtp_streaming_error(&oxfw->rx_stream) ||
> 325 amdtp_streaming_error(&oxfw->tx_stream)) {
> 326 amdtp_stream_stop(&oxfw->rx_stream);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> 327 cmp_connection_break(&oxfw->in_conn);
> 328
> 329 if (oxfw->has_output) {
> 330 amdtp_stream_stop(&oxfw->tx_stream);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> 331 cmp_connection_break(&oxfw->out_conn);
> 332 }
> 333 }
> 334
> 335 if (!amdtp_stream_running(&oxfw->rx_stream)) {
> 336 err = start_stream(oxfw, &oxfw->rx_stream);
> 337 if (err < 0) {
> 338 dev_err(&oxfw->unit->device,
> 339 "fail to start rx stream: %d\n", err);
> 340 goto error;
> 341 }
> 342 }
> 343
> 344 if (oxfw->has_output) {
> 345 if (!amdtp_stream_running(&oxfw->tx_stream)) {
> 346 err = start_stream(oxfw, &oxfw->tx_stream);
> 347 if (err < 0) {
> 348 dev_err(&oxfw->unit->device,
> 349 "fail to start tx stream: %d\n", err);
> 350 goto error;
> 351 }
> 352 }
> 353 }
> 354
> 355 return 0;
> 356 error:
> 357 amdtp_stream_stop(&oxfw->rx_stream);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Smatch is basically complaining that we call amdtp_stream_stop() and
> it's not clear that we necessarily reset things. I don't know this code
> very well so I have maybe missed something.
>
> 358 cmp_connection_break(&oxfw->in_conn);
> 359 if (oxfw->has_output) {
> 360 amdtp_stream_stop(&oxfw->tx_stream);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> 361 cmp_connection_break(&oxfw->out_conn);
> 362 }
> 363 return err;
> 364 }
>
> regards,
> dan carpenter
Thanks for you report, however the double-free doesn't occur because the
data of 'struct amdtp_stream' is protected by mutex and 'context' member
is a flag to call kernel APIs for 'struct iso_packets_buffer'. The kernel
API, amdtp_stream_stop()' can be called several times with no corruption.
```
(sound/firewire/amdtp-stream.c)
void amdtp_stream_stop(struct amdtp_stream *s)
{
mutex_lock(&s->mutex);
if (!amdtp_stream_running(s)) {
mutex_unlock(&s->mutex);
return;
}
...
fw_iso_context_destroy(s->context);
s->context = ERR_PTR(-1);
iso_packets_buffer_destroy(&s->buffer, s->unit);
...
mutex_unlock(&s->mutex);
}
EXPORT_SYMBOL(amdtp_stream_stop);
```
``
(sound/firewire/amdtp-stream.h)
static inline bool amdtp_stream_running(struct amdtp_stream *s)
{
return !IS_ERR(s->context);
}
``
Regards
Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
