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

[Terry Wilson]

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

(Updated March 8, 2012, 6:09 p.m.)


Review request for Asterisk Developers.


Changes
-------

Do a safer set of changes and remove unneeded a[sizeof(a) -1] = '\0' lines:

@@
expression a, b;
@@

- strncpy(a, b, sizeof(a) - 1)
+ ast_copy_string(a, b, sizeof(a))

@@
expression a, b;
@@

- strncpy(a, b, sizeof(a))
+ ast_copy_string(a, b, sizeof(a))

@@
expression a, b;
@@

ast_copy_string(a, b, sizeof(a));
- a[sizeof(a) - 1] = '\0';


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

  /trunk/addons/chan_ooh323.c 358725 
  /trunk/addons/ooh323c/src/ooCalls.c 358725 
  /trunk/addons/ooh323c/src/ooCmdChannel.c 358725 
  /trunk/addons/ooh323c/src/ooh323ep.c 358725 
  /trunk/addons/ooh323c/src/ooq931.c 358725 
  /trunk/apps/app_alarmreceiver.c 358725 
  /trunk/apps/app_rpt.c 358725 
  /trunk/channels/iax2-provision.c 358725 
  /trunk/res/snmp/agent.c 358725 
  /trunk/utils/ael_main.c 358725 
  /trunk/utils/astman.c 358725 
  /trunk/utils/muted.c 358725 

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/20120309/13821b60/attachment.htm>