[PATCH] Fix for possible null pointer dereference in dma.c

[PATCH] Fix for possible null pointer dereference in dma.c

[Rickard Strandqvist]

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()) {
-- 
1.7.10.4

[PATCH] Fix for possible null pointer dereference in dma.c

[Rickard Strandqvist]

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()) {
-- 
1.7.10.4

Re: [PATCH] Fix for possible null pointer dereference in dma.c

[Lothar Waßmann]

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

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

[PATCH] Fix for possible null pointer dereference in dma.c

[Lothar Waßmann]

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

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

Re: [PATCH] Fix for possible null pointer dereference in dma.c

[Tomasz Figa]

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

[PATCH] Fix for possible null pointer dereference in dma.c

[Tomasz Figa]

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

Re: [PATCH] Fix for possible null pointer dereference in dma.c

[Rickard Strandqvist]

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

[PATCH] Fix for possible null pointer dereference in dma.c

[Rickard Strandqvist]

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

Re: [PATCH] Fix for possible null pointer dereference in dma.c

[Tomasz Figa]

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

[PATCH] Fix for possible null pointer dereference in dma.c

[Tomasz Figa]

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

Re: [alsa-devel] [PATCH] Fix for possible null pointer dereference in dma.c

[Andrew Eikum]

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

Re: [PATCH] Fix for possible null pointer dereference in dma.c

[Andrew Eikum]

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

[alsa-devel] [PATCH] Fix for possible null pointer dereference in dma.c

[Andrew Eikum]

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

Re: [alsa-devel] [PATCH] Fix for possible null pointer dereference in dma.c

[Tomasz Figa]

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

[alsa-devel] [PATCH] Fix for possible null pointer dereference in dma.c

[Tomasz Figa]

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

Re: [alsa-devel] [PATCH] Fix for possible null pointer dereference in dma.c

[Lars-Peter Clausen]

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

[alsa-devel] [PATCH] Fix for possible null pointer dereference in dma.c

[Lars-Peter Clausen]

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

Re: [alsa-devel] [PATCH] Fix for possible null pointer dereference in dma.c

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[alsa-devel] [PATCH] Fix for possible null pointer dereference in dma.c

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140520/f0995ade/attachment.sig>