[Home]

Summary:ASTERISK-01062: [src audit] replacement for strncpy() function used everywhere in asterisk
Reporter:Rob Gagnon (rgagnon)Labels:
Date Opened:2004-02-20 14:48:30.000-0600Date Closed:2011-06-07 14:10:27
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description:the use strncpy() in asterisk has the possibility of leading to memory leaks and core dumps because some strings strncpy()'d may not be null terminated.

****** ADDITIONAL INFORMATION ******

I noticed most strncpy() calls are called as:

strncpy(dest, src, sizeof(dest) - 1);

which is good, but not perfect... The "-1" on the end does account for the strncpy() function not leaving a terminating null character, but it does not help you if the string was not previously zero-filled.

Example:
char dest[11] = "";
char src[11] = "1234567890";
strncpy(dest,src,sizeof(dest) - 1);

IF memory for byte dest[10] is not NULL, it will remain non-NULL after src is copied into it.

suggest this utility function to be added to core, and then wherever you need it, call it without using the "-1" following sizeof()

Ex:
The above example would be replaced with:
strNcpy(dest, src, sizeof(dest));

Going forward using this function should eliminate logical bugs from happening when a strncpy() would normally have been used, and very difficult to find.

==================================================
/*
* Replacement for strncpy() guarantees a null
* terminated dest string
*/
char *
strNcpy(char *dest, char *src, int n)
{
       if (n > 0) {
               strncpy(dest, src, n);
       } else {
               n = 1;
       }
       dest[n - 1] = 0;

       return dest;
}
Comments:By: James Golovich (jamesgolovich) 2004-02-21 13:43:32.000-0600

I don't know if this will get applied or not, but to follow the standard convention in asterisk the function should be called ast_strncpy
By: Rob Gagnon (rgagnon) 2004-03-07 05:07:27.000-0600

I guess that would make sense for a name.  I just thought I'd suggest something like this as I use it almost all the time myself in place of strncpy() to save myself that one-byte logical bug headache trying to find where the string got too long at.
By: Rob Gagnon (rgagnon) 2004-03-09 16:57:50.000-0600

I see something is being done about strncpy() elsewhere... bug ASTERISK-1111152 was just resoved under this very issue.
By: James Golovich (jamesgolovich) 2004-03-09 17:59:39.000-0600

Yeah bug 1152 just fixed the proper use of strncpy which I think is a better thing to do than to add this new function.
By: Rob Gagnon (rgagnon) 2004-03-09 18:05:04.000-0600

right, but the existing problem still exists:

What if the last byte of the buffer being copied to is not a NULL?

(rare, but when it happens... ka-ploooey!)

edited on: 03-09-04 16:53
By: James Golovich (jamesgolovich) 2004-03-09 18:23:10.000-0600

In that case then the code needs to be fixed.  This is going to have to be a call Mark makes. I just think that its not necessary and we should strive to have good code in the first place.
By: Brian West (bkw918) 2004-04-17 23:00:13

Can you bug kram about this?  Please do and i'm going to close this for now.

bkw