Garbage value even after initializing members of dynamically allocated struct array

Question

I have a dynamically allocated array of structures, 'buff'. Each element is a structure that has a few integer variables and a pointer 'buffer_ptr' which points to another dynamically allocated array of structures. The size of both arrays is given as command line input.

int buffer_size;
int user_num;
struct tuple
{
  char userID[5];
  char topic[16];
  int weight;
};
struct buff_ctrl    
{
  struct tuple* buffer_ptr;
  int in;
  int out;
  int numItems;
  int done;
};

The arrays are created and initialized in main() as follows:

int main(int argc, char* argv[])
{
  void *status;
  pthread_t mapThd;
  if(argc != 4)
  {
    printf("Input format: ./combiner <buffer_size> <number_of_users> <input_file>\n");
    return -1;
  }
  buffer_size = atoi(argv[1]);
  user_num = atoi(argv[2]);
  struct buff_ctrl *buff = (struct buff_ctrl*)malloc(user_num * sizeof(struct buff_ctrl)); 
  for(int i=0; i<user_num; i++)
  {
    struct buff_ctrl* curr_buff = (buff + (i*sizeof(struct buff_ctrl)));
    struct tuple *ptr = (struct tuple*)malloc(buffer_size * sizeof(struct tuple));
    curr_buff->buffer_ptr = ptr;//points to another array
    curr_buff->in = 8;
    curr_buff->out = 4;
    curr_buff->numItems = 7;
    curr_buff->done = 0;
    printf("%p\n",curr_buff);
  }

Then, I need to pass the 'buff' pointer as an argument when creating thread using pthread_create:

  pthread_create(&mapThd, NULL, mapper, (void*)buff);
  pthread_join(mapThd, &status);
  free(buff);
  /*end of main*/

My function pointer is as follows:

void* mapper(void *buff)
{
  struct buff_ctrl* arr = (struct buff_ctrl *)buff;
  struct buff_ctrl* temp_ptr;
  printf("######################################################\n");
  for(int k=0; k<user_num; k++)
  {
    /*Printing just to check values */
    temp_ptr = arr + (k*sizeof(struct buff_ctrl));
    printf("buffer ptr =  %p\n", temp_ptr->buffer_ptr);
    printf("in =  %d\n", temp_ptr->in);
    printf("out =  %d\n", temp_ptr->out);
    printf("numItems =  %d\n", temp_ptr->numItems);
  }
  printf("######################################################\n");
  pthread_exit((void*)buff);
}

But, when I print the values of 'buffer_ptr' from the created thread (only one), for ODD number of user_num, there is always ONE element of the array 'buff' which gives garbage value after pthread_create statement! When the values are checked in main itself after removing calls to pthread, it runs fine.

Good first question. – chux - Reinstate Monica Feb 09 '18 at 01:45 — chux - Reinstate Monica, Feb 09 '18 at 01:45

Pablo · Accepted Answer · 2018-02-09T01:42:36.843

4

This line

struct buff_ctrl* curr_buff = (buff + (i*sizeof(struct buff_ctrl)));

should be

struct buff_ctrl* curr_buff = buff + i;

buff + i is pointer arithmetic and the compiler already takes the size of the object pointed to by buff into consideration. By doing i*sizeof(struct buff_ctrl) you are assigning a pointer that may be after the allocated memory.

As general suggestion:

Don't cast malloc. And instead of using sizeof(<type>), use sizeof *variable, this is more safe, because it's easier to make mistakes when writing sizeof(<type>).

So:

struct buff_ctrl *buff = malloc(user_num * sizeof *buff);

...

struct tuple *ptr = malloc(buffer_size * sizeof *ptr);

And you don't need to declare a separate pointer, you can do:

for(int i=0; i<user_num; i++)
{
    buff[i].buffer_ptr = malloc(buffer_size * sizeof *buff[i].buffer_ptr);
    buff[i].in = 8;
    buff[i].out = 4;
    buff[i].numItems = 7;
    buff[i].done = 0;
}

Also you should always check for the return value of malloc. If it returns NULL, you cannot access that memory.

edited Feb 09 '18 at 01:42

answered Feb 09 '18 at 01:39

Pablo

13,271
4
39
59

Wow. That was only 8 seconds earlier than mine. – Iharob Al Asimi Feb 09 '18 at 01:40
@IharobAlAsimi for me was the other way round, when I pressed "Add answer" I saw yours. – Pablo Feb 09 '18 at 01:43
Yes, in fact ordering by "*oldest*" shows mine as the first answer to this question. I think that it was quite evident that this was causing problems, I immediately realized it. – Iharob Al Asimi Feb 09 '18 at 01:48
@IharobAlAsimi what caused problems? – Pablo Feb 09 '18 at 01:49
This `struct buff_ctrl *buff = malloc(user_num * sizeof *buff);` and other similar ones. – Iharob Al Asimi Feb 09 '18 at 01:51
@IharobAlAsimi now I'm confused. I though that both answers came almost immediately were causing problems. Or did you mean that I wrote something wrong regarding `malloc`? – Pablo Feb 09 '18 at 01:54
nothing. I mean that we both noticed the problem because it was too evident. Your answer is very nice, I upvoted it. – Iharob Al Asimi Feb 09 '18 at 01:55

score 3 · Answer 2 · answered Feb 09 '18 at 01:41

This is wrong:

struct buff_ctrl* curr_buff = (buff + (i*sizeof(struct buff_ctrl)));

When you do pointer arithmetic, it operates in units of the size of what the pointer points to, so you don't need to multiply by sizeof. As a result, you're effectively multiplying twice and accessing outside the array bounds.

Just treat buff as an array, rather than dealing with pointers.

  for(int i=0; i<user_num; i++)
  {
    struct tuple *ptr = malloc(buffer_size * sizeof(struct tuple));
    buff[i].buffer_ptr = ptr;//points to another array
    buff[i].in = 8;
    buff[i].out = 4;
    buff[i].numItems = 7;
    buff[i].done = 0;
  }

Also, see Do I cast the result of malloc?

score 1 · Answer 3 · edited Feb 09 '18 at 02:49

You have a fundamental error.

Pointer arithmetics works by adding the offset in multiples of the pointer type, so adding the offset yourself will not work as you apparently expect it to.

If it was a char * pointer then you would need to add the offset manually, increments would be multiplied by one. But in your case increments by n are multiplied by the size of the pointer base type.

There are times when doing pointer arithmetics with the addition notation makes sense, but most of the time it's much clearer to write index notation instead.

Garbage value even after initializing members of dynamically allocated struct array

3 Answers3