Why does this strncpy() implementation crashes on second run?

Question

Why does this strncpy() implementation crashes on second run, while the first run works ok?

strncpy

Copy characters from string Copies the first n characters of source to destination. If the end of the source C string (which is signaled by a null-character) is found before n characters have been copied, destination is padded with zeros until a total of n characters have been written to it.

No null-character is implicitly appended at the end of destination if source is longer than n (thus, in this case, destination may not be a null terminated C string).

char *strncpy(char *src, char *destStr, int n)
{
    char *save = destStr; //backing up the pointer to the first destStr char
    char *strToCopy = src; //keeps [src] unmodified

    while (n > 0)
    {
        //if [n] > [strToCopy] length (reaches [strToCopy] end),
        //adds n null-teminations to [destStr]
        if (strToCopy = '\0') 
            for (; n > 0 ; ++destStr)
                *destStr = '\0';

        *destStr = *strToCopy;
        strToCopy++;
        destStr++;
        n--;

        //stops copying when reaches [dest] end (overflow protection)
        if (*destStr == '\0')
            n = 0; //exits loop
    }

    return save;
}

/////////////////////////////////////////////

int main()
{
    char st1[] = "ABC";
    char *st2;
    char *st3 = "ZZZZZ";
    st2 = (char *)malloc(5 * sizeof(char));


    printf("Should be: ZZZZZ\n");
    st3 = strncpy(st1, st3, 0);
    printf("%s\n", st3);

    printf("Should be: ABZZZZZ\n");
    st3 = strncpy(st1, st3, 2);
    printf("%s\n", st3);

    printf("Should be: ABCZZZZZ\n");
    st3 = strncpy(st1, st3, 3);
    printf("%s\n", st3);

    printf("Should be: ABC\n");
    st3 = strncpy(st1, st3, 4);
    printf("%s\n", st3);

    printf("Should be: AB\n");
    st2 = strncpy(st1, st2, 2);
    printf("%s\n", st2);

    printf("Should be: AB\n");
    st2 = strncpy(st1, st2, 4);
    printf("%s\n", st2);
}

Please don't put the question in the title only, put it in the body of the actual question together with some descriptive text. — Some programmer dude, Nov 27 '12 at 16:03
Because you run out of allocated memory. First try is to reserve more space for `st2` string — George Gaál, Nov 27 '12 at 16:04
Every time I post a question I hope I didn't make a stupid mistake. I totally meant `if (strToCopy == '\0')` (I did leave a comment though) — tempy, Nov 27 '12 at 16:09
`if (strToCopy == '\0')` is still wrong - you probably meant `if (*strToCopy == '\0')` ? — Paul R, Nov 27 '12 at 16:10
@GeorgeGaál `strncpy()`'s "feature" of tail-filling the target buffer with 0's out to 'n' on an under-copy is probably second only to the failure to place a 0 on exact-copy or over-copy in terms of "didn't know it did that" factor. It still amazes me how many people *use* that function without knowing *either* of those features. — WhozCraig, Nov 27 '12 at 16:10

Daniel Fischer · Accepted Answer · 2012-11-27T16:10:12.150

5

You get a segmentation fault because

char *st3 = "ZZZZZ";

the destination is a string literal. String literals must not be modified, and often they are stored in write-protected memory. So when you call

strncpy(st1, st3, n);

with an n > 0, you are trying to modify the string literal and that results in a crash (not necessarily, but usually).

In the copy loop, you have forgotten to dereference strToCopy

if (strToCopy = '\0')

and wrote = instead of ==, so strToCopy is set to NULL, causing further dereferences of strToCopy to invoke undefined behaviour.

edited Nov 27 '12 at 16:10

answered Nov 27 '12 at 16:04

Daniel Fischer

181,706
17
308
431

...and string literals are `const` when written in this way. – ThiefMaster Nov 27 '12 at 16:04
Agree. Because original string isn't modified, the real function prototype must be `char *strncpy(const char *src, char *destStr, int n)` – George Gaál Nov 27 '12 at 16:08
@ThiefMaster: They're not `const` (in C, but are in C++), although modifying them is UB. – netcoder Nov 27 '12 at 16:14
Then how does it modify it on the first run? what should I do in order to be able to modify the string? Thank you for the help. – tempy Nov 27 '12 at 16:17
@tempy: You need to allocate it, either as an array (on the stack) or using `malloc` (on the heap). – netcoder Nov 27 '12 at 16:19
@tempy On the first run, you pass a length of 0, so the loop isn't run at all, and nothing bad is attempted. – Daniel Fischer Nov 27 '12 at 16:20
@netcoder I get an error that `char st3[] = "ZZZZZ";` is unmodifiable. – tempy Nov 27 '12 at 16:26
@tempy For what exact code? `char st3[] = "ZZZZZ";` declares and initialises an array of six `char`s (five 'Z' plus the 0-terminator) that may be modified. If your compiler objects to that, it violates the language standard. – Daniel Fischer Nov 27 '12 at 16:49

score 2 · Answer 2 · answered Nov 27 '12 at 16:04

2

I don't think you want this:

if (strToCopy = '\0')

Instead, what you probably meant to do is this:

if (*strToCopy == '\0')

In general, using yoda conditions will save you much headache from comparison-vs-assignment conflation issues:

if ('\0' == *strToCopy)

answered Nov 27 '12 at 16:04

sampson-chen

45,805
12
84
81

1

Better than using nasty syntax, always compile with warnings enabled, e.g. `gcc -Wall ...` - this will catch dumb mistakes like `=` instead of `==`. – Paul R Nov 27 '12 at 16:08
@PaulR Do you find Yoda Conditions to be nasty syntax? (If so, how come?) The problem with using `gcc -Wall` is that some legacy C code is written with legitimate assignment operations in the test conditions – sampson-chen Nov 27 '12 at 16:13
Yes, the Yoda syntax hack is confusing for beginners, less readable/natural and these days unnecessary. For legacy code that generates warnings then these warnings can be be preferably fixed or at least disabled on a per-file basis as necessary. – Paul R Nov 27 '12 at 16:15
1

@PaulR `-Wall -Werror` if thats the case. Shoot the messenger =P – WhozCraig Nov 27 '12 at 16:19

MOHAMED · Answer 3 · 2012-11-27T16:14:54.073

while (n > 0)
    {
        //if [n] > [strToCopy] length (and reaches [strToCopy] end),
        //adds n null-teminations to [destStr]

        *destStr = *strToCopy;
        //stops copying when reaches [dest] end (overflow protection)
        if (*destStr == '\0')
            break; //exits loop
        strToCopy++;
        destStr++;
        n--;


    }
if (*destStr != '\0') *destStr = '\0';

In the main:

printf("Should be: ZZZZZ\n");
    st3 = strncpy(st1, st3, 0);
    printf("%s\n", st3);

this wrong because the length you want to copy is 0 so you will not obtain ZZZZZ

Why does this strncpy() implementation crashes on second run?

strncpy

3 Answers3