[asterisk-dev] [Code Review]: Convert two classes of strncpy usage to ast_copy_string

[Terry Wilson]

> On Feb. 11, 2012, 1:27 a.m., Tilghman Lesher wrote:
> > /trunk/apps/app_rpt.c, line 5809
> > <https://reviewboard.asterisk.org/r/1732/diff/1/?file=24082#file24082line5809>
> >
> >     Would be nice to get rid of the red blobs, especially on lines that you're editing.

It would be nice, but there isn't an option to do that automatically! That sounds like work! I suppose I can make a quick run though and fix it though.


> On Feb. 11, 2012, 1:27 a.m., Tilghman Lesher wrote:
> > /trunk/res/ael/ael_lex.c, lines 1972-1973
> > <https://reviewboard.asterisk.org/r/1732/diff/1/?file=24085#file24085line1972>
> >
> >     Keep the -1 on the end here, or you'll copy the ending quote, which is not what you want.  However, you CAN get rid of the line following, as it becomes redundant with the use of ast_copy_string().

Yeah, I think I'll limit the -1 case to where it is sizeof(src) - 1 just in case there are others we miss.


> On Feb. 11, 2012, 1:27 a.m., Tilghman Lesher wrote:
> > /trunk/res/snmp/agent.c, lines 272-274
> > <https://reviewboard.asterisk.org/r/1732/diff/1/?file=24086#file24086line272>
> >
> >     As in the previous file, once you change to ast_copy_string, you can get rid of the following line, which redundantly places a terminating NULL.

I think I can actually add something to the "spatch" that takes care of these as well.


> On Feb. 11, 2012, 1:27 a.m., Tilghman Lesher wrote:
> > /trunk/utils/db1-ast/hash/ndbm.c, line 82
> > <https://reviewboard.asterisk.org/r/1732/diff/1/?file=24089#file24089line82>
> >
> >     You can drop the (void).  ast_copy_string() already returns void.

Actually, this is code outside of Asterisk code, I should really leave it alone.


> On Feb. 11, 2012, 1:27 a.m., Tilghman Lesher wrote:
> > /trunk/utils/muted.c, lines 152-153
> > <https://reviewboard.asterisk.org/r/1732/diff/1/?file=24090#file24090line152>
> >
> >     Why are we wrapping these lines, when the following lines are much longer?

I experimented with removing the -smpl_spacing and apparently it does silly things without that.


> On Feb. 11, 2012, 1:27 a.m., Tilghman Lesher wrote:
> > /trunk/apps/app_rpt.c, lines 13057-13064
> > <https://reviewboard.asterisk.org/r/1732/diff/1/?file=24082#file24082line13057>
> >
> >     Holy lack of curley braces, Batman!

I didn't remove them...you are missing the point of an automatic tool. :-D


- Terry


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1732/#review5481
-----------------------------------------------------------


On Feb. 10, 2012, 8:29 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1732/
> -----------------------------------------------------------
> 
> (Updated Feb. 10, 2012, 8:29 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> One of the janitor projects is to replace strncpy with ast_copy_string where possible. As a brief example of using the coccinelle tool, I thought I'd post an example for using it. There are some cases of strncpy which where ast_copy_string wouldn't be appropriate--sepcifically when it is used to copy non-NULL terminated buffers (instead of memcpy, which should be used for that). Two cases that I think should be completely safe are where we have something like strncpy(src, dst, ${something} - 1) and strncpy(src, dst, sizeof(src)) (which is a bug waiting to happen). 
> 
> Here is the coccinelle patch to do the translation:
> 
> strncpy.cocci
> @@
> expression a, b, c;
> @@
> 
> - strncpy(a, b, c - 1)
> + ast_copy_string(a, b, c)
> 
> @@
> expression a, b;
> @@
> 
> - strncpy(a, b, sizeof(a))
> + ast_copy_string(a, b, sizeof(a))
> 
> Coccinelle needs a little help with some of the macros in Asterisk--so far it looks like the ones in linkedlists.h and app.h give it the most trouble. You can generate a macro file to use by doing:
> 
> spatch -extract_macros include/asterisk/linkedlists.h > /tmp/spatch.h
> spatch -extract_macros include/asterisk/app.h >> /tmp/spatch.h
> 
> I've found that doing all of the includes sometimes makes it miss stuff for some reason...
> 
> Then to run the patch file on all the files in the asterisk directory:
> spatch -sp_file strncpy.cocci -preprocess -c -in_place -I include/ -local_includes -macro_file /tmp/spatch.h -smpl_spacing -dir .
> 
> You may have to fix some stuff up manually, but it goes a long way to making things a lot easier. This is a simple example of how the channel opaquification is done. I'll write this all up on the Wiki, but am basically just doing a quick brain dump before I leave for the night.
> 
> 
> Diffs
> -----
> 
>   /trunk/addons/chan_ooh323.c 354785 
>   /trunk/addons/ooh323c/src/ooCalls.c 354785 
>   /trunk/addons/ooh323c/src/ooCmdChannel.c 354785 
>   /trunk/addons/ooh323c/src/ooh323ep.c 354785 
>   /trunk/addons/ooh323c/src/ooq931.c 354785 
>   /trunk/apps/app_rpt.c 354785 
>   /trunk/channels/iax2-provision.c 354785 
>   /trunk/pbx/dundi-parser.c 354785 
>   /trunk/res/ael/ael_lex.c 354785 
>   /trunk/res/snmp/agent.c 354785 
>   /trunk/utils/ael_main.c 354785 
>   /trunk/utils/astman.c 354785 
>   /trunk/utils/db1-ast/hash/ndbm.c 354785 
>   /trunk/utils/muted.c 354785 
> 
> Diff: https://reviewboard.asterisk.org/r/1732/diff
> 
> 
> Testing
> -------
> 
> It compiles.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120211/d7420d33/attachment-0001.htm>