Re: [PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()

[Takashi Iwai <tiwai@suse.de>]

On Tue, 28 Nov 2017 07:33:24 +0100,
Kuninori Morimoto wrote:
> 
> 
> Hi Mark, Takashi-san
> 
> I wonder are these patches acceptable ? or not ?
> If acceptable, should I resend ?

Since the use-case to pass NULL is limited, I'd rather avoid the
change.  If there were more than handful users, maybe I would rethink,
but currently not convincing enough.

And, the NULL check is needed only when the caller is outside the
stream lock.  Such a situation already smells like a buggy code.
We don't need to make it easier :)


thanks,

Takashi

> > 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
>