| Message ID | 1400191076-28279-1-git-send-email-rickard_strandqvist@spectrumdigital.se |
|---|---|
| State | New |
| Headers | show |
Hi, Rickard Strandqvist wrote: > There is otherwise a risk of a possible null pointer dereference. > > Was largely found by using a static code analysis program called cppcheck. > > Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> > --- > sound/soc/samsung/dma.c | 10 ++++++---- > 1 fil ändrad, 6 tillägg(+), 4 borttagningar(-) > > diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c > index dc09b71..b1f6757 100644 > --- a/sound/soc/samsung/dma.c > +++ b/sound/soc/samsung/dma.c > @@ -115,17 +115,19 @@ static void dma_enqueue(struct snd_pcm_substream *substream) > static void audio_buffdone(void *data) > { > struct snd_pcm_substream *substream = data; > - struct runtime_data *prtd = substream->runtime->private_data; > + struct runtime_data *prtd = NULL; > > pr_debug("Entered %s\n", __func__); > > - if (prtd->state & ST_RUNNING) { > + if(substream) > Missing space. Lothar Waßmann
Hi Rickard, On 15.05.2014 23:57, Rickard Strandqvist wrote: > There is otherwise a risk of a possible null pointer dereference. > > Was largely found by using a static code analysis program called cppcheck. > > Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> > --- > sound/soc/samsung/dma.c | 10 ++++++---- > 1 fil ändrad, 6 tillägg(+), 4 borttagningar(-) > > diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c > index dc09b71..b1f6757 100644 > --- a/sound/soc/samsung/dma.c > +++ b/sound/soc/samsung/dma.c > @@ -115,17 +115,19 @@ static void dma_enqueue(struct snd_pcm_substream *substream) > static void audio_buffdone(void *data) > { > struct snd_pcm_substream *substream = data; > - struct runtime_data *prtd = substream->runtime->private_data; > + struct runtime_data *prtd = NULL; > > pr_debug("Entered %s\n", __func__); > > - if (prtd->state & ST_RUNNING) { > + if(substream) > + prtd = substream->runtime->private_data; > + > + if (prtd && prtd->state & ST_RUNNING) { > prtd->dma_pos += prtd->dma_period; > if (prtd->dma_pos >= prtd->dma_end) > prtd->dma_pos = prtd->dma_start; > > - if (substream) > - snd_pcm_period_elapsed(substream); > + snd_pcm_period_elapsed(substream); > > spin_lock(&prtd->lock); > if (!samsung_dma_has_circular()) { > Can you find a path (or use case, if possible) on which any of affected pointers will be NULL? Best regards, Tomasz
Hi Tomasz
What I based my patch on is really because of this line:
if (substream)
snd_pcm_period_elapsed(substream);
Boojin Kim thought that this was needed, if this is true anymore..? I
have not been able to immerse myself so much in all patches.
I'm working on about 100 similar patches.
344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02
09:44:43 +0900 123) prtd->dma_pos += prtd->dma_period;
344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02
09:44:43 +0900 124) if (prtd->dma_pos >= prtd->dma_end)
344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02
09:44:43 +0900 125) prtd->dma_pos =
prtd->dma_start;
5111c075 sound/soc/s3c24xx/s3c24xx-pcm.c (Mark Brown 2008-04-30
17:19:32 +0200 126)
344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02
09:44:43 +0900 127) if (substream)
344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02
09:44:43 +0900 128)
snd_pcm_period_elapsed(substream);
c0f41bb1 sound/soc/s3c24xx/s3c24xx-pcm.c (Ben Dooks 2007-02-14
13:20:03 +0100 129)
344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02
09:44:43 +0900 130) spin_lock(&prtd->lock);
Best regards
Rickard Strandqvist
2014-05-19 12:52 GMT+02:00 Tomasz Figa <tomasz.figa@gmail.com>:
> Hi Rickard,
>
> On 15.05.2014 23:57, Rickard Strandqvist wrote:
>> There is otherwise a risk of a possible null pointer dereference.
>>
>> Was largely found by using a static code analysis program called cppcheck.
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> ---
>> sound/soc/samsung/dma.c | 10 ++++++----
>> 1 fil ändrad, 6 tillägg(+), 4 borttagningar(-)
>>
>> diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c
>> index dc09b71..b1f6757 100644
>> --- a/sound/soc/samsung/dma.c
>> +++ b/sound/soc/samsung/dma.c
>> @@ -115,17 +115,19 @@ static void dma_enqueue(struct snd_pcm_substream *substream)
>> static void audio_buffdone(void *data)
>> {
>> struct snd_pcm_substream *substream = data;
>> - struct runtime_data *prtd = substream->runtime->private_data;
>> + struct runtime_data *prtd = NULL;
>>
>> pr_debug("Entered %s\n", __func__);
>>
>> - if (prtd->state & ST_RUNNING) {
>> + if(substream)
>> + prtd = substream->runtime->private_data;
>> +
>> + if (prtd && prtd->state & ST_RUNNING) {
>> prtd->dma_pos += prtd->dma_period;
>> if (prtd->dma_pos >= prtd->dma_end)
>> prtd->dma_pos = prtd->dma_start;
>>
>> - if (substream)
>> - snd_pcm_period_elapsed(substream);
>> + snd_pcm_period_elapsed(substream);
>>
>> spin_lock(&prtd->lock);
>> if (!samsung_dma_has_circular()) {
>>
>
> Can you find a path (or use case, if possible) on which any of affected
> pointers will be NULL?
>
> Best regards,
> Tomasz
Hi Rickard, On 20.05.2014 21:12, Rickard Strandqvist wrote: > Hi Tomasz > > What I based my patch on is really because of this line: > if (substream) > snd_pcm_period_elapsed(substream); > > Boojin Kim thought that this was needed, if this is true anymore..? I > have not been able to immerse myself so much in all patches. > I'm working on about 100 similar patches. To me having NULL as either data argument of buffer done callback or private_data would be a serious driver bug and IMHO it's better to let it crash with a NULL pointer dereference to let someone notice than mask it by adding a condition. Still, I'm not too experienced with ALSA and ASoC, so I might be wrong. Mark, what do you think about this? Best regards, Tomasz > > 344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02 > 09:44:43 +0900 123) prtd->dma_pos += prtd->dma_period; > 344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02 > 09:44:43 +0900 124) if (prtd->dma_pos >= prtd->dma_end) > 344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02 > 09:44:43 +0900 125) prtd->dma_pos = > prtd->dma_start; > 5111c075 sound/soc/s3c24xx/s3c24xx-pcm.c (Mark Brown 2008-04-30 > 17:19:32 +0200 126) > 344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02 > 09:44:43 +0900 127) if (substream) > 344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02 > 09:44:43 +0900 128) > snd_pcm_period_elapsed(substream); > c0f41bb1 sound/soc/s3c24xx/s3c24xx-pcm.c (Ben Dooks 2007-02-14 > 13:20:03 +0100 129) > 344b4c48 sound/soc/samsung/dma.c (Boojin Kim 2011-09-02 > 09:44:43 +0900 130) spin_lock(&prtd->lock); > > > Best regards > Rickard Strandqvist > > > 2014-05-19 12:52 GMT+02:00 Tomasz Figa <tomasz.figa@gmail.com>: >> Hi Rickard, >> >> On 15.05.2014 23:57, Rickard Strandqvist wrote: >>> There is otherwise a risk of a possible null pointer dereference. >>> >>> Was largely found by using a static code analysis program called cppcheck. >>> >>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> >>> --- >>> sound/soc/samsung/dma.c | 10 ++++++---- >>> 1 fil ändrad, 6 tillägg(+), 4 borttagningar(-) >>> >>> diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c >>> index dc09b71..b1f6757 100644 >>> --- a/sound/soc/samsung/dma.c >>> +++ b/sound/soc/samsung/dma.c >>> @@ -115,17 +115,19 @@ static void dma_enqueue(struct snd_pcm_substream *substream) >>> static void audio_buffdone(void *data) >>> { >>> struct snd_pcm_substream *substream = data; >>> - struct runtime_data *prtd = substream->runtime->private_data; >>> + struct runtime_data *prtd = NULL; >>> >>> pr_debug("Entered %s\n", __func__); >>> >>> - if (prtd->state & ST_RUNNING) { >>> + if(substream) >>> + prtd = substream->runtime->private_data; >>> + >>> + if (prtd && prtd->state & ST_RUNNING) { >>> prtd->dma_pos += prtd->dma_period; >>> if (prtd->dma_pos >= prtd->dma_end) >>> prtd->dma_pos = prtd->dma_start; >>> >>> - if (substream) >>> - snd_pcm_period_elapsed(substream); >>> + snd_pcm_period_elapsed(substream); >>> >>> spin_lock(&prtd->lock); >>> if (!samsung_dma_has_circular()) { >>> >> >> Can you find a path (or use case, if possible) on which any of affected >> pointers will be NULL? >> >> Best regards, >> Tomasz
On Tue, May 20, 2014 at 09:16:59PM +0200, Tomasz Figa wrote: > To me having NULL as either data argument of buffer done callback or > private_data would be a serious driver bug and IMHO it's better to let > it crash with a NULL pointer dereference to let someone notice than mask > it by adding a condition. > Please, no. Regardless of who is at fault, crashes in library code are insanely frustrating to application developers. Error codes can be handled nicely and expressed to the user as an error message. Crashes are hideous for end users and usually end up being support nightmares for developers. Related: https://git.kernel.org/cgit/linux/kernel/git/kay/libabc.git/tree/README?id=0573b57c7bdb4c82d6acd62f7923a6503f4f679a#n126 Andrew
On 20.05.2014 21:29, Andrew Eikum wrote: > On Tue, May 20, 2014 at 09:16:59PM +0200, Tomasz Figa wrote: >> To me having NULL as either data argument of buffer done callback or >> private_data would be a serious driver bug and IMHO it's better to let >> it crash with a NULL pointer dereference to let someone notice than mask >> it by adding a condition. >> > > Please, no. Regardless of who is at fault, crashes in library code > are insanely frustrating to application developers. Error codes can > be handled nicely and expressed to the user as an error message. > Crashes are hideous for end users and usually end up being support > nightmares for developers. How is the related code a library? Best regards, Tomasz
On 05/20/2014 09:16 PM, Tomasz Figa wrote: > Hi Rickard, > > On 20.05.2014 21:12, Rickard Strandqvist wrote: >> Hi Tomasz >> >> What I based my patch on is really because of this line: >> if (substream) >> snd_pcm_period_elapsed(substream); >> >> Boojin Kim thought that this was needed, if this is true anymore..? I >> have not been able to immerse myself so much in all patches. >> I'm working on about 100 similar patches. > > To me having NULL as either data argument of buffer done callback or > private_data would be a serious driver bug and IMHO it's better to let > it crash with a NULL pointer dereference to let someone notice than mask > it by adding a condition. > > Still, I'm not too experienced with ALSA and ASoC, so I might be wrong. > Mark, what do you think about this? Given that there is a patch[1] which removes the whole file I think we can stop the discussion about this patch here. But for the record, substream will never ever be NULL in this function. prtd might be though if the DMA completion callback races against the closing of the PCM stream. [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2014-May/076860.html
On Tue, May 20, 2014 at 11:21:08PM +0200, Lars-Peter Clausen wrote: > On 05/20/2014 09:16 PM, Tomasz Figa wrote: > >Still, I'm not too experienced with ALSA and ASoC, so I might be wrong. > >Mark, what do you think about this? > Given that there is a patch[1] which removes the whole file I think we can > stop the discussion about this patch here. > But for the record, substream will never ever be NULL in this function. prtd > might be though if the DMA completion callback races against the closing of > the PCM stream. Yeah, what Lars said - obviously the patch removing the driver is more comprehensive! For the record it's very helpful to be specific about what the problem being addressed is in the changelog, if it's not obvious what the bug that's being fixed is that'll slow things down. Please also use subject lines appropriate for the subsystem, it will help your patch get noticed.
diff --git a/sound/soc/samsung/dma.c b/sound/soc/samsung/dma.c index dc09b71..b1f6757 100644 --- a/sound/soc/samsung/dma.c +++ b/sound/soc/samsung/dma.c @@ -115,17 +115,19 @@ static void dma_enqueue(struct snd_pcm_substream *substream) static void audio_buffdone(void *data) { struct snd_pcm_substream *substream = data; - struct runtime_data *prtd = substream->runtime->private_data; + struct runtime_data *prtd = NULL; pr_debug("Entered %s\n", __func__); - if (prtd->state & ST_RUNNING) { + if(substream) + prtd = substream->runtime->private_data; + + if (prtd && prtd->state & ST_RUNNING) { prtd->dma_pos += prtd->dma_period; if (prtd->dma_pos >= prtd->dma_end) prtd->dma_pos = prtd->dma_start; - if (substream) - snd_pcm_period_elapsed(substream); + snd_pcm_period_elapsed(substream); spin_lock(&prtd->lock); if (!samsung_dma_has_circular()) {
There is otherwise a risk of a possible null pointer dereference. Was largely found by using a static code analysis program called cppcheck. Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> --- sound/soc/samsung/dma.c | 10 ++++++---- 1 fil ändrad, 6 tillägg(+), 4 borttagningar(-)