Partager via

Evils of strncat and strncpy - Answers

  • Article

Ok, so I took a little longer than expected to post the answers, but here they are. BTW, many people worked them out :)

// Example #1 (code prior to this verifies pszSrc is <= 50 chars)
#define MAX (50)
char *pszDest = malloc(sizeof(pszSrc));
strncpy(pszDest,pszSrc,MAX);
The code is allocating the size of a pointer, 4-bytes on a 32-bit CPU, and then trying to copy 40 bytes.

// Example #2
#define MAX (50)
char szDest[MAX];
strncpy(szDest,pszSrc,MAX);
If the length of the string pointed to by pszSrc is exactly MAX, then strncpy does NOT null-terminate szDest.

// Example #3
#define MAX (50)
char szDest[MAX];
strncpy(szDest,pszSrc,MAX);
pszDest[MAX] = '\0';
Oooops - we just whacked element 51, not 50!

// Example #4
#define MAX (50)
char szDest[MAX];
strncpy(szDest,pszSrc,MAX-1);
strncat(szDest,pszSrc,MAX-1);
The last arg to strncat is not the total length of szDest, it's how much space REMAINS!

// Example #5
char szDest[50];
_snprintf(szDest, strlen(szDest), "%s",szSrc);
szDest hasn't been initialized yet, so strlen(szDest) could return any value!

// Example #6
#define MAX (50)
void func(char *p) {
char szDest[MAX];
strncpy(szDest,p,MAX);
szDest[MAX-1] = '\0';
}
If p == NULL, you're app just died!

Comments

  • Anonymous
    December 10, 2004
    For #6: If p == (char *)6, your app still dies. And your check for (if (p == NULL)) did nothing to fix it.

    So what's a poor programmer to do?

    This is actually a serious question phrased in a snarky way.

    One solution is to just check for null since its the common case and ignore the others.

    The other solution is to use SEH to capture the parameters. But that has its own issues.

    Suggestions? I personally prefer the first one, since SEH as a parameter validation mechanism (can you say "corrupt memory and crash my app"?) is so heinous, but...
  • Anonymous
    December 12, 2004
    Some corrections:
    - The comment in example 1 says 40 iso 50
    - example 4 suffers from the same bug as example 2
    - example 5 is totally wrong, despite the buffer being initialized or not, it should say sizeof() (or sizeof() -1 if the implemented snprintf() is not compilant with standards) using strlen in this case is just silly.
  • Anonymous
    December 13, 2004
    #define MAX (50)
    void func(char *p) {
        char szDest[MAX];
        strncpy(szDest,p,MAX);
        szDest[MAX-1] = '�';
    }
    If p == NULL, you're app just died!

    ----------

    Good arguments for asserts in functions, but at the same time, you have to be able to trust your callers at some point; witness the kernel's design philosophy.

    last line: s/you're/your/
  • Anonymous
    March 29, 2006
    One of the cool new things we are doing in the security push is the conversion of all uses of potentially...