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-0600 | Date Closed: | 2011-06-07 14:10:27 |
Priority: | Minor | Regression? | No |
Status: | Closed/Complete | Components: | 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 |