Printable View
Boy, it's been a few days of crazy free() trips here for me...
Anyway, when I do this:
Both these free() calls cause a double-free/corruption error, which means I cannot free the memory allocated to either struct?Code:int main () {
struct in_addr *address=malloc(sizeof(struct in_addr));
struct hostent *test=malloc(sizeof(struct hostent));
if ((test==NULL) || (address==NULL)) exit (-1);
[...struct are both filled and used]
free(address);
free(&test); // alt version
Literally remove the portion that you omitted from your example. Do you still get the error?
Usually it's right on one or the other:
* Double free
* Corruption
It is likely that some of the code you have omitted is overwriting the end of your buffer.
--
Mats
Is incorrect, because you need to free the pointee, not the pointer itself.Code:free(&test); // alt version
So in other words freeing() the pointers corrupts vital memory that got overwritten into where the structs are allocated?Quote:
Originally Posted by matsp
I thought free() just affected the pointer and not the memory...but okay.
Nothing happens after the free() except a final return. I tried laserlight's recommendation and yep, it must be some other part of my code. Guess I better get on with installing valgrind or use a bigger font...
That's weird though. In view of Elysia's observation, which appears correct, I would expect the problem to remain. Perhaps it merely did not surface except in the presence of the code you omitted. Undefined behaviour is very problematic.Quote:
Originally Posted by MK27
No, it does not...Quote:
Originally Posted by MK27
That is correct...Quote:
I thought free() just affected the pointer and not the memory...but okay.
Usually, some corruption occurs in the application. Like the control structures that tells free how big that allocated block is, etc, has been corrupted, overwritten or something like that.
Or you could have freed that memory block once already, and so doing it twice causes an error (since the internal information is now gone).
These errors are usually easy to find with a good debugger.
For the second probability, set all pointers to NULL after freeing them to avoid double-free. Also use a good debugger that marks freed memory with a known pattern (Visual Studio does this), so you can see if you are double-freeing.
For the first probability, use a breakpoint to break if 1-4 bytes before and after your allocated memory gets written. That will find you the source of the corruption.
A good debugger will do all of this. Visual Studio certainly can.
malloc and free both uses a small amount of data that is before and/or after the data you actually get back - let's call it metadata. This is the "admin" side of malloc/free. If this metadata is overwritten by broken code, then free may well say "double free or corruption", because it validates the content before it allows the memory to be officially freed. So, for example, if you allocate a string with space for 10 characters, and write a 14 character string into it, you may well get this type of error.Quote:
Originally Posted by MK27
I think Elysia's point is a good'un tho'. You should probably not pass the address of the pointer, but the pointer itself. I should have spotted that.
--
Mats
Well, it's not been freed before so, it's not really a double free, so...
Just to make sure I have it straight, if this "admin" info (which is attached to the struct) is overwritten, then trying to free the struct could cause that error?Quote:
Originally Posted by matsp
Thanks all.
You don't know that.Quote:
Originally Posted by MK27
Yes, it would.Quote:
Just to make sure I have it straight, if this "admin" info (which is attached to the struct) is overwritten, then trying to free the struct could cause that error?
Thanks all.
Correct. But did you fix the extra & sign that shouldn't be there?Quote:
Originally Posted by MK27
Also, why are you allocating a hostent structure in the first place. As long as you only need ONE hostent, there's extremely little reason to allocate the memory in the first place. It is, after all only 4 pointers (16/32 bytes in a 32/64-bit machine) and two integers (8 bytes in nearly all machines).
And in_addr is even smaller - 4 bytes.
--
Mats
Who but me would have freed() it?Quote:
Originally Posted by Elysia
I wasn't really using that &, I was just covering a personal uncertainty ;)Quote:
Originally Posted by matsp
vis. NOT allocating the memory, what then: isn't there the danger of the same problem if those 24 bytes are written somewhere "unallocated"? in_addr is the same size as a pointer, but that doesn't mean those 4 bytes will be written into the pointer does it?
Or (this is a real question): if I'm using a pointer to structure like this but the logic of the program is such that the info from the struct gets used or copied out in the very next set of instructions, so it doesn't matter what happens to it, mean that I don't need to allocate memory for the return value of function before calling it because this unallocated returned data can't overwrite anything else because it was staticly allocated as part of the function, is now free, but still exists uncorrupted because there are no instructions writing anything to memory?
Using unallocated memory is Bad. Very Bad.Quote:
Originally Posted by MK27
But malloc is so far past overkill for this problem, you can't even see overkill from where you are. Why declare a pointer and keep malloc'ing space for it and freeing it over and over again, when you could just declare an actual struct in the first place?
I guess you make a fine point right here. Technically, it's because the function calls for a pointer, and I thought declaring a struct and casting it as a pointer and declaring a pointer then allocating it a struct were exactly the same thing, but I suppose there is some stack vs. heap difference or something...Quote:
Originally Posted by tabstop
On the other hand, I would never have noticed my evident memory corruption problem without this ;)
ps. you didn't exactly answer my last question, but of course you don't have to either
Well I have no idea exactly what function you refer to, since all I can find for the code is "...". All we know about that code is that (a) you call a function and (b) you overwrite some memory somewhere in it. And the bug would have seemed different, since instead of overwriting the part of memory that free() needed to do its job, you would have overwritten maybe another variable instead. (And you wouldn't cast the struct to a pointer -- you would just hand the function the address of the variable instead.)Quote:
Originally Posted by MK27
&What?!!!?Quote:
Originally Posted by tabstop
On the guess that that's a serious question, something like so:Quote:
Originally Posted by MK27
You weren't using &address with the code you had, were you? That wouldn't pass a pointer to a struct, that would pass a pointer to a pointer to a struct, which means that your function would cheerfully write to where your malloc information was (which means that when you tried to free it, bad things would happen ... wait a minute...).Code:int main() {
struct in_addr address;
struct hostent test;
SomeMythicalFunctionThatTakesAnAddress(&address);
SomeOtherFunction(&test);
}
Edit: I should be more clear, there, I think: you'll cheerfully write to where your pointer that came back from malloc was. So instead of freeing your pointer, you're going to try to free your IP address (or whatever the first four bytes of that structure are).
No. The actual main() looks like this:Quote:
Originally Posted by tabstop
I guess this is a bad habit I had started to acquire w/r/t not just using a static struct. I think it comes from using lots of gtk+ structs which really are just pointers to parts of other, already allocated material. I actually remember tacking the malloc calls in there at some point because of this. Thanks. I still haven't found the original corruption source tho.Code:int main (int argc, char *argv[]) {
char request[256], ascadd[4];
int sock, bytes;
struct in_addr *address=ec_malloc(sizeof(struct in_addr));
struct sockaddr_in connto;
struct hostent *test=ec_malloc(sizeof(struct hostent));
if (argc < 2) fatal(-1,"Grab what image?");
if ((sock=socket(PF_INET,SOCK_STREAM,0))==-1) perror("socket");
address=parseaddr(argv[1]);
sprintf(ascadd, "%s", inet_ntoa(*address));
printf("Connecting to %s\n",ascadd);
connto.sin_family=AF_INET;
connto.sin_port=htons(80);
connto.sin_addr.s_addr=address->s_addr;
memset(&(connto.sin_zero),0,8);
if (connect(sock,(struct sockaddr*)&connto,sizeof(struct sockaddr))==-1) {
perror("!connect");
fatal(-3,"...failed.");}
sprintf(request,"GET %s HTTP/1.0\r\n\r\n",imagepath);
printf("sending %s",request);
if (Transmit(sock,request)!=0) perror("!send");
bytes=getpage(sock);
printf("____PARTIAL HEADER____\n%s\n----------------------\n\nGrabbed Image Size: %d\n",reply,bytes);
free(reply);
if ((test=gethostbyaddr((const char*)&connto.sin_addr.s_addr,4,AF_INET))==NULL) printf("gethostbyaddr error: %d\n",h_
else printf("server hostname: %s\n",test->h_name);
return 0;
}
So that's what we mean -- assigning the result of parseaddr to address completely clobbers the pointer that came back from malloc (meaning those freshly malloc'ed bytes are gone forever), leaving address pointing to something that may or may not exist (if the address struct is local to the function, then NOT).
If the function parseaddr is to return a pointer, then parseaddr needs to malloc the memory (NOT main), and you can free that memory later.
enlightenment.Quote:
Originally Posted by tabstop
Okay, that's actually what I would usually do. Now here's a related question (I'm sure this is how I ended up here, because I just tried changing *test). gethostbyname() (and gethostbyaddr) requires a pointer to a struct hostent and you can't declare one and try (struct hostent*)info=gethostbyname or &info=gethostbyname. So does the pointer returned by gethostbyname() need to be freed? I think it can't so, I guess there's the answer...Quote:
Originally Posted by tabstop
They return a pointer to a struct, yes. According to the bottom of the man page:
So the memory is owned by gethostbyname -- but its declared static, so one copy with program lifetime storage, not malloc'ed memory. So no, it can't be freed. So you should be able to doCode:These functions use static data storage; if the data is needed for future
use, it should be copied before any subsequent calls overwrite it.
or whatever your parseaddr does, without any extra memory.Code:struct hostent *test;
test = gethostbyname(argv[1]);
(Now, if you do need to save it, you would want to copy it into a "real" struct, but it sounded like you don't.)
Okay, so as long I don't call gethostbyname() again, I can just pass around pointers to its stuff?
Here's a weird thing about in_addr: inet_aton() actually outputs into a struct in_addr*. If you do this:
It passes the compiler but the contents of netaddr end up bogus. I have to do this:Code:struct in_addr netaddr;
char *ascaddr;
inet_aton(ascaddr,&netaddr);
to get it to workCode:struct in_addr netaddr, *ptr=&netaddr;
char *ascaddr;
inet_aton(ascaddr,ptr);
ps. would malloc'ing "test" then etc. explain why freeing them would be a double free? Or should I still be looking for memory corruption?
You'll have to show more with that example; the following works as I would expect:Quote:
Originally Posted by MK27
And yes, the reassigning test then freeing would cause a corruption error at runtime.Code:#include <stdio.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <arpa/inet.h>
int main(void) {
struct in_addr netaddr;
char *ascaddr = "127.0.0.1";
inet_aton(ascaddr, &netaddr);
printf("%lx\n", netaddr.s_addr);
return 0;
}
Okay, how do you like this behaviour (maybe it's my code...)!
Witness *ptr is actually unused because you're right, &address works BUT ONLY IF I LEAVE *ptr=&address in. Otherwise I repeatedly get **2.0.0.0** regardless of the address.Code:struct in_addr parseaddr (char *addr) {
struct hostent *info;
struct in_addr address, *ptr=&address;
char *end;
if ((strncmp(addr,"http",4))==0) pretrunc(addr,7);
if ((end=strchr(addr,'/'))==NULL) fatal(-2,"Grab what image?");
end[0]=0;
imagepath=++end;
if (strlen(imagepath)<6) fatal(-22,"Grab what image?");
if ((imname=strrchr(imagepath,'/'))==NULL) imname=imagepath;
imname++;
printf("Grabbing %s from %s...\n",imname,addr);
info=gethostbyname(addr);
inet_aton(info->h_addr,&address);
printf("**%s**\n",inet_ntoa(address));
return address;
}
I just tried this all three ways like five or ten times and then went thru them again. And again. Definately the way I had it works, inet_aton(info->h_addr,ptr), then after I change it to inet_aton(info->h_addr,&address) it works, but after I remove the unused *ptr=&address it won't...could it be memory corruption again and the extra pointer is a space that takes the damage?
ps. no this is not quite the original "struct in_addr *parseaddr ()"
Alright, I don't know about your gethostbyname, but my gethostbyname doesn't actually return readable things in the h_addr field. Code:
and output:Code:#include <stdio.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <netdb.h>
int main(void) {
struct hostent *info;
struct in_addr address;
info = gethostbyname("www.cprogramming.com");
printf("Gethostbyname says name: %s\n", info->h_name);
printf("Gethostbyname says addr: %s\n", info->h_addr);
/* inet_aton(info->h_addr, &address);
printf("%lx\n", address.s_addr); */
address.s_addr = *((int *)info->h_addr);
printf("**%s**\n", inet_ntoa(address));
return 0;
}
Code:$ ./temp
Gethostbyname says name: cprogramming.com
Gethostbyname says addr: @"?X
**64.34.169.88**
It's no wonder you get errors.
You need to differentiate between pointer and pointee. The former is the actual variable that stores a memory address. It is not a of the type, so you can't store anything in it.
If you must store something in it, you must first ask for a of the type, and store its address in the pointer, so it points to a pointee (malloc).
However, seeing as a pointer is a variable that stores addresses, it is possible to assign another address to it. That will change what pointee it points to, it will not change the pointee itself.
http://cboard.cprogramming.com/showp...3&postcount=31
http://cboard.cprogramming.com/showp...65&postcount=4
http://apps.sourceforge.net/mediawik...er_on_pointers
http://cslibrary.stanford.edu/104/
Some resources about pointers.
And you must realize that:
...and...Code:struct in_addr netaddr;
char* ascaddr;
inet_aton(ascaddr, &netaddr);
...does the very same thing. You are just storing the address of the struct in a temporary pointer and passing it as opposed to directly.Code:struct in_addr netaddr;
struct in_addr* ptr = &netaddr;
char* ascaddr;
inet_aton(ascaddr, ptr);
And the first argument should probably be an address - ie a char array, and not an empty pointer. No function ever takes an empty pointer unless it takes an argument of type T**.
Yes. Did you actually read the question I was asking?Quote:
Originally Posted by Elysia
If you mean
Then yes, and it can only (probably) be explained by undefined behavior.Quote:
It passes the compiler but the contents of netaddr end up bogus. I have to do this:
to get it to work
Hopefully we're on the same page now then ;) altho I may not be completely out of the woods.Quote:
Originally Posted by Elysia
Anyway, I presume it is corruption, since this works:
I just can't see where I went wrong in the other code.Code:#include <stdio.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <netdb.h>
int main (int argc, char *argv[]) {
struct hostent *info=gethostbyname(argv[1]);
struct in_addr address;
inet_aton(info->h_addr,&address);
printf("%s is %s\n", info->h_name, inet_ntoa(address));
}
Unless I'm wrong about this bit. Again, the function (should) look like this (except for the red bit):
*addr points to argv[1]. pretrunc is just this:Code:struct in_addr parseaddr (char *addr) {
struct hostent *info;
struct in_addr address, *ptr=NULL; //unused
char *end;
if ((strncmp(addr,"http",4))==0) pretrunc(addr,7);
if ((end=strchr(addr,'/'))==NULL) fatal(-2,"Grab what image?");
end[0]=0;
imagepath=++end;
if (strlen(imagepath)<6) fatal(-22,"Grab what image?");
if ((imname=strrchr(imagepath,'/'))==NULL) imname=imagepath;
else imname++;
printf("Grabbing %s from %s...\n",imname,addr);
info=gethostbyname(addr);
inet_aton(info->h_addr,&address);
printf("**%s**\n",inet_ntoa(address));
return address;
}
"*end" is kind of extraneous, I could really just use the global "imagepath":Code:void pretrunc (char *string, int chs) {
int i;
for (i=0;i<strlen(string);i++) string[i]=string[i+chs];
string[i]='\0';
}
But guess what happens if I remove char *end? (Almost) the same thing as if I had removed *ptr=NULL, address is left overwritten. But all that happened up until now is this:Code:if ((imagepath=strchr(addr,'/'))==NULL) fatal(-2,"Grab what image?");
imagepath[0]=0;
imagepath++;
There's only two things I have any doubts about here: 1) I'm returning a struct wrong...not. 2) imagepath and imname are both global char* that I use to point into argv[1]. So they don't need any allocated space of their own, right?Code:int main (int argc, char *argv[]) {
char request[256], ascadd[4];
int sock, bytes;
struct in_addr address;
struct sockaddr_in connto;
struct hostent *test;
if (argc < 2) fatal(-1,"Grab what image?");
if ((sock=socket(PF_INET,SOCK_STREAM,0))==-1) perror("socket");
address=parseaddr(argv[1]);
What else could be causing memory corruption at this point, such that I need this extra 4-8 bytes to protect address? I guess I'm untarring valgrind real soon...
That's why I said use a debugger. They are made for purposes such as this.Quote:
Originally Posted by MK27
A good debugger can detect whenever something is written in "those extra bytes."
Or if you want, you can just step through the code with a debugger and watch those bytes.
No, they don't, since you are changing the pointer, not the pointee.Quote:
Originally Posted by MK27