[Cocci] Checking statement order for patch generation with SmPL support

[Cocci] Checking statement order for patch generation with SmPL support

[SF Markus Elfring]

Hello,

I have constructed another small script for the semantic patch language.

@usage@
identifier action, member, release=~"^.+free$";
expression context;
@@
*release(context);
 <+...
*action(..., (context)->member, ...)
 ...+>


The following source code place can be found by such a simple approach
for further software development considerations.
https://lkml.org/lkml/2017/9/6/669

elfring at Sonne:~/Projekte/Linux/next-patched> git checkout next-20170905 && spatch.opt ~/Projekte/Coccinelle/janitor/show_use_after_free1.cocci sound/pci/ymfpci/ymfpci.c
?
@@ -336,8 +336,6 @@ static int snd_card_ymfpci_probe(struct
 			legacy_ctrl &= ~YMFPCI_LEGACY_FMEN;
 			pci_write_config_word(pci, PCIR_DSXG_LEGACY, legacy_ctrl);
 		} else if ((err = snd_opl3_hwdep_new(opl3, 0, 1, NULL)) < 0) {
-			snd_card_free(card);
-			dev_err(card->dev, "cannot create opl3 hwdep\n");
 			return err;
 		}
 	}


I have tried the SmPL script out on another source file.

elfring at Sonne:~/Projekte/Linux/next-patched> spatch.opt ~/Projekte/Coccinelle/janitor/show_use_after_free1.cocci sound/core/seq/seq_queue.c
?
@@ -246,9 +246,7 @@ struct snd_seq_queue *snd_seq_queue_find
 
 	for (i = 0; i < SNDRV_SEQ_MAX_QUEUES; i++) {
 		if ((q = queueptr(i)) != NULL) {
-			if (strncmp(q->name, name, sizeof(q->name)) == 0)
 				return q;
-			queuefree(q);
 		}
 	}
 	return NULL;


Now I wonder why the software ?Coccinelle 1.0.6-00242-g3f038a5d? finds
this place relevant when the function call sequence does not fit to the order
I tried to express for a known use case.
I would appreciate further advice.

Regards,
Markus

[Cocci] Checking statement order for patch generation with SmPL support

[Julia Lawall]

On Thu, 7 Sep 2017, SF Markus Elfring wrote:

> Hello,
>
> I have constructed another small script for the semantic patch language.
>
> @usage@
> identifier action, member, release=~"^.+free$";
> expression context;
> @@
> *release(context);
>  <+...
> *action(..., (context)->member, ...)
>  ...+>
>
>
> The following source code place can be found by such a simple approach
> for further software development considerations.
> https://lkml.org/lkml/2017/9/6/669
>
> elfring at Sonne:~/Projekte/Linux/next-patched> git checkout next-20170905 && spatch.opt ~/Projekte/Coccinelle/janitor/show_use_after_free1.cocci sound/pci/ymfpci/ymfpci.c
> ?
> @@ -336,8 +336,6 @@ static int snd_card_ymfpci_probe(struct
>  			legacy_ctrl &= ~YMFPCI_LEGACY_FMEN;
>  			pci_write_config_word(pci, PCIR_DSXG_LEGACY, legacy_ctrl);
>  		} else if ((err = snd_opl3_hwdep_new(opl3, 0, 1, NULL)) < 0) {
> -			snd_card_free(card);
> -			dev_err(card->dev, "cannot create opl3 hwdep\n");
>  			return err;
>  		}
>  	}
>
>
> I have tried the SmPL script out on another source file.
>
> elfring at Sonne:~/Projekte/Linux/next-patched> spatch.opt ~/Projekte/Coccinelle/janitor/show_use_after_free1.cocci sound/core/seq/seq_queue.c
> ?
> @@ -246,9 +246,7 @@ struct snd_seq_queue *snd_seq_queue_find
>
>  	for (i = 0; i < SNDRV_SEQ_MAX_QUEUES; i++) {
>  		if ((q = queueptr(i)) != NULL) {
> -			if (strncmp(q->name, name, sizeof(q->name)) == 0)
>  				return q;
> -			queuefree(q);
>  		}
>  	}
>  	return NULL;
>
>
> Now I wonder why the software ?Coccinelle 1.0.6-00242-g3f038a5d? finds
> this place relevant when the function call sequence does not fit to the order
> I tried to express for a known use case.
> I would appreciate further advice.

Because there is a loop, and you did nothing to prevent an update to q
because the free and the dereference.

The rule would be just as well as:

@usage@
identifier action, member, release=~"^.+free$";
expression context,e;
@@
*release(context);
 ... when != context = e  // to get the first result
*action(..., (context)->member, ...)

or

@usage@
identifier action, member, release=~"^.+free$";
expression context,e;
@@
*release(context);
 ... when != context = e
     when any  // to get all results
*action(..., (context)->member, ...)

julia

[Cocci] Checking statement order for patch generation with SmPL support

[SF Markus Elfring]

>> Now I wonder why the software ?Coccinelle 1.0.6-00242-g3f038a5d? finds
>> this place relevant when the function call sequence does not fit to the order
>> I tried to express for a known use case.
>> I would appreciate further advice.
> 
> Because there is a loop,

This information is appropriate.

But I have got difficulties to interpret it in an useful way.


> and you did nothing to prevent an update to q because the free and the dereference.

I omitted an additional constraint for a simple test.


> @usage@
> identifier action, member, release=~"^.+free$";
> expression context,e;
> @@
> *release(context);
>  ... when != context = e
>      when any  // to get all results
> *action(..., (context)->member, ...)

Should the SmPL construct ?<+.. ...+>? work also similar to your suggestion?

Regards,
Markus

[Cocci] Checking statement order for patch generation with SmPL support

[Julia Lawall]

On Thu, 7 Sep 2017, SF Markus Elfring wrote:

> >> Now I wonder why the software ?Coccinelle 1.0.6-00242-g3f038a5d? finds
> >> this place relevant when the function call sequence does not fit to the order
> >> I tried to express for a known use case.
> >> I would appreciate further advice.
> >
> > Because there is a loop,
>
> This information is appropriate.
>
> But I have got difficulties to interpret it in an useful way.


Coccinelle follows control-flow paths.  Thers is a control-flow path from
the bottom of a loop back up to the top.

>
>
> > and you did nothing to prevent an update to q because the free and the dereference.
>
> I omitted an additional constraint for a simple test.
>
>
> > @usage@
> > identifier action, member, release=~"^.+free$";
> > expression context,e;
> > @@
> > *release(context);
> >  ... when != context = e
> >      when any  // to get all results
> > *action(..., (context)->member, ...)
>
> Should the SmPL construct ?<+.. ...+>? work also similar to your suggestion?

<+... ...+> would also not allow context = e after the last match of the
pattern inside the nest.  A when on a <+... ...+> applies to the entire
matched region.

julia

[Cocci] Checking statement order for patch generation with SmPL support

[SF Markus Elfring]

>> But I have got difficulties to interpret it in an useful way.
> 
> Coccinelle follows control-flow paths.

This information is generally fine.


> Thers is a control-flow path from the bottom of a loop back up to the top.

I can not follow with my intermediate understanding to such a view
at the moment.

Why was the combination of a call for the function ?strncmp? before a ?queuefree?
really displayed as an update candidate?


>>> @usage@
>>> identifier action, member, release=~"^.+free$";
>>> expression context,e;
>>> @@
>>> *release(context);
>>>  ... when != context = e
>>>      when any  // to get all results
>>> *action(..., (context)->member, ...)
>>
>> Should the SmPL construct ?<+.. ...+>? work also similar to your suggestion?
> 
> <+... ...+> would also not allow context = e after the last match of the
> pattern inside the nest.

This information sounds promising.

I am looking still for possibilities to clarify the overlap better
in the shown functionality.


> A when on a <+... ...+> applies to the entire matched region.

Can the parameter ?when? be combined with this construct anyhow?

Regards,
Markus

[Cocci] Checking statement order for patch generation with SmPL support

[SF Markus Elfring]

> Thers is a control-flow path from the bottom of a loop back up to the top.

I wonder also about the information how an ordinary for loop could influence
the shown source code analysis result for the function ?snd_seq_queue_find_name?
when the questionable marked statements are contained in a single if branch.
http://elixir.free-electrons.com/linux/v4.13/source/sound/core/seq/seq_queue.c#L241
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/sound/core/seq/seq_queue.c?id=c6be5a0e3cebc145127d46a58350e05d2bcf6323#n252


I have tried the following SmPL script variant out on another source file.

@usage@
identifier action!~"^.+free$", member, release=~"^.+free$";
expression context, ex;
@@
*release(context);
 ... when != context = ex
     when any
*action(..., (context)->member, ...)


elfring at Sonne:~/Projekte/Linux/next-patched> spatch.opt ~/Projekte/Coccinelle/janitor/show_use_after_free3.cocci sound/soc/soc-pcm.c
?
@@ -1199,14 +1199,11 @@ void dpcm_be_disconnect(struct snd_soc_p
 			stream ? "<-" : "->", dpcm->be->dai_link->name);
 
 		/* BEs still alive need new FE */
-		dpcm_be_reparent(fe, dpcm->be, stream);
 
 #ifdef CONFIG_DEBUG_FS
-		debugfs_remove(dpcm->debugfs_state);
 #endif
 		list_del(&dpcm->list_be);
 		list_del(&dpcm->list_fe);
-		kfree(dpcm);
 	}
 }


I find the shown matches also questionable for this test result.
Would you like to clarify such software situations a bit more
for the desired handling of statement sequences?

Regards,
Markus

[Cocci] Checking statement order for patch generation with SmPL support

[Julia Lawall]

On Thu, 7 Sep 2017, SF Markus Elfring wrote:

> > Thers is a control-flow path from the bottom of a loop back up to the top.
>
> I wonder also about the information how an ordinary for loop could influence
> the shown source code analysis result for the function ?snd_seq_queue_find_name?
> when the questionable marked statements are contained in a single if branch.
> http://elixir.free-electrons.com/linux/v4.13/source/sound/core/seq/seq_queue.c#L241
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/sound/core/seq/seq_queue.c?id=c6be5a0e3cebc145127d46a58350e05d2bcf6323#n252

I don't understand the question.  I already explained the issue here.
Execution can go from queuefree to the top of the loop, to the first if in
the loop to the second if in the loop that has the dereference.  That is
how loops work.

>
> I have tried the following SmPL script variant out on another source file.
>
> @usage@
> identifier action!~"^.+free$", member, release=~"^.+free$";
> expression context, ex;
> @@
> *release(context);
>  ... when != context = ex
>      when any
> *action(..., (context)->member, ...)
>
>
> elfring at Sonne:~/Projekte/Linux/next-patched> spatch.opt ~/Projekte/Coccinelle/janitor/show_use_after_free3.cocci sound/soc/soc-pcm.c
> ?
> @@ -1199,14 +1199,11 @@ void dpcm_be_disconnect(struct snd_soc_p
>  			stream ? "<-" : "->", dpcm->be->dai_link->name);
>
>  		/* BEs still alive need new FE */
> -		dpcm_be_reparent(fe, dpcm->be, stream);
>
>  #ifdef CONFIG_DEBUG_FS
> -		debugfs_remove(dpcm->debugfs_state);
>  #endif
>  		list_del(&dpcm->list_be);
>  		list_del(&dpcm->list_fe);
> -		kfree(dpcm);
>  	}
>  }
>
>
> I find the shown matches also questionable for this test result.
> Would you like to clarify such software situations a bit more
> for the desired handling of statement sequences?

The list_for_each_entry_safe operator also makes a loop.

julia

[Cocci] Checking statement order for patch generation with SmPL support

[SF Markus Elfring]

>> I wonder also about the information how an ordinary for loop could influence
>> the shown source code analysis result for the function ?snd_seq_queue_find_name?
>> when the questionable marked statements are contained in a single if branch.
>> http://elixir.free-electrons.com/linux/v4.13/source/sound/core/seq/seq_queue.c#L241
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/sound/core/seq/seq_queue.c?id=c6be5a0e3cebc145127d46a58350e05d2bcf6323#n252
> 
> I don't understand the question.

I hope that we can achieve a better common understanding also for the mentioned
test examples.


> I already explained the issue here.

You tried it.


> Execution can go from queuefree to the top of the loop, to the first if in
> the loop to the second if in the loop that has the dereference.  That is
> how loops work.

I can agree to this view.

But I find the existence of a loop not so relevant for the source code
search pattern which is discussed.


>> @@ -1199,14 +1199,11 @@ void dpcm_be_disconnect(struct snd_soc_p
>>  			stream ? "<-" : "->", dpcm->be->dai_link->name);
>>
>>  		/* BEs still alive need new FE */
>> -		dpcm_be_reparent(fe, dpcm->be, stream);
>>
>>  #ifdef CONFIG_DEBUG_FS
>> -		debugfs_remove(dpcm->debugfs_state);
>>  #endif
>>  		list_del(&dpcm->list_be);
>>  		list_del(&dpcm->list_fe);
>> -		kfree(dpcm);
>>  	}
>>  }
>>
>>
>> I find the shown matches also questionable for this test result.
>> Would you like to clarify such software situations a bit more
>> for the desired handling of statement sequences?
> 
> The list_for_each_entry_safe operator also makes a loop.

Yes. - But how could the Coccinelle software know more about this identifier
during execution of the small script ?show_use_after_free3.cocci? than
that it is a macro call in the implementation of the function ?dpcm_be_disconnect?
(when extra include parameters were not specified)?
http://elixir.free-electrons.com/linux/v4.13/source/include/linux/list.h#L542
http://elixir.free-electrons.com/linux/v4.13/source/sound/soc/soc-pcm.c#L1184

Do you find the minus characters appropriate at the beginning of these three lines?

Regards,
Markus

[Cocci] Checking statement order for patch generation with SmPL support

[Julia Lawall]

> > Execution can go from queuefree to the top of the loop, to the first if in
> > the loop to the second if in the loop that has the dereference.  That is
> > how loops work.
>
> I can agree to this view.
>
> But I find the existence of a loop not so relevant for the source code
> search pattern which is discussed.

yes it is.  If you make a pattern like

A
...
B

It matches A, and then goes forth along all control flow paths, whether
forwards or backwards, until it reaches a B.  If there is a loop, it will
go around the loop and match code that appears before A in terms of line
numbers.  The fact that in your case both A and B are in the same if
branch is irrelevant.

>
>
> >> @@ -1199,14 +1199,11 @@ void dpcm_be_disconnect(struct snd_soc_p
> >>  			stream ? "<-" : "->", dpcm->be->dai_link->name);
> >>
> >>  		/* BEs still alive need new FE */
> >> -		dpcm_be_reparent(fe, dpcm->be, stream);
> >>
> >>  #ifdef CONFIG_DEBUG_FS
> >> -		debugfs_remove(dpcm->debugfs_state);
> >>  #endif
> >>  		list_del(&dpcm->list_be);
> >>  		list_del(&dpcm->list_fe);
> >> -		kfree(dpcm);
> >>  	}
> >>  }
> >>
> >>
> >> I find the shown matches also questionable for this test result.
> >> Would you like to clarify such software situations a bit more
> >> for the desired handling of statement sequences?
> >
> > The list_for_each_entry_safe operator also makes a loop.
>
> Yes. - But how could the Coccinelle software know more about this identifier
> during execution of the small script ?show_use_after_free3.cocci? than
> that it is a macro call in the implementation of the function ?dpcm_be_disconnect?
> (when extra include parameters were not specified)?
> http://elixir.free-electrons.com/linux/v4.13/source/include/linux/list.h#L542
> http://elixir.free-electrons.com/linux/v4.13/source/sound/soc/soc-pcm.c#L1184

Coccinelle has a number of hard coded heuristics about macros, including
knowing that list_for_each and ohter similar things represent loops.

> Do you find the minus characters appropriate at the beginning of these three lines?

The behavior observed corresponds comepletely to the semantic patch you
have written.

julia