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

[Tilghman Lesher]

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



/trunk/apps/app_rpt.c
<https://reviewboard.asterisk.org/r/1732/#comment10087>

    Would be nice to get rid of the red blobs, especially on lines that you're editing.



/trunk/apps/app_rpt.c
<https://reviewboard.asterisk.org/r/1732/#comment10088>

    Holy lack of curley braces, Batman!



/trunk/res/ael/ael_lex.c
<https://reviewboard.asterisk.org/r/1732/#comment10090>

    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().



/trunk/res/snmp/agent.c
<https://reviewboard.asterisk.org/r/1732/#comment10091>

    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.



/trunk/utils/db1-ast/hash/ndbm.c
<https://reviewboard.asterisk.org/r/1732/#comment10093>

    You can drop the (void).  ast_copy_string() already returns void.



/trunk/utils/muted.c
<https://reviewboard.asterisk.org/r/1732/#comment10092>

    Why are we wrapping these lines, when the following lines are much longer?


- Tilghman


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/a91250b5/attachment-0001.htm>