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

[Kuninori Morimoto]

Hi Mark, Takashi-san

I wonder are these patches acceptable ? or not ?
If acceptable, should I resend ?

> 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


Best regards
---
Kuninori Morimoto