how to release memory?

[mynews]

I use free() function to release memory,
But the memory usage is still increasing.

cygwin

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

struct queue{
char data[1024];
struct queue *next;
};
struct queue *myqueue[1024];
struct queue *pre_item[1024];
struct queue *first_item[1024];

void enqueue(int sockfd,char *var){
myqueue[sockfd]=(struct queue *) malloc(sizeof (struct queue));
strcpy((char *)&myqueue[sockfd]->data,var);
myqueue[sockfd]->next=NULL;
if (pre_item[sockfd]!=NULL){
pre_item[sockfd]->next=myqueue[sockfd];
}else{
first_item[sockfd]=myqueue[sockfd];
}
pre_item[sockfd]=myqueue[sockfd];
}

char *dequeue(int sockfd){
char r[1024];
struct queue *temp;
temp=first_item[sockfd];
strcpy(r,temp->data);
first_item[sockfd]=first_item[sockfd]->next;
temp=NULL;
free(temp);
return r;
}

 

[rahul]

I use free() function to release memory,
But the memory usage is still increasing.

temp=NULL;
free(temp);

Since when are you allowed to call free(NULL)?
May be what you want is :
free(temp);
temp = NULL;

I have not seen your code carefully but the approach seems to be
flawed. You are maintaining 3 array of pointers, each 1024 elements,
to implement a simple queue. Further, you are freeing only elements in
first_item (well, as of now, not even that). What about the elements
of per_item? Your logic is not clear to me. Perhaps you should re-
consider your data structures and algorithms.

 

[Keith Thompson]

Since when are you allowed to call free(NULL)?
May be what you want is :
free(temp);
temp = NULL;

[...]

You're certainly allowed to call free(NULL). It does nothing.

And that's (at least part of) the problem. The memory leak occurs
when he assigns ``temp=NULL;'', thereby losing the value of the
pointer that he wants to free.

There are numerous other problems, which I'll go over in a separate
followup.

 

[Keith Thompson]

I use free() function to release memory,
But the memory usage is still increasing.

cygwin

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

struct queue{
char data[1024];
struct queue *next;
};
struct queue *myqueue[1024];
struct queue *pre_item[1024];
struct queue *first_item[1024];

Three arrays of pointers seems excessive. I'm sure there's a simpler
way to do this, but I'll just deal with C issues here.

void enqueue(int sockfd,char *var){
myqueue[sockfd]=(struct queue *) malloc(sizeof (struct queue));

Don't cast the result of malloc(). The recommended idiom for this is:
myqueue[sockfd] = malloc(sizeof *myqueue[sockfd]);
More generally:
ptr = malloc(sizeof *ptr);
or
ptr = malloc(N * sizeof *ptr);
You can add parentheses if you like:
ptr = malloc(sizeof(*ptr));

strcpy((char *)&myqueue[sockfd]->data,var);

Most casts are unnecessary. This one, I suspect, was added to
suppress a warning. Adding a cast to supress a warning is like
disconnecting a warning light on your car's dashboard; the problem is
still there.

strcpy() expects a char* as its first argument.
``myqueue[sockfd]->data'' is of type char[1024], which decays to
char* in most contexts, so that's exactly what you want:

strcpy(myqueue[sockfd]->data, var);

What you've done is take the address of the array, which is of type
char(*)[1024], and then forcibly converted that to the expected type.

myqueue[sockfd]->next=NULL;
if (pre_item[sockfd]!=NULL){
pre_item[sockfd]->next=myqueue[sockfd];
}else{
first_item[sockfd]=myqueue[sockfd];
}
pre_item[sockfd]=myqueue[sockfd];
}

char *dequeue(int sockfd){
char r[1024];
struct queue *temp;
temp=first_item[sockfd];
strcpy(r,temp->data);
first_item[sockfd]=first_item[sockfd]->next;
temp=NULL;
free(temp);

You need to set temp to NULL *after* calling free().

For that matter, since temp is just about to go out of scope, there's
no need to set it to NULL.

return r;

r is a local variable of type char[1024]. It decays to a pointer to
its first element. Returning a pointer to a local variable is A Bad
Thing. You're giving the caller a pointer to something that will
no longer exist.

 

[Bartc]

You need to set temp to NULL *after* calling free().

For that matter, since temp is just about to go out of scope, there's
no need to set it to NULL.

It might be good practice to set temp to NULL. The code may grow in future
or it may be copied elsewhere.

Probably the compiler will eliminate it in this case.

 

[Chris Dollin]

It might be good practice to set temp to NULL. The code may grow in future
or it may be copied elsewhere.

Or it might not. Or reading the superfluous line of code might be
the last straw and send some poor programmer round the twist. Or
the extra bytes might overflow the disc. Or Cthulhu might turn up.

If the code /does/ grow or get copied, then appropriate action may
be taken.

 

[pereges]

I also have a few doubts with regard to releasing memory and other
resources. When an error occurs at some point in the program then
usually we proceed by printing an error message and propogating a flag
backwards ( return (1) ) indicating error. Is it a good policy to
release all the resources which were held up till then (when the error
occured) in the program. For eg. a file may be open which must be
closed, a data structure that must be freed etc. It can vary at
different points in the program, different resources may have to be
freed depending on what was allocated till that point. At the very end
(just before main process ends) all the resources can be freed.

 

[santosh]

I also have a few doubts with regard to releasing memory and other
resources. When an error occurs at some point in the program then
usually we proceed by printing an error message and propogating a flag
backwards ( return (1) ) indicating error. Is it a good policy to
release all the resources which were held up till then (when the error
occured) in the program. For eg. a file may be open which must be
closed, a data structure that must be freed etc. It can vary at
different points in the program, different resources may have to be
freed depending on what was allocated till that point. At the very end
(just before main process ends) all the resources can be freed.

You have the right idea. It's always good practise to release resources
held after a critical failure. Modern OSes may be able to recover
memory even if you do not free it before the program terminates, but
other resources like file streams may still need flushing, and this may
not happen automatically during abnormal termination.

In C this is generally done by propagating the critical failure up the
call stack, with each function cleaning up it's resources, all the way
up to main. Of course, this is tedious and may not be practical for
large programs. You might find that it often easier to just exit from
many important functions rather than go all the way back to main for
every error. If you need to invariably perform certain actions at exit,
then you can register the concerned functions with atexit. You should
generally reserve abort and _Exit for serious conditions like internal
inconsistency.

 

[Dan]

temp=NULL;
free(temp);

you need to pass temp to free before you null it....
either this is a silly mistake, or you need to go back and understand what a
pointer is

 

[Richard Bos]

It might be good practice to set temp to NULL. The code may grow in future
or it may be copied elsewhere.

It is not good practice to set pointers to null for no better reason
than that one has just free()d them. It will lead you to believe that
all pointers are either valid or null; and sooner or later, you will
encounter one which is not. It's one of those stopgap measures which
work on the idea that programmers don't think about their code, so they
should be given crutches. Better make sure you get it _right_, not
probably-not-all-too-dramatic-consequences-if-I-get-it-wrong.

What "mynews" needs is simply this, no more:

free(temp);

Richard

 

[Bartc]

It is not good practice to set pointers to null for no better reason
than that one has just free()d them. It will lead you to believe that
all pointers are either valid or null; and sooner or later, you will
encounter one which is not.

No. It just means that if accidentally a freed pointer is dereferenced, an
error is raised. Otherwise it might quietly work until it causes trouble
later on.

You would also argue then that:

int *p = NULL;

is not a good idea?

(There's little effort involving in nullifying freed pointers: #define
freez(p) {free(p); p=NULL;} then use freez() instead. Not that I do any of
this myself of course..)

 

[Richard]

It is not good practice to set pointers to null for no better reason
than that one has just free()d them. It will lead you to believe that
all pointers are either valid or null; and sooner or later, you will
encounter one which is not. It's one of those stopgap measures which
work on the idea that programmers don't think about their code, so they
should be given crutches. Better make sure you get it _right_, not
probably-not-all-too-dramatic-consequences-if-I-get-it-wrong.

What "mynews" needs is simply this, no more:

free(temp);

Richard

This is dubious advice. NULLing a pointer makes it a lot clearer, for
example, in a debugger or whatever "framework" that the pointer is not
pointing to usable data. Sure you MIGHT forget to NULL some, in the same
way you MIGHT program other bugs but it can do no harm to use this a
reasonable method. Personally I do not NULL all malloc pointers, but I
know people who do and it works for them.

 

[Ben Bacarisse]

No. It just means that if accidentally a freed pointer is dereferenced, an
error is raised. Otherwise it might quietly work until it causes trouble
later on.

You would also argue then that:

int *p = NULL;

is not a good idea?

That's a different matter. At this point we know there is no copy of
p because we just made it. The problem with free(p); p = NULL; is
that is does nothing for any copies that exist.

(There's little effort involving in nullifying freed pointers: #define
freez(p) {free(p); p=NULL;} then use freez() instead.

That is bad for so many reasons. I know you are just throwing it out
but (for others tempted by it):

(1) A macro should have a stand-out name -- especially if is look like
a function but does not behave like a function. This one modifies an
object and references p twice. FREEZ is better.

(2) For macro bodies that are to be statements (when followed by a
you need either the do {S1; S2; ...; Sn } while (0) idiom or you need
the macro to expand to an expression. In this case, the latter is
simpler:

#define FREEZ(p) (free(p), (p)=NULL)

(Sorry to labour the point, but 'if (cond) freez(p); else S;' is not
legal with your example.)

(3) It is better to just get into the habit of using parentheses like
(p) above. In this case, since only the assignment and comma
operators can cause problems, it is probably OK -- a comma expression
needs to parenthesised in the *calling* code and assignment operators
associate to the right anyway, but do you really want everyone to
reason that out?

Not that I do any of
this myself of course..)

Maybe you've decided it is not a useful thing to do?

 

[Kenny McCormack]

This is dubious advice. NULLing a pointer makes it a lot clearer, for
example, in a debugger or whatever "framework" that the pointer is not
pointing to usable data. Sure you MIGHT forget to NULL some, in the same
way you MIGHT program other bugs but it can do no harm to use this a
reasonable method. Personally I do not NULL all malloc pointers, but I
know people who do and it works for them.

Isn't it actually better to set it to some sentinel value, not NULL
(which is, in a sense, already "taken"). I belive the usual is to set
it to 0xDEADBEEF..

Then you can test for that in your debugger, etc.

 

[Richard]

Isn't it actually better to set it to some sentinel value, not NULL
(which is, in a sense, already "taken"). I belive the usual is to set
it to 0xDEADBEEF..

Then you can test for that in your debugger, etc.

Good practical advice. I agree...

 

[Chris Dollin]

"Bartc" <[email protected]> writes:

That's a different matter. At this point we know there is no copy of
p because we just made it. The problem with free(p); p = NULL; is
that is does nothing for any copies that exist.


That is bad for so many reasons. I know you are just throwing it out
but (for others tempted by it):

(1) A macro should have a stand-out name -- especially if is look like
a function but does not behave like a function. This one modifies an
object and references p twice. FREEZ is better.

(2) For macro bodies that are to be statements (when followed by a
you need either the do {S1; S2; ...; Sn } while (0) idiom or you need
the macro to expand to an expression. In this case, the latter is
simpler:

#define FREEZ(p) (free(p), (p)=NULL)

(Sorry to labour the point, but 'if (cond) freez(p); else S;' is not
legal with your example.)

(3) If `p` has a side-effect, it happens twice in invocations of
`freez(p)`. Hilarity follows.

[I do this less often than I had expected, and the one place I found,
I'm calling a pool-memory-free function, not `free`, anyway.]

 

[Ben Bacarisse]

Ben Bacarisse wrote:

(3) If `p` has a side-effect, it happens twice in invocations of
`freez(p)`. Hilarity follows.

Maybe I did not stress it enough but that is what I meant in (1) when
I said "does not behave like a function ... references p twice".

 

[Richard Tobin]

(1) A macro should have a stand-out name -- especially if is look like
a function but does not behave like a function. This one modifies an
object and references p twice. FREEZ is better.

[/QUOTE]

(3) If `p` has a side-effect, it happens twice in invocations of
`freez(p)`. Hilarity follows.

This is just (1) again. If it only evaluated the argument once,
like a function, there would be little reason to draw attention
to its macrocity by using capitals.

-- Richard

 

[Barry Schwarz]

No. It just means that if accidentally a freed pointer is dereferenced, an
error is raised. Otherwise it might quietly work until it causes trouble
later on.

Dereferencing a NULL pointer will invoke undefined behavior but there
is no guarantee that an error will be raised.


Remove del for email

 

[Raymond Martineau]

No. It just means that if accidentally a freed pointer is dereferenced, an
error is raised.

At least one platform requires you to dereference the equivalant of a
NULL pointer, since the BIOS stored an important table at that
location. If you accidently dereference said pointer and change its
content, you take out the system rather than raising an error.

When you are dealing with pointers, the rule is not to use an invalid
pointer for whatever reason. In the case above, setting the pointer
to NULL just changes how the trouble will occurr rather then stopping
it immediatly.