free() multiple allocation error in C

free() multiple allocation error in C
====================================

Hi!
I have written a program in C on PC with Windows 2000 in a Visual C
environment.
I have an error in freeing multiple allocation as follows:
1. I allocated an array of pointer.
2. I Read line by line from a text file.
3. I allocated memory for the read line.
4. I wrote the pointer from malloc() to the array of pointers.
5. The problem is in the free() function. When I tried to free in a
loop the
allocated lines, the Visual C throw me with an exception.
6. I wanted to know why?. I tried also to free in the opposite order I
have
allocated them, but this also doesn't work.
7. Does anyone has an idea?
8. Please send answer to a.zeevi@solidgroup.com .
9. Thanks in advance.
Ariel Ze'evi.

Parts of the code:

#include "stdafx.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define byte unsigned char
#define word unsigned short

typedef struct {
word FlashAddress, index;
} IndexAddress;

FILE *fp;
char **FileBuff;
IndexAddress *AddStruct;
word LinesNumber=0;


int main(int argc, char* argv[])
{
char FileName[]={"c:\\ariel\\flash196\\modbus.hex"};
char line[MAX_LINE];
long BufSize=0;
word i,LineLen=0;


// Open for read (will fail if file "FileName" does not exist)
if( (fp = fopen( FileName, "r" )) == NULL )
printf( "The file %s was not opened\n",FileName );
else
printf( "The file '%s was opened\n",FileName );

// Finds Number of lines in the Hex File
while (!feof(fp))
{
fgets(line,MAX_LINE,fp);
LinesNumber++;
}
printf("\nLinesNumber=%d\n",LinesNumber);

// Allocate an array of pointers to number of lines
if((FileBuff = (char **)malloc(LinesNumber * sizeof(char *)))==NULL)
{
printf("malloc error!\n");
return -1;
}

rewind(fp);

// Read line by line from file, allocate a memory to it and copy line
to allocation
for(i=0; i<LinesNumber; i++)
{
fgets(line,MAX_LINE,fp);
if((FileBuff[i] = (char *)malloc(strlen(line) *
sizeof(char)))==NULL)
{
printf("malloc error!\n");
return -1;
}
strcpy(FileBuff[i],line);
}


// Allocate an array of pointers to number of lines
AddStruct = (IndexAddress*)malloc(LinesNumber *
sizeof(IndexAddress));
if(AddStruct ==NULL)
{
printf("malloc error!\n");
return -1;
}

// Prepare for sorting
for(i=0; i<LinesNumber; i++)
{
AddStruct[i].FlashAddress=Address(i);
AddStruct[i].index=i;
}
PrintAddIndex();

QuickSort(AddStruct,0,LinesNumber);
PrintAddIndex();


for(i=0; i<LinesNumber; i++)
free(FileBuff[i]); <====== IN THIS POINT THE PROGRAM WAS
THROWN.
free(FileBuff);

/* Close fp */
if( fclose( fp ) )
printf( "The file %s was not closed\n",FileName );

return 0;
}

re: free() multiple allocation error in C

Check if u are trying to free the memory which is freed before..

re: free() multiple allocation error in C

[snips]

On Thu, 27 Apr 2006 03:15:30 -0700, a.zeevi@solidgroup.com wrote:
[color=blue]
> #define byte unsigned char
> #define word unsigned short[/color]

Some reason for not using typedefs?
[color=blue]
> FILE *fp;
> char **FileBuff;
> IndexAddress *AddStruct;
> word LinesNumber=0;
>
>
> int main(int argc, char* argv[])
> {
> char FileName[]={"c:\\ariel\\flash196\\modbus.hex"};[/color]

Ick. Lose the braces.
[color=blue]
> char line[MAX_LINE];
> long BufSize=0;
> word i,LineLen=0;
>
>
> // Open for read (will fail if file "FileName" does not exist)
> if( (fp = fopen( FileName, "r" )) == NULL )
> printf( "The file %s was not opened\n",FileName );
> else
> printf( "The file '%s was opened\n",FileName );
>[/color]

If it doesn't open, wouldn't it be more graceful to handle that
explicitly, say by exiting, or whatever?
[color=blue]
> // Finds Number of lines in the Hex File
> while (!feof(fp)) {
> fgets(line,MAX_LINE,fp);
> LinesNumber++;
> }[/color]

Ugly, as it gets the eof test bass-ackwards. Basically, feof is only
going to be true _after_ fgets fails, reading past the last line.
[color=blue]
> printf("\nLinesNumber=%d\n",LinesNumber);
>
> // Allocate an array of pointers to number of lines
> if((FileBuff =
> (char **)malloc(LinesNumber * sizeof(char *)))==NULL) {[/color]

Repeat after me: "We do *not* cast malloc." Repeat five or six hundred
times. If your compiler is complaining, then a) You're actually using
C++, b) You forget a header you need, or c) you're doing something wrong.
[color=blue]
> printf("malloc error!\n");
> return -1;[/color]

No such critter as -1. It's 0, EXIT_SUCCESS or EXIT_FAILURE.
[color=blue]
> // Read line by line from file, allocate a memory to it and copy line
> to allocation
> for(i=0; i<LinesNumber; i++)
> {
> fgets(line,MAX_LINE,fp);
> if((FileBuff[i] = (char *)malloc(strlen(line) *
> sizeof(char)))==NULL)[/color]

Why are you using sizeof(char)? If you had to multiply 7*3, would you
write it as (7*1) * (3*1)? No? So why are you doing completely pointless
multiplication by 1? Hint: sizeof(char) is 1. Period. Hence it's
completely pointless in virtually every case you'll ever see it used.

That said, you're allocating one too few characters. For example, if your
line in the file is ABC\n, your "line" variable will contain "ABC\n\0".
That's 5 chars - but strlen only returns 4; it doesn't include the \0 at
the end. So you're short one byte.
[color=blue]
> strcpy(FileBuff[i],line);[/color]

And, of course, this right here may well puke and die as you try to
merrily write past the end of the allocated region.
[color=blue]
> for(i=0; i<LinesNumber; i++)
> free(FileBuff[i]); <====== IN THIS POINT THE PROGRAM WAS
> THROWN.[/color]

At a guess, since none of your strings are actually strings - lacking
the \0 - I'd suspect you're trashing the data that malloc/free use to
track what's been allocated.

It's also entirely possible that your sort routine is mucking things up;
if it's doing string-based comparisons on non-strings, bad things will
happen there, too.

re: free() multiple allocation error in C

a.zeevi@solidgroup.com wrote:[color=blue]
> free() multiple allocation error in C
> ====================================
>
> Hi!
> I have written a program in C on PC with Windows 2000 in a Visual C
> environment.
> I have an error in freeing multiple allocation as follows:
> 1. I allocated an array of pointer.
> 2. I Read line by line from a text file.
> 3. I allocated memory for the read line.
> 4. I wrote the pointer from malloc() to the array of pointers.
> 5. The problem is in the free() function. When I tried to free in a
> loop the
> allocated lines, the Visual C throw me with an exception.
> 6. I wanted to know why?. I tried also to free in the opposite order I
> have
> allocated them, but this also doesn't work.
> 7. Does anyone has an idea?
> 8. Please send answer to a.zeevi@solidgroup.com .
> 9. Thanks in advance.
> Ariel Ze'evi.
>
> Parts of the code:
>
> #include "stdafx.h"
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> #define byte unsigned char
> #define word unsigned short
>
> typedef struct {
> word FlashAddress, index;
> } IndexAddress;
>
> FILE *fp;
> char **FileBuff;
> IndexAddress *AddStruct;
> word LinesNumber=0;
>
>
> int main(int argc, char* argv[])
> {
> char FileName[]={"c:\\ariel\\flash196\\modbus.hex"};
> char line[MAX_LINE];
> long BufSize=0;
> word i,LineLen=0;
>
>
> // Open for read (will fail if file "FileName" does not exist)
> if( (fp = fopen( FileName, "r" )) == NULL )
> printf( "The file %s was not opened\n",FileName );
> else
> printf( "The file '%s was opened\n",FileName );
>
> // Finds Number of lines in the Hex File
> while (!feof(fp))
> {
> fgets(line,MAX_LINE,fp);
> LinesNumber++;
> }
> printf("\nLinesNumber=%d\n",LinesNumber);
>
> // Allocate an array of pointers to number of lines
> if((FileBuff = (char **)malloc(LinesNumber * sizeof(char *)))==NULL)
> {
> printf("malloc error!\n");
> return -1;
> }
>
> rewind(fp);
>
> // Read line by line from file, allocate a memory to it and copy line
> to allocation
> for(i=0; i<LinesNumber; i++)
> {
> fgets(line,MAX_LINE,fp);
> if((FileBuff[i] = (char *)malloc(strlen(line) *
> sizeof(char)))==NULL)
> {
> printf("malloc error!\n");
> return -1;
> }
> strcpy(FileBuff[i],line);
> }
>
>
> // Allocate an array of pointers to number of lines
> AddStruct = (IndexAddress*)malloc(LinesNumber *
> sizeof(IndexAddress));
> if(AddStruct ==NULL)
> {
> printf("malloc error!\n");
> return -1;
> }
>
> // Prepare for sorting
> for(i=0; i<LinesNumber; i++)
> {
> AddStruct[i].FlashAddress=Address(i);
> AddStruct[i].index=i;
> }
> PrintAddIndex();
>
> QuickSort(AddStruct,0,LinesNumber);
> PrintAddIndex();
>
>
> for(i=0; i<LinesNumber; i++)
> free(FileBuff[i]); <====== IN THIS POINT THE PROGRAM WAS
> THROWN.
> free(FileBuff);
>
> /* Close fp */
> if( fclose( fp ) )
> printf( "The file %s was not closed\n",FileName );
>
> return 0;
> }
>[/color]
[color=blue]
>1. I allocated an array of pointer
> 5. The problem is in the free() function. When I tried to free in a
> loop the
> allocated lines, the Visual C throw me with an exception.[/color]

Passing the base address of the pointer array should
free the entire memory you allocated in step 1 (I assume you
allocated one big chunk). As somebody posted before, you might
be trying to free the same memory twice.

Rgds.
Amogh

re: free() multiple allocation error in C

Kelsey Bjarnason wrote:[color=blue]
> [snips]
>
> On Thu, 27 Apr 2006 03:15:30 -0700, a.zeevi@solidgroup.com wrote:
>
>[color=green]
>>#define byte unsigned char
>>#define word unsigned short[/color]
>
>
> Some reason for not using typedefs?
>
>[color=green]
>>FILE *fp;
>>char **FileBuff;
>>IndexAddress *AddStruct;
>>word LinesNumber=0;
>>
>>
>>int main(int argc, char* argv[])
>>{
>> char FileName[]={"c:\\ariel\\flash196\\modbus.hex"};[/color]
>
>
> Ick. Lose the braces.
>
>[color=green]
>> char line[MAX_LINE];
>> long BufSize=0;
>> word i,LineLen=0;
>>
>>
>> // Open for read (will fail if file "FileName" does not exist)
>> if( (fp = fopen( FileName, "r" )) == NULL )
>> printf( "The file %s was not opened\n",FileName );
>> else
>> printf( "The file '%s was opened\n",FileName );
>>[/color]
>
>
> If it doesn't open, wouldn't it be more graceful to handle that
> explicitly, say by exiting, or whatever?
>
>[color=green]
>> // Finds Number of lines in the Hex File
>> while (!feof(fp)) {
>> fgets(line,MAX_LINE,fp);
>> LinesNumber++;
>> }[/color]
>
>
> Ugly, as it gets the eof test bass-ackwards. Basically, feof is only
> going to be true _after_ fgets fails, reading past the last line.
>
>[color=green]
>> printf("\nLinesNumber=%d\n",LinesNumber);
>>
>> // Allocate an array of pointers to number of lines
>> if((FileBuff =
>> (char **)malloc(LinesNumber * sizeof(char *)))==NULL) {[/color]
>
>
> Repeat after me: "We do *not* cast malloc." Repeat five or six hundred
> times. If your compiler is complaining, then a) You're actually using
> C++, b) You forget a header you need, or c) you're doing something wrong.
>
>[color=green]
>> printf("malloc error!\n");
>> return -1;[/color]
>
>
> No such critter as -1. It's 0, EXIT_SUCCESS or EXIT_FAILURE.
>
>[color=green]
>> // Read line by line from file, allocate a memory to it and copy line
>>to allocation
>> for(i=0; i<LinesNumber; i++)
>> {
>> fgets(line,MAX_LINE,fp);
>> if((FileBuff[i] = (char *)malloc(strlen(line) *
>>sizeof(char)))==NULL)[/color]
>
>
> Why are you using sizeof(char)? If you had to multiply 7*3, would you
> write it as (7*1) * (3*1)? No? So why are you doing completely pointless
> multiplication by 1? Hint: sizeof(char) is 1. Period. Hence it's
> completely pointless in virtually every case you'll ever see it used.
>
> That said, you're allocating one too few characters. For example, if your
> line in the file is ABC\n, your "line" variable will contain "ABC\n\0".
> That's 5 chars - but strlen only returns 4; it doesn't include the \0 at
> the end. So you're short one byte.
>
>[color=green]
>> strcpy(FileBuff[i],line);[/color]
>
>
> And, of course, this right here may well puke and die as you try to
> merrily write past the end of the allocated region.
>
>[color=green]
>> for(i=0; i<LinesNumber; i++)
>> free(FileBuff[i]); <====== IN THIS POINT THE PROGRAM WAS
>>THROWN.[/color]
>
>
> At a guess, since none of your strings are actually strings - lacking
> the \0 - I'd suspect you're trashing the data that malloc/free use to
> track what's been allocated.
>
> It's also entirely possible that your sort routine is mucking things up;
> if it's doing string-based comparisons on non-strings, bad things will
> happen there, too.
>
>[/color]
[color=blue]
>Repeat after me: "We do *not* cast malloc." Repeat five or six hundred
>times. If your compiler is complaining, then a) You're actually using
>C++, b) You forget a header you need, or c) you're doing something >wrong.[/color]

Why shouldnt one cast malloc ? I use gcc 3.2, and it does crib if I dont
cast malloc. I might be missing your point somehow. Please enlighten.

Rgds.
Amogh

re: free() multiple allocation error in C

Amogh opined:
[color=blue]
> Kelsey Bjarnason wrote:[color=green]
>> [snips]
>>
> >Repeat after me: "We do *not* cast malloc." Repeat five or six
> >hundred
> >times. If your compiler is complaining, then a) You're actually
> >using C++, b) You forget a header you need, or c) you're doing
> >something >wrong.[/color]
>
> Why shouldnt one cast malloc ? I use gcc 3.2, and it does crib if I
> dont cast malloc.[/color]

That'd be because you forgot to include <stdlib.h>.
[color=blue]
> I might be missing your point somehow. Please enlighten.[/color]

The point is to crib if you forget to include <stdlib.h>
All other points made above apply as well.

--
"It's God. No, not Richard Stallman, or Linus Torvalds, but God."
(By Matt Welsh)

<http://clc-wiki.net/wiki/Introduction_to_comp.lang.c>

re: free() multiple allocation error in C

a.zeevi@solidgroup.com wrote:[color=blue]
> free() multiple allocation error in C
> ====================================
>
> Hi!
> I have written a program in C on PC with Windows 2000 in a Visual C
> environment.
> I have an error in freeing multiple allocation as follows:
> 1. I allocated an array of pointer.
> 2. I Read line by line from a text file.
> 3. I allocated memory for the read line.
> 4. I wrote the pointer from malloc() to the array of pointers.
> 5. The problem is in the free() function. When I tried to free in a
> loop the
> allocated lines, the Visual C throw me with an exception.
> 6. I wanted to know why?. I tried also to free in the opposite order I
> have
> allocated them, but this also doesn't work.
> 7. Does anyone has an idea?
> 8. Please send answer to a.zeevi@solidgroup.com .
> 9. Thanks in advance.
> Ariel Ze'evi.
>
> Parts of the code:
>
> #include "stdafx.h"
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> #define byte unsigned char
> #define word unsigned short
>
> typedef struct {
> word FlashAddress, index;
> } IndexAddress;
>
> FILE *fp;
> char **FileBuff;
> IndexAddress *AddStruct;
> word LinesNumber=0;
>
>
> int main(int argc, char* argv[])
> {
> char FileName[]={"c:\\ariel\\flash196\\modbus.hex"};
> char line[MAX_LINE];
> long BufSize=0;
> word i,LineLen=0;
>
>
> // Open for read (will fail if file "FileName" does not exist)
> if( (fp = fopen( FileName, "r" )) == NULL )
> printf( "The file %s was not opened\n",FileName );
> else
> printf( "The file '%s was opened\n",FileName );
>
> // Finds Number of lines in the Hex File
> while (!feof(fp))
> {
> fgets(line,MAX_LINE,fp);
> LinesNumber++;
> }
> printf("\nLinesNumber=%d\n",LinesNumber);
>
> // Allocate an array of pointers to number of lines
> if((FileBuff = (char **)malloc(LinesNumber * sizeof(char *)))==NULL)
> {
> printf("malloc error!\n");
> return -1;
> }
>
> rewind(fp);
>
> // Read line by line from file, allocate a memory to it and copy line
> to allocation
> for(i=0; i<LinesNumber; i++)
> {
> fgets(line,MAX_LINE,fp);
> if((FileBuff[i] = (char *)malloc(strlen(line) *
> sizeof(char)))==NULL)
> {
> printf("malloc error!\n");
> return -1;
> }
> strcpy(FileBuff[i],line);
> }
>
>
> // Allocate an array of pointers to number of lines
> AddStruct = (IndexAddress*)malloc(LinesNumber *
> sizeof(IndexAddress));
> if(AddStruct ==NULL)
> {
> printf("malloc error!\n");
> return -1;
> }
>
> // Prepare for sorting
> for(i=0; i<LinesNumber; i++)
> {
> AddStruct[i].FlashAddress=Address(i);
> AddStruct[i].index=i;
> }
> PrintAddIndex();
>
> QuickSort(AddStruct,0,LinesNumber);
> PrintAddIndex();
>
>
> for(i=0; i<LinesNumber; i++)
> free(FileBuff[i]); <====== IN THIS POINT THE PROGRAM WAS
> THROWN.
> free(FileBuff);
>
> /* Close fp */
> if( fclose( fp ) )
> printf( "The file %s was not closed\n",FileName );
>
> return 0;
> }
>[/color]
[color=blue]
>1. I allocated an array of pointer
> 5. The problem is in the free() function. When I tried to free in a
> loop the
> allocated lines, the Visual C throw me with an exception.[/color]

Passing the base address of the pointer array should
free the entire memory you allocated in step 1 (I assume you
allocated one big chunk). As somebody posted before, you might
be trying to free the same memory twice.

Rgds.
Amogh

re: free() multiple allocation error in C

Kelsey Bjarnason wrote:[color=blue]
> [snips]
>
> On Thu, 27 Apr 2006 03:15:30 -0700, a.zeevi@solidgroup.com wrote:
>
>[color=green]
>>#define byte unsigned char
>>#define word unsigned short[/color]
>
>
> Some reason for not using typedefs?
>
>[color=green]
>>FILE *fp;
>>char **FileBuff;
>>IndexAddress *AddStruct;
>>word LinesNumber=0;
>>
>>
>>int main(int argc, char* argv[])
>>{
>> char FileName[]={"c:\\ariel\\flash196\\modbus.hex"};[/color]
>
>
> Ick. Lose the braces.
>
>[color=green]
>> char line[MAX_LINE];
>> long BufSize=0;
>> word i,LineLen=0;
>>
>>
>> // Open for read (will fail if file "FileName" does not exist)
>> if( (fp = fopen( FileName, "r" )) == NULL )
>> printf( "The file %s was not opened\n",FileName );
>> else
>> printf( "The file '%s was opened\n",FileName );
>>[/color]
>
>
> If it doesn't open, wouldn't it be more graceful to handle that
> explicitly, say by exiting, or whatever?
>
>[color=green]
>> // Finds Number of lines in the Hex File
>> while (!feof(fp)) {
>> fgets(line,MAX_LINE,fp);
>> LinesNumber++;
>> }[/color]
>
>
> Ugly, as it gets the eof test bass-ackwards. Basically, feof is only
> going to be true _after_ fgets fails, reading past the last line.
>
>[color=green]
>> printf("\nLinesNumber=%d\n",LinesNumber);
>>
>> // Allocate an array of pointers to number of lines
>> if((FileBuff =
>> (char **)malloc(LinesNumber * sizeof(char *)))==NULL) {[/color]
>
>
> Repeat after me: "We do *not* cast malloc." Repeat five or six hundred
> times. If your compiler is complaining, then a) You're actually using
> C++, b) You forget a header you need, or c) you're doing something wrong.
>
>[color=green]
>> printf("malloc error!\n");
>> return -1;[/color]
>
>
> No such critter as -1. It's 0, EXIT_SUCCESS or EXIT_FAILURE.
>
>[color=green]
>> // Read line by line from file, allocate a memory to it and copy line
>>to allocation
>> for(i=0; i<LinesNumber; i++)
>> {
>> fgets(line,MAX_LINE,fp);
>> if((FileBuff[i] = (char *)malloc(strlen(line) *
>>sizeof(char)))==NULL)[/color]
>
>
> Why are you using sizeof(char)? If you had to multiply 7*3, would you
> write it as (7*1) * (3*1)? No? So why are you doing completely pointless
> multiplication by 1? Hint: sizeof(char) is 1. Period. Hence it's
> completely pointless in virtually every case you'll ever see it used.
>
> That said, you're allocating one too few characters. For example, if your
> line in the file is ABC\n, your "line" variable will contain "ABC\n\0".
> That's 5 chars - but strlen only returns 4; it doesn't include the \0 at
> the end. So you're short one byte.
>
>[color=green]
>> strcpy(FileBuff[i],line);[/color]
>
>
> And, of course, this right here may well puke and die as you try to
> merrily write past the end of the allocated region.
>
>[color=green]
>> for(i=0; i<LinesNumber; i++)
>> free(FileBuff[i]); <====== IN THIS POINT THE PROGRAM WAS
>>THROWN.[/color]
>
>
> At a guess, since none of your strings are actually strings - lacking
> the \0 - I'd suspect you're trashing the data that malloc/free use to
> track what's been allocated.
>
> It's also entirely possible that your sort routine is mucking things up;
> if it's doing string-based comparisons on non-strings, bad things will
> happen there, too.
>
>[/color]
[color=blue]
>Repeat after me: "We do *not* cast malloc." Repeat five or six hundred
>times. If your compiler is complaining, then a) You're actually using
>C++, b) You forget a header you need, or c) you're doing something >wrong.[/color]

Why shouldnt one cast malloc ? I use gcc 3.2, and it does crib if I dont
cast malloc. I might be missing your point somehow. Please enlighten.

Rgds.
Amogh

re: free() multiple allocation error in C

Amogh opined:
[color=blue]
> Kelsey Bjarnason wrote:[color=green]
>> [snips]
>>
> >Repeat after me: "We do *not* cast malloc." Repeat five or six
> >hundred
> >times. If your compiler is complaining, then a) You're actually
> >using C++, b) You forget a header you need, or c) you're doing
> >something >wrong.[/color]
>
> Why shouldnt one cast malloc ? I use gcc 3.2, and it does crib if I
> dont cast malloc.[/color]

That'd be because you forgot to include <stdlib.h>.
[color=blue]
> I might be missing your point somehow. Please enlighten.[/color]

The point is to crib if you forget to include <stdlib.h>
All other points made above apply as well.

--
"It's God. No, not Richard Stallman, or Linus Torvalds, but God."
(By Matt Welsh)

<http://clc-wiki.net/wiki/Introduction_to_comp.lang.c>

re: free() multiple allocation error in C

Vladimir Oka opined:
[color=blue]
> Amogh opined:
>[color=green]
>> Kelsey Bjarnason wrote:[color=darkred]
>>> [snips]
>>>
>> >Repeat after me: "We do *not* cast malloc." Repeat five or six
>> >hundred
>> >times. If your compiler is complaining, then a) You're actually
>> >using C++, b) You forget a header you need, or c) you're doing
>> >something >wrong.[/color]
>>
>> Why shouldnt one cast malloc ? I use gcc 3.2, and it does crib if I
>> dont cast malloc.[/color]
>
> That'd be because you forgot to include <stdlib.h>.
>[color=green]
>> I might be missing your point somehow. Please enlighten.[/color]
>
> The point is to crib if you forget to include <stdlib.h>
> All other points made above apply as well.[/color]

To expand a bit:

If you don't include <stdlib.h> the compiler does not see a prototype
for `malloc()` and is forced to assume it returns an `int`. If you
don't cast, it will then complain about assigning an `int` to a
pointer. If you, however, do cast, compiler will try to interpret the
pointer returned by `malloc()` as an `int` and then convert that `int`
back to your pointer type. Therein lies the monster of Undefined
Behaviour, and weird things may happen.

--
"... freedom ... is a worship word..."
"It is our worship word too."
-- Cloud William and Kirk, "The Omega Glory", stardate unknown

<http://clc-wiki.net/wiki/Introduction_to_comp.lang.c>

re: free() multiple allocation error in C

Vladimir Oka wrote:[color=blue]
> Vladimir Oka opined:
>[color=green]
>> Amogh opined:
>>[color=darkred]
>>> Kelsey Bjarnason wrote:
>>>> [snips]
>>>>
>>> >Repeat after me: "We do *not* cast malloc." Repeat five or six
>>> >hundred
>>> >times. If your compiler is complaining, then a) You're actually
>>> >using C++, b) You forget a header you need, or c) you're doing
>>> >something >wrong.
>>>
>>> Why shouldnt one cast malloc ? I use gcc 3.2, and it does crib if I
>>> dont cast malloc.[/color]
>> That'd be because you forgot to include <stdlib.h>.
>>[color=darkred]
>>> I might be missing your point somehow. Please enlighten.[/color]
>> The point is to crib if you forget to include <stdlib.h>
>> All other points made above apply as well.[/color]
>
> To expand a bit:
>
> If you don't include <stdlib.h> the compiler does not see a prototype
> for `malloc()` and is forced to assume it returns an `int`. If you
> don't cast, it will then complain about assigning an `int` to a
> pointer. If you, however, do cast, compiler will try to interpret the
> pointer returned by `malloc()` as an `int` and then convert that `int`
> back to your pointer type. Therein lies the monster of Undefined
> Behaviour, and weird things may happen.[/color]

And, to expand even further, we have had people posting here asking why
code did not work on their nice new systems where the reason was that
they had failed to include stdlib.h and had used a cast to shut up the
compiler! The new system was a 64 bit system with 64 bit pointers and 32
bit int. Obviously, converting a 32 bit int to a pointer ignored the
other 32 bits that the real pointer had! There are other ways it can
fail on real systems.
--
Flash Gordon, living in interesting times.
Web site - http://home.flash-gordon.me.uk/
comp.lang.c posting guidelines and intro:
http://clc-wiki.net/wiki/Intro_to_clc

re: free() multiple allocation error in C

Groovy hepcat a.zeevi@solidgroup.com was jivin' on 27 Apr 2006
03:15:30 -0700 in comp.lang.c.
free() multiple allocation error in C's a cool scene! Dig it!
[color=blue]
>I have written a program in C on PC with Windows 2000 in a Visual C
>environment.[/color]

You mean with Visual C in a Windoze 2000 environment. Windoze is the
environment. Visual C is what you're programming with.
[color=blue]
>I have an error in freeing multiple allocation as follows:
>1. I allocated an array of pointer.
>2. I Read line by line from a text file.
>3. I allocated memory for the read line.
>4. I wrote the pointer from malloc() to the array of pointers.
>5. The problem is in the free() function. When I tried to free in a
>loop the
> allocated lines, the Visual C throw me with an exception.
>6. I wanted to know why?. I tried also to free in the opposite order I
>have
> allocated them, but this also doesn't work.
>7. Does anyone has an idea?
>8. Please send answer to a.zeevi@solidgroup.com .[/color]

No. Post here, read here. That's the custom.
[color=blue]
>9. Thanks in advance.
>Ariel Ze'evi.
>
>Parts of the code:[/color]

Oh dear! Where to begin... The beginning looks like a likely place.
I realise some of what I'm about to say has already been said. You can
skip what you've already seen and just take the other comments, if you
like.
[color=blue]
>#include "stdafx.h"[/color]

Non-standard, non-portable header. Judging by the quotes, it's a
user defined header (or a third party header), but you have not shown
us what is in it.
[color=blue]
>#include <stdio.h>
>#include <stdlib.h>
>#include <string.h>
>
>#define byte unsigned char
>#define word unsigned short[/color]

What's the point of that? Pascal programmer, are we? This does
nothing for the legibility of your code. It's just pointless clutter.
You ought to start getting used to using C's own language, and not try
to change it into some grotesque half-arsed imitation of another
language.
[color=blue]
>typedef struct {
> word FlashAddress, index;
>} IndexAddress;
>
>FILE *fp;
>char **FileBuff;
>IndexAddress *AddStruct;
>word LinesNumber=0;[/color]

None of these objects belong here. So-called "global variables"
should be avoided wherever practical. They can lead to bad things. Put
all of these object definitions inside main().
[color=blue]
>int main(int argc, char* argv[])[/color]

You don't seem to be using main()'s parameters, so leave them out.
There are two legal formats for main(). They are the one you have here
and this one:

int main(void)[color=blue]
>{
> char FileName[]={"c:\\ariel\\flash196\\modbus.hex"};
> char line[MAX_LINE];[/color]

MAX_LINE does not exist. Perchance, is it defined in the header you
haven't shown us?
[color=blue]
> long BufSize=0;
> word i,LineLen=0;
>
> // Open for read (will fail if file "FileName" does not exist)[/color]

Useless comments like this one should be avoided. Comments should
make code clearer, explain some construct that is not obvious or easy
to understand, describe the purpose of a small group of statements,
etc. Comments that don't add to understanding just get in the way.
They add to clutter, and are best left out.
Also, C++ style comments (those beginning with //) are frowned upon
in here. There are two main reasons. One reason is that they are not
legal in C90, the standard to which most currently available compilers
conform). Also, long comments tend to wrap when posted here. When C++
style comments wrap, it makes it more difficult to copy, paste &
compile your code.
[color=blue]
> if( (fp = fopen( FileName, "r" )) == NULL )
> printf( "The file %s was not opened\n",FileName );[/color]

If the file fails to open, you output an error message. How
thoughtful! But you still go on trying to read (below) from the file.
This results in undefined behaviour. Extremely bad! Quit or otherwise
handle the error so that you never attempt to read from an unopened
file. (But by all means do still output an error message.)
By the way, diagnostic output usually goes to stderr. That's what
it's there for, after all.
[color=blue]
> else
> printf( "The file '%s was opened\n",FileName );
>
> // Finds Number of lines in the Hex File[/color]

This is a slightly better comment than the one above. At least this
one explains the intent of the next few lines of code. Good!
[color=blue]
> while (!feof(fp))[/color]

The FAQ list of this newsgroup explains why this is wrong. Go now -
yes, right now - and read the FAQ (at this location:
http://www.eskimo.com/~scs/C-faq/top.html). Please don't come back
until you have read it.
[color=blue]
> {
> fgets(line,MAX_LINE,fp);
> LinesNumber++;
> }
> printf("\nLinesNumber=%d\n",LinesNumber);
>
> // Allocate an array of pointers to number of lines
> if((FileBuff = (char **)malloc(LinesNumber * sizeof(char *)))==NULL)[/color]

Don't cast the return value of malloc(). It is never required, if
you have properly declared malloc() by including stdlib.h, and can
hide a bug if you haven't. See questions 7.6 and 7.7 in the FAQ for
clues to the proper way to use malloc().
[color=blue]
> {
> printf("malloc error!\n");
> return -1;[/color]

Non-portable, non-standard return value. You can only portably
return 0, EXIT_SUCCESS or EXIT_FAILURE from main(), the latter two
being macros defined in stdlib.h.
[color=blue]
> }
>
> rewind(fp);[/color]

It would be a good idea to make sure this call succeeded. Check the
return value.
[color=blue]
> // Read line by line from file, allocate a memory to it and copy line
>to allocation
> for(i=0; i<LinesNumber; i++)
> {
> fgets(line,MAX_LINE,fp);
> if((FileBuff[i] = (char *)malloc(strlen(line) *
>sizeof(char)))==NULL)[/color]

See above. Also see question 7.8 of the FAQ.
And here we see a possible cause of your problem. You are not
allocating enough memory here. Remember, a string needs a terminating
'\0'. You must make room for it. strlen() returns the number of
characters in the string *not* including the '\0'. So you need to
allocate strlen(line) + 1 bytes here.
[color=blue]
> {
> printf("malloc error!\n");
> return -1;[/color]

Don't mix tabs and spaces in your code. We don't all set tabstops
the same as you. And so what looks right in your editor looks out of
whack to many of us. Be consistent with your indentation. Either only
use tabs or only use spaces, and use the same number of tabs and
spaces for each successive level of indentation. Many people here,
myself included, prefer spaces to tabs.
[color=blue]
> }
> strcpy(FileBuff[i],line);
> }
>
>
> // Allocate an array of pointers to number of lines
> AddStruct = (IndexAddress*)malloc(LinesNumber *
>sizeof(IndexAddress));
> if(AddStruct ==NULL)
> {
> printf("malloc error!\n");
> return -1;
> }
>
> // Prepare for sorting
> for(i=0; i<LinesNumber; i++)
> {
> AddStruct[i].FlashAddress=Address(i);
> AddStruct[i].index=i;
> }
> PrintAddIndex();[/color]

No such function or macro.
[color=blue]
> QuickSort(AddStruct,0,LinesNumber);[/color]

No such function or macro.
[color=blue]
> PrintAddIndex();
>
>
> for(i=0; i<LinesNumber; i++)
> free(FileBuff[i]); <====== IN THIS POINT THE PROGRAM WAS
>THROWN.[/color]

Don't put non-C text in C code. It makes it a pain to copy, paste &
compile.
[color=blue]
> free(FileBuff);
>
> /* Close fp */
> if( fclose( fp ) )
> printf( "The file %s was not closed\n",FileName );
>
> return 0;
>}[/color]

All in all that was quite ugly code. Ugly code is illegible code.
Illegible code is incomprehensible code. Incomprehensible code is
unmaintainable code. This is something you need to work on. And you
need to leave Pascalisms (such as byte and word) in Pascal Land.

--

Dig the even newer still, yet more improved, sig!

http://alphalink.com.au/~phaywood/
"Ain't I'm a dog?" - Ronny Self, Ain't I'm a Dog, written by G. Sherry & W. Walker.
I know it's not "technically correct" English; but since when was rock & roll "technically correct"?

re: free() multiple allocation error in C

Peter "Shaggy" Haywood wrote:[color=blue]
> Groovy hepcat a.zeevi@solidgroup.com was jivin' on 27 Apr 2006
> 03:15:30 -0700 in comp.lang.c.
> free() multiple allocation error in C's a cool scene! Dig it!
>[color=green]
> >I have written a program in C on PC with Windows 2000 in a Visual C
> >environment.[/color]
>
> You mean with Visual C in a Windoze 2000 environment. Windoze is the
> environment. Visual C is what you're programming with.
>[color=green]
> >I have an error in freeing multiple allocation as follows:
> >1. I allocated an array of pointer.
> >2. I Read line by line from a text file.
> >3. I allocated memory for the read line.
> >4. I wrote the pointer from malloc() to the array of pointers.
> >5. The problem is in the free() function. When I tried to free in a
> >loop the
> > allocated lines, the Visual C throw me with an exception.
> >6. I wanted to know why?. I tried also to free in the opposite order I
> >have
> > allocated them, but this also doesn't work.
> >7. Does anyone has an idea?
> >8. Please send answer to a.zeevi@solidgroup.com .[/color]
>
> No. Post here, read here. That's the custom.
>[color=green]
> >9. Thanks in advance.
> >Ariel Ze'evi.
> >
> >Parts of the code:[/color]
>
> Oh dear! Where to begin... The beginning looks like a likely place.
> I realise some of what I'm about to say has already been said. You can
> skip what you've already seen and just take the other comments, if you
> like.
>[color=green]
> >#include "stdafx.h"[/color]
>
> Non-standard, non-portable header. Judging by the quotes, it's a
> user defined header (or a third party header), but you have not shown
> us what is in it.
>[color=green]
> >#include <stdio.h>
> >#include <stdlib.h>
> >#include <string.h>
> >
> >#define byte unsigned char
> >#define word unsigned short[/color]
>
> What's the point of that? Pascal programmer, are we? This does
> nothing for the legibility of your code. It's just pointless clutter.
> You ought to start getting used to using C's own language, and not try
> to change it into some grotesque half-arsed imitation of another
> language.
>[color=green]
> >typedef struct {
> > word FlashAddress, index;
> >} IndexAddress;
> >
> >FILE *fp;
> >char **FileBuff;
> >IndexAddress *AddStruct;
> >word LinesNumber=0;[/color]
>
> None of these objects belong here. So-called "global variables"
> should be avoided wherever practical. They can lead to bad things. Put
> all of these object definitions inside main().
>[color=green]
> >int main(int argc, char* argv[])[/color]
>
> You don't seem to be using main()'s parameters, so leave them out.
> There are two legal formats for main(). They are the one you have here
> and this one:
>
> int main(void)[color=green]
> >{
> > char FileName[]={"c:\\ariel\\flash196\\modbus.hex"};
> > char line[MAX_LINE];[/color]
>
> MAX_LINE does not exist. Perchance, is it defined in the header you
> haven't shown us?
>[color=green]
> > long BufSize=0;
> > word i,LineLen=0;
> >
> > // Open for read (will fail if file "FileName" does not exist)[/color]
>
> Useless comments like this one should be avoided. Comments should
> make code clearer, explain some construct that is not obvious or easy
> to understand, describe the purpose of a small group of statements,
> etc. Comments that don't add to understanding just get in the way.
> They add to clutter, and are best left out.
> Also, C++ style comments (those beginning with //) are frowned upon
> in here. There are two main reasons. One reason is that they are not
> legal in C90, the standard to which most currently available compilers
> conform). Also, long comments tend to wrap when posted here. When C++
> style comments wrap, it makes it more difficult to copy, paste &
> compile your code.
>[color=green]
> > if( (fp = fopen( FileName, "r" )) == NULL )
> > printf( "The file %s was not opened\n",FileName );[/color]
>
> If the file fails to open, you output an error message. How
> thoughtful! But you still go on trying to read (below) from the file.
> This results in undefined behaviour. Extremely bad! Quit or otherwise
> handle the error so that you never attempt to read from an unopened
> file. (But by all means do still output an error message.)
> By the way, diagnostic output usually goes to stderr. That's what
> it's there for, after all.
>[color=green]
> > else
> > printf( "The file '%s was opened\n",FileName );
> >
> > // Finds Number of lines in the Hex File[/color]
>
> This is a slightly better comment than the one above. At least this
> one explains the intent of the next few lines of code. Good!
>[color=green]
> > while (!feof(fp))[/color]
>
> The FAQ list of this newsgroup explains why this is wrong. Go now -
> yes, right now - and read the FAQ (at this location:
> http://www.eskimo.com/~scs/C-faq/top.html). Please don't come back
> until you have read it.
>[color=green]
> > {
> > fgets(line,MAX_LINE,fp);
> > LinesNumber++;
> > }
> > printf("\nLinesNumber=%d\n",LinesNumber);
> >
> > // Allocate an array of pointers to number of lines
> > if((FileBuff = (char **)malloc(LinesNumber * sizeof(char *)))==NULL)[/color]
>
> Don't cast the return value of malloc(). It is never required, if
> you have properly declared malloc() by including stdlib.h, and can
> hide a bug if you haven't. See questions 7.6 and 7.7 in the FAQ for
> clues to the proper way to use malloc().
>[color=green]
> > {
> > printf("malloc error!\n");
> > return -1;[/color]
>
> Non-portable, non-standard return value. You can only portably
> return 0, EXIT_SUCCESS or EXIT_FAILURE from main(), the latter two
> being macros defined in stdlib.h.
>[color=green]
> > }
> >
> > rewind(fp);[/color]
>
> It would be a good idea to make sure this call succeeded. Check the
> return value.
>[color=green]
> > // Read line by line from file, allocate a memory to it and copy line
> >to allocation
> > for(i=0; i<LinesNumber; i++)
> > {
> > fgets(line,MAX_LINE,fp);
> > if((FileBuff[i] = (char *)malloc(strlen(line) *
> >sizeof(char)))==NULL)[/color]
>
> See above. Also see question 7.8 of the FAQ.
> And here we see a possible cause of your problem. You are not
> allocating enough memory here. Remember, a string needs a terminating
> '\0'. You must make room for it. strlen() returns the number of
> characters in the string *not* including the '\0'. So you need to
> allocate strlen(line) + 1 bytes here.
>[color=green]
> > {
> > printf("malloc error!\n");
> > return -1;[/color]
>
> Don't mix tabs and spaces in your code. We don't all set tabstops
> the same as you. And so what looks right in your editor looks out of
> whack to many of us. Be consistent with your indentation. Either only
> use tabs or only use spaces, and use the same number of tabs and
> spaces for each successive level of indentation. Many people here,
> myself included, prefer spaces to tabs.
>[color=green]
> > }
> > strcpy(FileBuff[i],line);
> > }
> >
> >
> > // Allocate an array of pointers to number of lines
> > AddStruct = (IndexAddress*)malloc(LinesNumber *
> >sizeof(IndexAddress));
> > if(AddStruct ==NULL)
> > {
> > printf("malloc error!\n");
> > return -1;
> > }
> >
> > // Prepare for sorting
> > for(i=0; i<LinesNumber; i++)
> > {
> > AddStruct[i].FlashAddress=Address(i);
> > AddStruct[i].index=i;
> > }
> > PrintAddIndex();[/color]
>
> No such function or macro.
>[color=green]
> > QuickSort(AddStruct,0,LinesNumber);[/color]
>
> No such function or macro.
>[color=green]
> > PrintAddIndex();
> >
> >
> > for(i=0; i<LinesNumber; i++)
> > free(FileBuff[i]); <====== IN THIS POINT THE PROGRAM WAS
> >THROWN.[/color]
>
> Don't put non-C text in C code. It makes it a pain to copy, paste &
> compile.
>[color=green]
> > free(FileBuff);
> >
> > /* Close fp */
> > if( fclose( fp ) )
> > printf( "The file %s was not closed\n",FileName );
> >
> > return 0;
> >}[/color]
>
> All in all that was quite ugly code. Ugly code is illegible code.
> Illegible code is incomprehensible code. Incomprehensible code is
> unmaintainable code. This is something you need to work on. And you
> need to leave Pascalisms (such as byte and word) in Pascal Land.
>
> --
>
> Dig the even newer still, yet more improved, sig!
>
> http://alphalink.com.au/~phaywood/
> "Ain't I'm a dog?" - Ronny Self, Ain't I'm a Dog, written by G. Sherry & W. Walker.
> I know it's not "technically correct" English; but since when was rock & roll "technically correct"?[/color]

re: free() multiple allocation error in C

Thank you for your time and clever remarks. ariel.