C iterate over struct bad data

Question

I add values to my structure in while loop.

struct person
{
    char * name;
    char * surname;
    int  age;
    char * email;
};
struct person * tablicaOsob[100];

//for loop
tablicaOsob[i] = createPerson(name, surename, age, email);

Data has been added correctly. I check this via debugger

Now I would like iterate over tablicaOsob I create function

void list_persons(struct person *p, int k)
{
    printf("Lista osob\n");

    int i;
    for(i=0; i<k; i++)
    {
        printf("%s\n", p[i].name );
    }

}

But when I fire this function: list_persons(&tablicaOsob,i); I gets some bad data. Where is the problem. And my compilator says:

main.c:74:17: warning: passing argument 1 of 'list_persons' from incompatible pointer type [enabled by default]
                 list_persons(&tablicaOsob,i);
                 ^
main.c:17:6: note: expected 'struct person *' but argument is of type 'struct person * (*)[100]'
 void list_persons(struct person *p, int k)

EDIT:

createPerson()

struct person * createPerson(char * name, char * surename, int age, char * email)
{

    struct person * p = (struct person *) malloc(sizeof(struct person *));

    p->name = name;
    p->surname = surename;
    p->email = email;
    p->age = age;

    return p;

}

`list_persons(&tablicaOsob,i);` --> `list_persons(tablicaOsob,i);` — Sourav Ghosh, May 10 '16 at 08:23
Its not working :( I put breakpint in for in printf http://s32.postimg.org/6anjl56fp/brakpoint.jpg — lukassz, May 10 '16 at 08:26
Please how us `createPerson()`. I smell null-termination issue there... — Sourav Ghosh, May 10 '16 at 08:28
Ok, I put this function in my question. But why output from debugger of `tablicaOsob` is ok? — lukassz, May 10 '16 at 08:30

score 2 · Accepted Answer · edited May 23 '17 at 10:33

2

Yes, the issue in in memory allocation. You're allocating way less memory to p than it needs. After that, accessing the returned pointer cause out of bounds memory access which in turn invokes undefined behavior.

In your code,

  struct person * p = (struct person *) malloc(sizeof(struct person *));

you are allocating memory only for the size of a "pointer to object", not for the size of "object" itself. You should write

struct person * p = malloc(sizeof(struct person));

or, for better

struct person * p =  malloc(sizeof*p);

Also, while calling the function, you should pass a pointer to the structure, something like

 list_persons(tablicaOsob[0],i); //tablicaOsob[0]is of type struct person *

should do the job, as long as i value does not cause out of bound access.

That said, Please see this discussion on why not to cast the return value of malloc() and family in C..

edited May 23 '17 at 10:33

Community

1
1

answered May 10 '16 at 08:31

Sourav Ghosh

133,132
16
183
261

I change fire function to `list_persons(*tablicaOsob,i);` and It works. I don't change memory allocation. – lukassz May 10 '16 at 08:38
Ok, It's not ok. Function `list_persons` shows only last record. – lukassz May 10 '16 at 12:25
@lukassz What do you mean only last record? You know the difference of using `=` between an `int` and a `char` array, right? – Sourav Ghosh May 10 '16 at 12:28
When I add 2 record in my for loop into struct, and when I fire `list_persons` function they show me only last record whose I put into struct. – lukassz May 10 '16 at 12:30
@lukassz check the value if `i`, it maybe having a value that you don't expect it to have which can cause UB. – Sourav Ghosh May 10 '16 at 13:00
I check this, is correct. When I execute this for without function `list_presons` and `printf("%s", tablicaOsob[i]->name); it's ok. – lukassz May 10 '16 at 13:02

score 1 · Answer 2 · answered May 10 '16 at 08:52

1

Your createPerson function is totally wrong.

You are doing the initial malloc incorrectly, as mentioned by Alter Mann and Saurav.
You need to also allocate memory for the pointers name, suname and email. This is required for allocation of multiple persons over a loop. You need to allocate memory of strlen(xxx) +1 for each character array.

The function will look like this:

struct person * createPerson(char * name, char * surename, int age, char * email)
{
  struct person * p = malloc(sizeof(struct person));

  p->name = malloc(strlen(name)+1);
  strcpy(p->name, name);

  p->surname = malloc(strlen(surename)+1);
  strcpy(p->surname, surename);

  p->email = malloc(strlen(email)+1);
  strcpy(p->email, email);

  p->age = age;

  return p;

}

answered May 10 '16 at 08:52

Rishikesh Raje

8,556
2
16
31

Ok, thanks for correct. Why I must add `+1` to `strlen`? – lukassz May 10 '16 at 12:17
@lukassz For a C String, the last character is `\0`. You need to make space for this character in your memory. e.g. if string is "abc", you store a,b,c,\0 in memory. (4 locations) – Rishikesh Raje May 10 '16 at 12:40
Ok. But why for loop in `list_person` display only first record of structure? When I put two record. – lukassz May 10 '16 at 12:44
Call the function as `list_persons(*tablicaOsob,i)` – Rishikesh Raje May 10 '16 at 12:52
I call them. But *p in this function has only one element. – lukassz May 10 '16 at 13:00

David Ranieri · Answer 3 · 2016-05-10T08:52:20.550

0

struct person * p = (struct person *) malloc(sizeof(struct person *));

This is wrong, you want to reserve space for a struct, not for a pointer to this struct.

Change to

struct person *p = malloc(sizeof(struct person)); /* Don't cast malloc */

or

struct person *p = malloc(sizeof(*person));

This is also wrong:

list_persons(&tablicaOsob,i);

list_persons is expecting a pointer and tablicaOsob is an array of pointers, dont pass the address of an array of pointers to a single pointer, pass his value or simply pass the first element.

list_persons(*tablicaOsob,i);

edited May 10 '16 at 08:52

answered May 10 '16 at 08:32

David Ranieri

39,972
7
52
94

I don't known... I change function to `list_persons(*tablicaOsob,i);` and it's works fine. – lukassz May 10 '16 at 08:39
So I have one more question. Why is wrong in my memory allocation? I don't change this for yours and it works. – lukassz May 10 '16 at 08:42
And can you explain me the point of * in `list_persons(*tablicaOsob,i)`? – lukassz May 10 '16 at 08:44
Is wrong because `sizeof(struct person *)` returns the size of a pointer (4 or 8 bytes depending of the architecture) and you want the size of the `struct` itself. – David Ranieri May 10 '16 at 08:45

C iterate over struct bad data

3 Answers3