Program Crashing when freeing memory

[vashwath]

Hi all,
I am not able to figure out why the following program fails. Hope this
long program does not irritate you.
#include <stdlib.h>
#include <string.h>
#include <stdio.h>

#define K_TITLE_LEN 14
#define K_MAX_PLMS_ENTRIES 2000
#define K_SUMMARY_TITLE_LEN 18
typedef struct
{
int desc_code;
int meas_num;
char title[K_SUMMARY_TITLE_LEN];
}cond_summ_data_t;
typedef struct
{
int count;
cond_summ_data_t data[K_MAX_PLMS_ENTRIES];

}cond_summ_t;
void listGetCondSumTitle(/* IN */ const cond_summ_t *plms_table,
/* IN */ int num_meas,
/* IN */ int num_desc,
/* IN */ const int *meas_num,
/* IN */ int *desc_code,
/* OUT */char title[][K_TITLE_LEN])
{
int plms_ind = 0;
int meas_ind = 0;
int desc_ind = 0;
typedef char temp_title_t[K_TITLE_LEN];
temp_title_t **temp_title;

temp_title = malloc(num_desc * sizeof(temp_title_t *));
for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
temp_title[desc_ind] = malloc(num_meas * sizeof(temp_title));
}

for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
for (meas_ind = 0; meas_ind < num_meas; meas_ind++)
{
strcpy(temp_title[desc_ind][meas_ind]," ");
}
}

/*For each plot meas entry*/
for (plms_ind = 0; plms_ind < plms_table->count; plms_ind++)
{
/*Until no measurement number left in meas_num*/
for (meas_ind = 0; meas_ind < num_meas; meas_ind++)
{
/*Compare with the measurement number of plotmeas meas_no*/
if ( meas_num[meas_ind] ==
(plms_table->data[plms_ind]).meas_num )
{
/*If Matches*/
/*Compare with desc code in plotmeas*/
for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
/*Compare with desc code in plotmeas.*/
if (desc_code[desc_ind] == (plms_table

->data[plms_ind]).desc_code)
{
/*If matches get the title and store in
temp_title*/
strcpy(
temp_title[desc_ind][meas_ind],
(plms_table->data[plms_ind]).title);
break;
}
}

}
}
}
for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
for (meas_ind = 0; meas_ind < num_meas; meas_ind++)
{
printf("temp_title[%d][%d] = %7s ",desc_ind, meas_ind,
temp_title[desc_ind][meas_ind]);
}
printf("\n");
}

for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
free(temp_title[desc_ind]); /*Program crashes here*/
}
free(temp_title);
}

int main()
{
cond_summ_t plms_table;
int num_meas=4;
int num_desc=7;
int desc_code[7] = {2,3,4,5,7,8,10};
int meas_num[4]={1000,2000,3000,4000};
char title[30][K_TITLE_LEN];

plms_table.count = 30;

plms_table.data[0].desc_code=2;
plms_table.data[0].meas_num=100;
strcpy(plms_table.data[0].title,"Two");


plms_table.data[1].desc_code=3;
plms_table.data[1].meas_num=100;
strcpy(plms_table.data[1].title,"Two");


plms_table.data[2].desc_code=3;
plms_table.data[2].meas_num=1000;
strcpy(plms_table.data[2].title,"Three");


plms_table.data[3].desc_code=7;
plms_table.data[3].meas_num=1000;
strcpy(plms_table.data[3].title,"Seven");


plms_table.data[4].desc_code=3;
plms_table.data[4].meas_num=2000;
strcpy(plms_table.data[2].title,"Three");


plms_table.data[5].desc_code=7;
plms_table.data[5].meas_num=2000;
strcpy(plms_table.data[3].title,"2-Seven");

plms_table.data[6].meas_num=4000;
strcpy(plms_table.data[6].title,"Three");


plms_table.data[7].desc_code=10;
plms_table.data[7].meas_num=4000;
strcpy(plms_table.data[7].title,"Ten");

listGetCondSumTitle(&plms_table,
num_meas,
num_desc,
meas_num,
desc_code,
title);
return 0;
}


Thanks for help

 

[Yangqing, JIA]

Hi all,
I am not able to figure out why the following program fails. Hope this
long program does not irritate you.
#include <stdlib.h>
#include <string.h>
#include <stdio.h>

#define K_TITLE_LEN 14
#define K_MAX_PLMS_ENTRIES 2000
#define K_SUMMARY_TITLE_LEN 18
typedef struct
{
int desc_code;
int meas_num;
char title[K_SUMMARY_TITLE_LEN];
}cond_summ_data_t;
typedef struct
{
int count;
cond_summ_data_t data[K_MAX_PLMS_ENTRIES];

}cond_summ_t;
void listGetCondSumTitle(/* IN */ const cond_summ_t *plms_table,
/* IN */ int num_meas,
/* IN */ int num_desc,
/* IN */ const int *meas_num,
/* IN */ int *desc_code,
/* OUT */char title[][K_TITLE_LEN])
{
int plms_ind = 0;
int meas_ind = 0;
int desc_ind = 0;
typedef char temp_title_t[K_TITLE_LEN];
temp_title_t **temp_title;

temp_title = malloc(num_desc * sizeof(temp_title_t *));

I think here using explicit casting "(temp_title_t **)malloc(...)" is
a better way.

for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
temp_title[desc_ind] = malloc(num_meas * sizeof(temp_title));
}

Here you made a small mistake, inside the bracket of sizeof it should
be temp_title_t, and the same as above, explicit casting "(temp_title_t
*)malloc(...)" might be better.
I used VC.Net to test the file, as currently I do not have a gcc
environment. Excuse me for my English... It's not my native tongue.

 

[Richard Heathfield]

Yangqing, JIA said:

I think here using explicit casting "(temp_title_t **)malloc(...)" is
a better way.

Why? It's a useless cast. An implicit conversion is supplied. If you're
going to suggest an improvement, suggest:

temp_title = malloc(num_desc * sizeof *temp_title);

for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
temp_title[desc_ind] = malloc(num_meas * sizeof(temp_title));
}

Here you made a small mistake, inside the bracket of sizeof it should
be temp_title_t, and the same as above, explicit casting "(temp_title_t
*)malloc(...)" might be better.

Forget the useless cast. The best way to do this call is:

temp_title[desc_ind] = malloc(num_meas * sizeof *temp_title[desc_ind]);

 

[Niklas Norrthon]

I think here using explicit casting "(temp_title_t **)malloc(...)" is
a better way.

Why do you think that? My personal preferences is to avoid casts as
much as possible, and when not possible to avoid them I usually
try to find some other solution first.

Here a cast is totally meaningless, since the void* returned by
malloc is automagically converted to the correct pointer type.

Could you please elaborate on why you think the cast is a better
way?

The prefered way in comp.lang.c is:

ptr = malloc(count * sizeof *ptr);

for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
temp_title[desc_ind] = malloc(num_meas * sizeof(temp_title));
}

Here you made a small mistake, inside the bracket of sizeof it should
be temp_title_t, and the same as above, explicit casting "(temp_title_t
*)malloc(...)" might be better.

You are correct. The bug is that the amount of memory allocated is not
enough, and later in the code a strcpy writes out of bounds because of
this.

This would of course not have happened if the original poster had used
the style I proposed:

temp_title[desc_ind] = malloc(num_meas * sizeof *temp_title[desc_ind]);

/Niklas Norrthon

 

[Barry Schwarz]

Hi all,
I am not able to figure out why the following program fails. Hope this
long program does not irritate you.
#include <stdlib.h>
#include <string.h>
#include <stdio.h>

#define K_TITLE_LEN 14
#define K_MAX_PLMS_ENTRIES 2000
#define K_SUMMARY_TITLE_LEN 18
typedef struct
{
int desc_code;
int meas_num;
char title[K_SUMMARY_TITLE_LEN];
}cond_summ_data_t;
typedef struct
{
int count;
cond_summ_data_t data[K_MAX_PLMS_ENTRIES];

}cond_summ_t;
void listGetCondSumTitle(/* IN */ const cond_summ_t *plms_table,
/* IN */ int num_meas,
/* IN */ int num_desc,
/* IN */ const int *meas_num,
/* IN */ int *desc_code,
/* OUT */char title[][K_TITLE_LEN])
{
int plms_ind = 0;
int meas_ind = 0;
int desc_ind = 0;
typedef char temp_title_t[K_TITLE_LEN];
temp_title_t **temp_title;

temp_title = malloc(num_desc * sizeof(temp_title_t *));
for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
temp_title[desc_ind] = malloc(num_meas * sizeof(temp_title));

As others have pointed out, you are using the wrong sizeof. temp_title
is a pointer. Odds are its size is 4. So you have allocated
num_meas*4 bytes.

}

for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
for (meas_ind = 0; meas_ind < num_meas; meas_ind++)
{
strcpy(temp_title[desc_ind][meas_ind]," ");

Here you lie to the compiler. temp_title is a pointer to pointer to
array of 18 char. temp_title[...] is a pointer to such an array.
temp_title[...][...] is such an array. But you did not allocate space
for num_meas such arrays. You under-allocated by 75%. At some point,
strcpy will begin to overflow your allocated area. This is undefined
behavior. One common outcome of doing this is that you destroy the
data malloc uses to keep track of allocated memory.

}
}

/*For each plot meas entry*/
for (plms_ind = 0; plms_ind < plms_table->count; plms_ind++)
{
/*Until no measurement number left in meas_num*/
for (meas_ind = 0; meas_ind < num_meas; meas_ind++)
{
/*Compare with the measurement number of plotmeas meas_no*/
if ( meas_num[meas_ind] ==
(plms_table->data[plms_ind]).meas_num )
{
/*If Matches*/
/*Compare with desc code in plotmeas*/
for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
/*Compare with desc code in plotmeas.*/
if (desc_code[desc_ind] == (plms_table

->data[plms_ind]).desc_code)
{
/*If matches get the title and store in
temp_title*/
strcpy(
temp_title[desc_ind][meas_ind],
(plms_table->data[plms_ind]).title);
break;
}
}

}
}
}
for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
for (meas_ind = 0; meas_ind < num_meas; meas_ind++)
{
printf("temp_title[%d][%d] = %7s ",desc_ind, meas_ind,
temp_title[desc_ind][meas_ind]);
}
printf("\n");
}

for (desc_ind = 0; desc_ind < num_desc; desc_ind++)
{
free(temp_title[desc_ind]); /*Program crashes here*/

And since malloc is now completely confused, free gets confused to.

Be thankful, a program crash is one of the better manifestations of
undefined behavior.

}
free(temp_title);
}

snip code for main


<<Remove the del for email>>