Summary:ASTERISK-01965: [patch][src-audit] chan_sip.c: Fixed possible buffer overruns, general cleanup
Reporter:Rob Gagnon (rgagnon)Labels:
Date Opened:2004-07-07 14:51:41Date Closed:2008-01-15 15:01:51.000-0600
Versions:Frequency of
Environment:Attachments:( 0) chan_sip.c.patch.txt
Description:Similar to what I did with ASTERISK-1941977 for app_voicemail.c, this time for chan_sip.c:

Searched for all occurances of the following:  snprintf, sprintf, strcpy, strncpy, strcat, strncat, and made sure they are protected properly.

- Almost all sprintf's changed to snprintf.
- Almost all strcpy's changed to strncpy.
- Same for strcat's being changed to strncat.

Changed ALL strncat's so they have the needed "-1" for the last paramter, as you could end up with strncat writing a null character outside your buffer (I tested that operation, and it would do that too)

Also, modified names of global_xxx variables to keep a naming convention:

IE: We had "globaldtmfmode" next to "global_realm", so now all globals contain the underscore ("_")

Found some places where the new ast_inet_ntoa() function's functionality was not fully taken advantage of.  What I mean here is something like this:

strncpy(buff, ast_inet_ntoa(iabuf, sizeof(iabuf), sin_addr), sizeof(buff) - 1);

which can be replaced with:

ast_inet_ntoa(buff, sizeof(buff), sinaddr);

Thus, optimizing the code, and sometimes eliminating a variable.

The patch is kinda big, so I would ask it be reviewed before commit.  I have tested it minimally due to all the changes being basically trivial, with no change in functionality.


Comments:By: Olle Johansson (oej) 2004-07-07 16:14:57

What is the difference between global_ and default_ ?

Otherwise great patch from reading it.

I'll test it and see if I can make any more comments after testing. Will also check against chan_sip2 where I made similar changes a while ago to improve readability of the code. I think there is a few additional changes I would like to make.

By: Rob Gagnon (rgagnon) 2004-07-07 16:29:10

It was kinda difficult to find a reason between either global_ or default_  except to say that I might have used default_ because, in a couple of places, the comments mentioned "/* Setup default settings */" or something similar.

I guess one name could be chosen over the other, OR.... we could define a struct local to the file called "globals" and place all of them in there.

Then global_dtmfmode would become globals.dtmfmode, which could be handy for people editting code with a c-tags.

By: Mark Spencer (markster) 2004-07-08 07:19:15

Looks good to me.  I checked the patch and put it in.

By: Digium Subversion (svnbot) 2008-01-15 15:01:51.000-0600

Repository: asterisk
Revision: 3392

U   trunk/channels/chan_sip.c

r3392 | markster | 2008-01-15 15:01:51 -0600 (Tue, 15 Jan 2008) | 2 lines

Cleanup SIP formatting, strncpy's strncats and global variable names (bug ASTERISK-1965)