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:41	Date Closed:	2008-01-15 15:01:51.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
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. ****** ADDITIONAL INFORMATION ****** [disclaimed]
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) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=3392