Bug hunting with Valgrind

I'm using Valgrind to find if there's bug in my program (i'm coding under linux) and it find someting, but i don't understand where's the problem.

Valgrind sais:

==5397== Thread 2:
==5397== Conditional jump or move depends on uninitialised value(s)
==5397== at 0x401358: ListSearch (packethandler.c:97)
==5397== by 0x40177D: HandlePackets (packetreader.c:47)
==5397== by 0x401643: Listen (packethandler.c:146)
==5397== by 0x4E2BFC6: start_thread (in /lib/libpthread-2.7.so)
==5397== by 0x51105AC: clone (in /lib/libc-2.7.so)
==5397==
==5397== Jump to the invalid address stated on the next line
==5397== at 0x0: ???
==5397== by 0x401643: Listen (packethandler.c:146)
==5397== by 0x4E2BFC6: start_thread (in /lib/libpthread-2.7.so)
==5397== by 0x51105AC: clone (in /lib/libc-2.7.so)
==5397== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==5397==
==5397== Process terminating with default action of signal 11 (SIGSEGV)
==5397== Bad permissions for mapped region at address 0x0
==5397== at 0x0: ???
==5397== by 0x401643: Listen (packethandler.c:146)
==5397== by 0x4E2BFC6: start_thread (in /lib/libpthread-2.7.so)
==5397== by 0x51105AC: clone (in /lib/libc-2.7.so)
--5397-- REDIR: 0x50b69e0 (free) redirected to 0x4c20b00 (free)

For the first error valgrind is referring to that conditional jump:

Code:

RecvPacketList *ListSearch(RecvPacketList *plist, unsigned char i)
{
        while (plist != NULL)<- THIS ONE
        {
                if (plist->ID == i)
                {
                        return plist;
                }
                plist = plist->next;
        }
        return NULL;
}

ListSearch is called here:

Code:

void HandlePackets(RecvPacketList **plist, char *recvBuff)
{	
	unsigned int ID = -1;
	RecvPacketList *list;
	while(ID != 0)
	{
		ID = (unsigned int)*recvBuff;
		recvBuff++;
		list = ListSearch(*plist, ID);
		if(list != NULL){
			pFunction pfunc = list->func;
			(*pfunc)(&recvBuff);
		}
	}
}

And going again "back", HandlePackets is called here:

Code:

void *Listen(void *args)
{
	char recvPacketBuff[1024] = {0};
	RecvPacketList *plist;
	fd_set fset;
	FD_ZERO(&fset);
	printf("Initializing Packet List: ");
	Initialize(&plist);
	printf("Done\n");
	
	while(1)
	{
		FD_SET(sock_fd, &fset);
		select(sock_fd + 1, &fset, NULL, NULL, NULL);
		if(FD_ISSET(sock_fd, &fset)){
			recv(sock_fd, &recvPacketBuff, sizeof(recvPacketBuff), 0);
			HandlePackets(&plist, recvPacketBuff); <- THE SECOND ERROR, ln 146
			CleanBuff(recvPacketBuff);
		}
	}
}

And as you can see here.. plist IS initialized:

Code:

void Initialize(RecvPacketList **plist)
{
	pFunction temp = NULL;
	*plist = malloc(sizeof(RecvPacketList));
	if(*plist == NULL){
		printf("Error to allocate space for plist");
		exit(1);
	}
	(*plist)->ID = 0;
	(*plist)->func = NULL;
	Register(0x0B,plist, temp = DamagePacket);	
	Register(0x24,plist, temp = DisplayBuyList);
	Register(0x25,plist, temp = TradeEquipOp);
	Register(0x3C,plist, temp = VendorBuyContentOp);
	Register(0x6F,plist, temp = SecureTradeOp);
	Register(0x74,plist, temp = VendorBuyList);
	Register(0x82,plist, temp = AccountLoginRej);
	Register(0xA8,plist, temp = AccountLoginAck);
	Register(0xBA,plist, temp = ArrowOp);
	Register(0xB7,plist, temp = ObjectHelpResponse);
}

If you want also to see register:

Code:

void Register(unsigned char value, RecvPacketList **plist,  pFunction func)
{
	RecvPacketList *nextlist = (RecvPacketList *)malloc(sizeof(RecvPacketList));

	if(nextlist == NULL){
		printf("Memory allocation failed for nextlist");
		return;
	}
 	nextlist->next = *plist;
	*plist = nextlist;
	nextlist->ID = value;
	nextlist->func = func;

	
}

Anyway plist is a linked list that contains "static" packet informations (packet to be read).
I also highlighted where's the second error.

edit: read it more carefully. Hmmm. Well, dunno yet, but putting plist as a ** and a * was confusing and I really see no reason on having a ** when you use it as a *

But it's initialized!
Maybe it's difficult to understand the order of the execution so:

All starts from Listen function (called by a new thread created), as you can see there is the "real" RecvPacketList *plist.
Now i need to initialize this pointer to a structure so i pass it byreference through function Initialize and the function accept a pointer to pointer to structure so i'm sure i'm modifying that pointer and not a copied one.
In the function it allocates memory for that pointer, it set ID to 0 and func to NULL, then Register function appends other RecvPacketList structures (to create the linked list) so the last structure it link is the first i can encounter, and it's intialized as you can see:

Code:

RecvPacketList *nextlist = (RecvPacketList *)malloc(sizeof(RecvPacketList));

	if(nextlist == NULL){
		printf("Memory allocation failed for nextlist");
		return;
	}
 	nextlist->next = *plist;
	*plist = nextlist;
	nextlist->ID = value;
	nextlist->func = func;

After initialization we are back in Listen function and it pass plist (the "real" one, now it's no more the pointer pointing to the space allocated in Initialize function but the pointer to the space allocated in the last Register) by reference through HandlePackets, so i'm sure again is that pointer and not another one, and then it pass it to ListSearch.

I also debugged with Kdevelop and gdb and i can see that plist in ListSearch is initialized because has values etc..

> In the function it allocates memory for that pointer, it set ID to 0 and func to NULL
But it doesn't set ->next to NULL (which is what you eventually get to test, and valgrind objects to)

> then Register function appends other RecvPacketList structures
Being pedantic, it prepends to the front of the list, not appends to the end.

> Register(0x0B,plist, temp = DamagePacket);
Do all these temp = do anything useful?

Originally Posted by Salem

> In the function it allocates memory for that pointer, it set ID to 0 and func to NULL
But it doesn't set ->next to NULL (which is what you eventually get to test, and valgrind objects to)

Oh lol it was so simple XD, but i thought valgrind was referring to the last object i added to the list :\

Originally Posted by Salem

> then Register function appends other RecvPacketList structures
Being pedantic, it prepends to the front of the list, not appends to the end.

Don't worry you're not pedantic :P and i'm not english, i chose the wrong word ^^

Originally Posted by Salem

> Register(0x0B,plist, temp = DamagePacket);
Do all these temp = do anything useful?

No, it was an old try, when i wasn't sure if had to pass the function by name or pass it as a pointer (so using a variable).
I removed it.
Aniway there's still the second problem...

So what's the second problem?

If you're referring to the "Jump to the invalid address stated on the next line", well that's the code falling off the end of the broken list.
If the list is fixed, then the code won't jump off into hyperspace.

Fresh code + fresh trace if you've still got problems.

Yes i'm referring to the jump, and i still have that problem, i only changed a line to solve the first problem.

This:

Code:

void Initialize(RecvPacketList **plist)
{
	pFunction temp = NULL;
	*plist = malloc(sizeof(RecvPacketList));
	if(*plist == NULL){
		printf("Error to allocate space for plist");
		exit(1);
	}
	(*plist)->ID = 0;
	(*plist)->func = NULL;
	Register(0x0B,plist, temp = DamagePacket);	
	Register(0x24,plist, temp = DisplayBuyList);
	Register(0x25,plist, temp = TradeEquipOp);
	Register(0x3C,plist, temp = VendorBuyContentOp);
	Register(0x6F,plist, temp = SecureTradeOp);
	Register(0x74,plist, temp = VendorBuyList);
	Register(0x82,plist, temp = AccountLoginRej);
	Register(0xA8,plist, temp = AccountLoginAck);
	Register(0xBA,plist, temp = ArrowOp);
	Register(0xB7,plist, temp = ObjectHelpResponse);
        etc....................................
}

Now is (i also removed the useless temp variable ^^):

Code:

void Initialize(RecvPacketList **plist)
{
	*plist = malloc(sizeof(RecvPacketList));
	if(*plist == NULL){
		printf("Error to allocate space for plist");
		exit(1);
	}
	(*plist)->ID = 0;
	(*plist)->func = NULL;
	(*plist)->next = NULL; <- THIS IS THE NEW LINE
	Register(0x0B,plist, DamagePacket);	
	Register(0x1A,plist, WorldItem);
	Register(0x1D,plist, RemoveItem);
        etc............................
}

Maybe test pfunc for NULL before trying to call it

The fact is that pfunc cannot be null if ListSearch find a valid ID, it was my fault to initialize plist->ID (in Initialize function) to 0, because when HandlePacket encounter a 0 ID in the buffer and use ListSearch, this function returns the structure with all NULL, so to resolve i simply initialized ID to -1.
Thnx to all, stupid bugs, the next time is better to think more eheh!