strncpy only copies part of string

Question

Supposed I have structures like this

typedef struct _student {
    int studentID;
    char name[30];
    char class[10];
    char department[10];
} Student;

and the following function creates new variables of type Student:

Student *new_student(int id, char *name, char *class, char *dept) {
    Student *s = (Student *)malloc(sizeof(Student *));

    s->studentID = id;
    strncpy(s->name, name, sizeof(s->name) - 1);
    s->name[sizeof(s->name) - 1] = '\0';
    strncpy(s->class, class, sizeof(s->class) - 1);
    s->class[sizeof(s->class) - 1] = '\0';
    strncpy(s->department, dept, sizeof(s->department) - 1);
    s->department[sizeof(s->department) - 1] = '\0';
    return s;
}

void display_student(Student *s) {
    printf("Student: %d | %s | %s | %s\n", s->studentID, s->name, s->class, s->department);
}

To test my code, I just write something simple in my main()

int main() {

    Student *s1 = new_student(20111201, "Lurther King Anders Something", "ICT-56", "SoICT");
    Student *s2 = new_student(20111202, "Harry Potter", "ICT-56", "SoICT");
    Student *s3 = new_student(20111203, "Hermione Granger", "ICT-56", "SoICT");
    Student *s4 = new_student(20111204, "Ron Weasley", "ICT-56", "SoICT");
    display_student(s1);
    display_student(s2);
    display_student(s3);
    display_student(s4);

    return 0;
}

However, the results is unexpected and weird to me:

Erro

Can someone please explain for me why the weird result is! I think I did things in a correct manner, I've applied safe use of strncpy, but I dont' understand the output.

alk · Accepted Answer · 2015-01-24T13:57:41.130

4

This

 ... malloc(sizeof(Student *));

allocates

sizeof(Student *)

bytes. Which typically is 4 or 8 as Student * is a pointer type.

You propably want

     ... malloc(sizeof(Student));

ov even better:

Student * s = malloc(sizeof(*s));

or even without the useless parenthesis:

Student * s = malloc(sizeof *s); /* sizeof is an operator, not a function. */

Read malloc(sizeof *s) as: "Allocate as much bytes as what s is pointing to."

edited Jan 24 '15 at 13:57

answered Jan 24 '15 at 13:52

alk

69,737
10
105
255

Oh, I see. thank you very much. I didn't see that simple thing. I will accept your answer right now :D – DucCuong Jan 24 '15 at 13:55

score 2 · Answer 2 · edited May 23 '17 at 12:07

2

Student *s = (Student *)malloc(sizeof(Student *));

That line is wrong. You allocate memory you want to use for a Student, but only ask for enough for a Student*.

You can make such an error much less likely by passing an expression instead of a type to sizeof.
Also, in C you don't cast on assigning from a void* to an other data-pointer-type:

Student *s = malloc(sizeof *s);

As a suggestion, consider using strlcpy, if neccessary defining it yourself.
Unless, of course, you rely on zeroing the rest of the buffer, like because you write them directly to a file.
strncpy is nearly always wrong, though you seem to have adroitely avoided all the pitfalls (with the possible exception of performance).

edited May 23 '17 at 12:07

Community

1
1

answered Jan 24 '15 at 13:55

Deduplicator

44,692
7
66
118

I've not heard about strlcpy (I'm starting out with C). Thank you for your suggestion, I'm gonna find more about that – DucCuong Jan 24 '15 at 14:06
`strlcpy()` is not part of the C Standard. @DucCuong It is an extension on some platforms. – alk Jan 24 '15 at 14:07

score -1 · Answer 3 · answered Jan 24 '15 at 14:33

Ok first of all: malloc(sizeof(Student*)) you got just size of pointers that is 4 bytes so you didn't get enough memory for your struct. I'm wondering how it is actually work, but whatever. So to get size of your struct use following example:

Student *s = (Student *)malloc(sizeof(Student));

Second you allocated new paice of data in heap, after you try to perform:

strncpy(s->name, name, sizeof(s->name) - 1);

Here your s->name has some garbage in the memory because you didn't assigned any data to this memory, you should use length of data that is in argument of function

 Student *new_student(int id, char *name, char *classSt, char *dept) 
{
    Student *s = (Student *)malloc(sizeof(Student));

    s->studentID = id;

    strncpy(s->name, name, strlen(name) + 1);
    strncpy(s->classSt, classSt, strlen(classSt) + 1);
    strncpy(s->department, dept, strlen(dept) + 1);

    return s;
}

"*you should use length of data*" This is very dangerous, as the source might be longer then the destination and with this the `strncpy()` would write behind the destination. Also `sizeof(s->name)` as per the OP's code will always be `30` independently wether `s->name` had been initilaised or not. -1 — alk, Jan 24 '15 at 14:45
NO, you built a buffer overflow trap. You give the size of the destination buffer, not the length of the input. — Patrick Schlüter, Jan 24 '15 at 14:45

strncpy only copies part of string

3 Answers3