[alsa-devel] [PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()

[Takashi Iwai]

On Thu, 09 Nov 2017 03:40:22 +0100,
Takashi Sakamoto wrote:
> 
> On Nov 9 2017 11:11, Kuninori Morimoto wrote:
> >
> > Hi Takashi-san, Mark
> >
> > snd_pcm_running() is using "substream" and "substream->runtime"
> > pointer, no check.
> > These patches adds its check in function,
> > and removes duplicate checks from each drivers.
> >
> > Not super important, but can be cleanup
> >
> > Kuninori Morimoto (6):
> >    ALSA: pcm: check parameter on snd_pcm_running()
> >    ALSA: pdaudiocf: remove unneeded check for snd_pcm_running()
> >    ASoC: dwc: remove unneeded check for snd_pcm_running()
> >    ASoC: omap-hdmi-audio: remove unneeded check for snd_pcm_running()
> >    ASoC: xtfpga-i2s: remove unneeded check for snd_pcm_running()
> >    ASoC: rsnd: remove unneeded check for snd_pcm_running()
> >
> >   include/sound/pcm.h                    | 3 +++
> >   sound/pcmcia/pdaudiocf/pdaudiocf_irq.c | 2 +-
> >   sound/soc/dwc/dwc-pcm.c                | 2 +-
> >   sound/soc/omap/omap-hdmi-audio.c       | 3 +--
> >   sound/soc/sh/rcar/core.c               | 5 +----
> >   sound/soc/xtensa/xtfpga-i2s.c          | 4 ++--
> >   6 files changed, 9 insertions(+), 10 deletions(-)
> 
> This is a bad direction. I exactly oppose to your idea.
> 
> >  include/sound/pcm.h | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > index 24febf9..a8e49f5 100644
> > --- a/include/sound/pcm.h
> > +++ b/include/sound/pcm.h
> > @@ -664,6 +664,9 @@ void snd_pcm_stream_unlock_irqrestore(struct 
> snd_pcm_substream *substream,
> >   */
> >  static inline int snd_pcm_running(struct snd_pcm_substream *substream)
> >  {
> > +     if (!substream || !substream->runtime)
> > +             return 0;
> > +
> >       return (substream->runtime->status->state == 
> SNDRV_PCM_STATE_RUNNING ||
> >               (substream->runtime->status->state == 
> SNDRV_PCM_STATE_DRAINING &&
> >                substream->stream == SNDRV_PCM_STREAM_PLAYBACK));
> 
> In a view of 'design by contract', this function has a pre-condition
> that a given argument should not be NULL. Callers _should_ guarantee
> it to keep semantics of this function.

Generally I agree, but note that it depends on the exposure of the API
function itself.  If the API function is supposed to be an interface
directly communicated with the outside, it's not seldom to allow NULL
there.  An implicit NULL-check would be handy and often makes coding
easier (see the case of free()).

> Your idea appends the duty of callers to this function. This causes a
> semantical contradiction. If it were something to bring kernel
> corruption such as BUG_ON(), the original design would be kept. When
> substream is NULL, it's a bug of drivers in adding PCM
> components. When runtime is NULL, it's a bug of ALSA PCM core in
> handling open system call.

When you call snd_pcm_running(), basically you're evaluating the PCM
stream status, and likely a state machine.  It often assumes that PCM
state is consistent during the following action, and it implies the
PCM stream lock was acquired.  And, of course, PCM stream lock
requires the non-NULL substream.

That said, if the code has a proper protection for the PCM stream
consistency, the substream NULL check had to be done far before that
point due to a stream lock.

Though, most codes aren't super-critical about the state change and
the direct snd_pcm_running() works in most cases.  But in the perfect
world, stream locking is preferred around the state evaluation and the
action according to it.


thanks,

Takashi