Is this C code using pointers valid?

Started by
11 comments, last by FRex 9 years ago


I'm just interested in knowing if the pointer usage is correct rather than actual logic or error checking.

i suspect there are logic flaws, hidden by the use of pointer vs array notation, which makes it hard to even spot the probable errors in the pointer notation code.

try rewriting it with array notation, and make it actually do something (even dummy calls to get server info). if it still doesn't work, then post that code.

Norm Barrows

Rockland Software Productions

"Building PC games since 1989"

rocklandsoftware.net

PLAY CAVEMAN NOW!

http://rocklandsoftware.net/beta.php

Advertisement

I did try using array notation but couldn't get it to work, so guess I need to read up about that a bit more unless someone is kind enough to show a quick example.

NOTE: this is off the top of my head... be sure to double check the syntax!

basically:

 
void main()
{
// -------------- constants -----------------
#define LISTSIZE 3   // in your code your list is hard coded to size 3.
 
//----------- variables ----------------------
serverInfo *list // pointer to your list array
int i;   // loop counter

// ----------- code ---------------------------
list=(serverInfo *)malloc(sizeof(serverInfo)*LISTSIZE);   // allocate your list
// ---------------------  populate list     
for (i=0; i<LISTSIZE; i++)     
    {    
    strcpy(list[i].serverIP, "10.10.100.10");     
    // etc...    
    // IE: list[i].whatever=some_value;    
    }
// ------------------------- print list     
for (i=0; i<LISTSIZE; i++)     
   {    
   printf("%s\n",list[i].serverIP);     
   // and so on....    
   }
// ----------------------- deallocate list
free(list);    // free the RAM
}  
 
 

Norm Barrows

Rockland Software Productions

"Building PC games since 1989"

rocklandsoftware.net

PLAY CAVEMAN NOW!

http://rocklandsoftware.net/beta.php

This code may read freed memory. - I don't access the pointer again after the call to free

This code may double free. - Not sure how\where I'm doing this?

This is both because if realloc fails to resize the memory buffer given, it will allocate new one, copy content to it and free the old one. This is actually why memory leaks happens too.

It's basically this scenario:

1. You alloc a 1 element array and save pointer to it.

2. You pass it to your function and call realloc many times.

3. Your function returns, but you still have pointer to original array from point 1, because of the mistake I mentioned in my first post.

//If realloc always resized in place, it's OK - the memory block never moved, the original pointer still points to it (which is why your memory tool didn't catch an error and code is working)

//But - assuming that realloc at least once didn't resize but did a alloc-copy-free cycle you have following three bugs:

4. You use this pointer from point 1 - this is invalid reads, this memory was long freed by the realloc.

5. You free this pointer from point 1 - this is double free, as above - memory was already freed by realloc.

6. Program ends, but you never freed the NEW buffer that realloc allocated - this is a memory leak. (You never even had a pointer to it anywhere in main actually).

I tagged it as C as I didn't want people suggesting the use of new/delete or vector class etc. I'm using Visual C++ compiler so I confess it may not be *pure C* but for the purposes of this post it served its purpose.

Does Visual C++ really compile this code when the file has .c extension though? I'm surprised it does, last one I used was 2012 2 years ago and I though I remembered it as being more strict than that.

Your main could be rewritten like so (assuming getServerListTest is fixed):


serverInfo *serverList = (serverInfo*)calloc(1, sizeof(serverInfo));
int serverCount = getServerListTest(&serverList);
	
for (int i = 0; i < serverCount; i++)
{
	printf("ip:%s name:%s\n", serverList[i].serverIP, serverList[i].serverName);
}
	
free(serverList);

It's much more clearer this way already.

To fix the function itself, it's probably best to allocate an array in it once and write its' pointer to the passed argument and then fill it with data.

To fix it even further: it's very counter intuitive to rely on the user of the function to pass 1 element array into it.

This topic is closed to new replies.

Advertisement