*sigh* String array (char**) in C

[WeaselNo7]

Hi all!

I'm having evil troubles getting some simple code to work. I'm making a simple C routine that adds and remove strings to and from an array of strings. Can anyone spot what I've done wrong?

// Bulking tracker...
// Provides functionality for storing an array of strings in the 
// row memory space, together with the size of the array.
// Parameters:
// ADD xxxx 		- Adds an entry at the end of the array.
// REMOVE xxxx	- Removes an entry (if it exists).
// RESET			- Resets to a 0 length array.


typedef struct bulkstype {
	char** bulks;
	int count;
} bulks_struct;



// Debug stuff.
char popmsg[128];
bool DEBUG = true;

// Param one : mode can be one of the following:
// ADD
char trigger[33];
Get_trigger(trigger, 0);

// Param two : String to add/remove.
char param1[16];
Get_param(param1, 1);


// Get our structure.
bulks_struct* bulks;
Get_bulks( bulks );


// By the time we get here, it's a valid structure.

if (strcmp(trigger,"ADD") == 0) 
{
	if( DEBUG ) 
	{
		sprintf( popmsg, "Adding to %s at %d", param1, bulks->count );
		Pop_Msg( popmsg );
	}

	int position = bulks->count + 1;

	//Create new one, but one bigger in size.
	bulks_struct* bulks_new;	
	bulks_new = new bulks_struct();
	bulks_new->count = position;
	bulks_new->bulks = new char*[position];
	// Copy original data across.
	for( int i=0; i<(position-1); i++ ) 
	{
		strcpy(bulks_new->bulks[i], bulks->bulks[i]);
	}
	
	// Add new item at end.
	strcpy( bulks_new->bulks[position-1], param1);
	
	// Clear up old stuff and assign me as the new bulks thing.
	delete( bulks );
	bulks = bulks_new;
}	
else if (strcmp( trigger, "REMOVE" ) == 0) 
{
	if( DEBUG )
	{
		sprintf( popmsg, "Removing %s", param1 );
		Pop_Msg( popmsg );
	}

	int count = bulks->count;
	int found = -1;
	int i=0;
	
	// Search for matching entry.
	for( i=0; i<count; i++ ) 
	{
		if( strcmp(bulks->bulks[i], param1) == 0  ) 
		{
			if( DEBUG )
			{
				sprintf( popmsg, "Found %s at %d", bulks->bulks[i], i );
				Pop_Msg( popmsg );
			}
			found = i;
		}
	}

	// If we didn't find it... quit
	if( found == -1 ) 
	{
		sprintf( popmsg, "Couldn't find it, exiting." );
		Pop_Msg( popmsg );
		return 0;
	}
	
	//Create new one, but one smaller in size.
	bulks_struct* bulks_new;	
	bulks_new = new bulks_struct();
	bulks_new->count = count-1;
	bulks_new->bulks = new char*[count-1];
	// Copy original data across.
	int current=0;
	for( int i=0; i<(count); i++ ) 
	{
		// Except of course the item we don't want.
		if( i <> found ) 
		{
			strcpy(bulks_new->bulks[current], bulks->bulks[i]);
			current++;
		}
	}	
	
	// Clear up old stuff and assign me as the new bulks thing.
	delete( bulks );
	bulks = bulks_new;
}
else if (strcmp( trigger, "RESET" ) == 0)
{		
	if( DEBUG ) 
	{
		sprintf( popmsg, "Resetting at size %d", bulks->count );
		Pop_Msg( popmsg );
	}

	// Create a new collection with 0 size.
	bulks_struct* bulks_new;	
	bulks_new = new bulks_struct();
	bulks_new->count = 0;
	
	// Clear up old stuff and assign me as the new bulks thing.
	delete( bulks );
	bulks = bulks_new;
	Set_UserPointer((char *)bulks);
}

// DEBUG, prints out current structure.
char cur_str[16];
for( int i=0; i<(bulks->count); i++ ) 
{
	strcpy( cur_str, bulks->bulks[i] );
	sprintf( popmsg, "%d : %s", i, cur_str );
	Pop_Msg( popmsg );
}

Set_bulks( bulks );

Pop_Msg simply puts a message to my output, and Set_bulks and Get_bulks get and set the structure I'm using.

My use of pointers is awful, and I'm also concerned about my freeing of memory.

Can anyone help?

Weasel.

 

[Salem]

1. This is written in C++, not C.

2. bulks_new = new bulks_struct();
This is probably different to what you probably meant, namely
[tt]bulks_new = new bulks_struct;[/tt]

3. strcpy(bulks_new->bulks, bulks->bulks);
You didn't allocate space for the strings in your new bulk struct. Assuming the old one had allocated space for each string, you should just copy the pointers with
[tt]bulks_new->bulks = bulks->bulks;[/tt]

4. strcpy( bulks_new->bulks[position-1], param1);
You don't allocate any space for this either, say
[tt]bulks_new->bulks[position-1] = new char[strlen(param1)+1];
strcpy( bulks_new->bulks[position-1], param1);[/tt]

5. delete( bulks );
Are you sure this is the same as
[tt]delete [] bulks;[/tt]

6. bulks = bulks_new;
Since you didn't actually paste the prototype of your function, I can't tell whether you passed this by value or by reference. If it's by value, then the caller function will NOT see the modified pointer, and will continue to use the old (and now invalid) pointer.

--

 

[WeaselNo7]

Thanks Salem,

I need to make efforts to making this C and not C++!
We should assume that nothing is being passed to this method, either by val or by ref, and I'm fetching all my data using methods in the code.
Here's my next version of the code!

// Bulking tracker...
// Provides functionality for storing an array of strings in the 
// row memory space, together with the size of the array.
// Parameters:
// ADD xxxx 		- Adds an entry at the end of the array.
// REMOVE xxxx	- Removes an entry (if it exists).
// RESET			- Resets to a 0 length array.

typedef struct bulkstype {
	char** bulks;
	int count;
} bulks_struct;


// Debug stuff.
char popmsg[128];
bool DEBUG = true;

// Param one : mode can be one of the following:
// ADD
char trigger[33];
Get_trigger(trigger, 0);

// Param two : String to add/remove.
char param1[16];
Get_param(param1, 1);


// Get our structure.
bulks_struct* bulks;
Get_bulks( bulks );


// By the time we get here, it's a valid structure.

if (strcmp(trigger,"ADD") == 0) 
{
	if( DEBUG ) 
	{
		sprintf( popmsg, "Adding to %s at %d", param1, bulks->count );
		Pop_Msg( popmsg );
	}

	int position = bulks->count + 1;

	//Create new one, but one bigger in size.
	bulks_struct* bulks_new;	
	bulks_new = (bulks_struct*)malloc(sizeof(bulks_struct));
	bulks_new->count = position;
	bulks_new->bulks = (char**)malloc(position * sizeof(char*));
	// Copy original data across.
	for( int i=0; i<(position-1); i++ ) 
	{
		bulks_new->bulks[i] = bulks->bulks[i];
	}
	
	// Add new item at end.
	bulks_new->bulks[position-1] = param1;
	
	// Clear up old stuff and assign me as the new bulks thing.
	free( bulks );
	bulks = bulks_new;
}	
else if (strcmp( trigger, "REMOVE" ) == 0) 
{
	if( DEBUG )
	{
		sprintf( popmsg, "Removing %s", param1 );
		Pop_Msg( popmsg );
	}

	int count = bulks->count;
	int found = -1;
	int i=0;
	
	// Search for matching entry.
	for( i=0; i<count; i++ ) 
	{
		if( strcmp(bulks->bulks[i], param1) == 0  ) 
		{
			if( DEBUG )
			{
				sprintf( popmsg, "Found %s at %d", bulks->bulks[i], i );
				Pop_Msg( popmsg );
			}
			found = i;
		}
	}

	// If we didn't find it... quit
	if( found == -1 ) 
	{
		sprintf( popmsg, "Couldn't find it, exiting." );
		Pop_Msg( popmsg );
		return 0;
	}
	
	//Create new one, but one smaller in size.
	bulks_struct* bulks_new;	
	bulks_new = (bulks_struct*)malloc(sizeof(bulks_struct));
	bulks_new->count = count-1;
	bulks_new->bulks = (char**)malloc((count-1) * sizeof(char*));
	// Copy original data across.
	int current=0;
	for( int i=0; i<(count); i++ ) 
	{
		// Except of course the item we don't want.
		if( i <> found ) 
		{
			bulks_new->bulks[current] = bulks->bulks[i];
			current++;
		}
	}	
	
	// Clear up old stuff and assign me as the new bulks thing.
	free( bulks );
	bulks = bulks_new;
}
else if (strcmp( trigger, "RESET" ) == 0)
{		
	if( DEBUG ) 
	{
		sprintf( popmsg, "Resetting at size %d", bulks->count );
		Pop_Msg( popmsg );
	}

	// Create a new collection with 0 size.
	bulks_struct* bulks_new;	
	bulks_new = (bulks_struct*)malloc(sizeof(bulks_struct));
	bulks_new->count = 0;
	
	// Clear up old stuff and assign me as the new bulks thing.
	free( bulks );
	bulks = bulks_new;
}

// DEBUG, prints out current structure.
char cur_str[16];
for( int i=0; i<(bulks->count); i++ ) 
{
	strcpy( cur_str, bulks->bulks[i] );
	sprintf( popmsg, "%d : %s", i, cur_str );
	Pop_Msg( popmsg );
}

Set_bulks( bulks );

Hehehehe I'm so crap at this it's funny!

Thanks for your help!

 

[Salem]

Better, but you're still compiling the code using a C++ compiler. Rename your source code to be prog.c (not prog.cpp).

> int count = bulks->count;
C++ allows declarations to be mixed with statements, C does not.

> bulks_new = (bulks_struct*)malloc(sizeof(bulks_struct));
Casting malloc in C++ is necessary (but you should be using new anyway).
In C, the cast only serves to hide a possible error when you fail to include stdlib.h

> bulks_new->bulks[position-1] = param1;
You didn't allocate space like I showed you.
All you have now is every entry in your table pointing at the same array, which means everything will print the same thing you last stored there.
malloc the space, and strcpy it.

> Get_bulks( bulks );
Again, you're passing by value here, so whatever happens to the pointer inside the function will NOT be reflected back in the caller.

Here's a demo.

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

void bad ( char *p ) {
  free ( p );
  p = malloc( 50 );
}
void good_1 ( char **p ) {
  free ( *p );
  *p = malloc ( 150 );
}
char *good_2 ( char *p ) {
  free ( p );
  p = malloc ( 250 );
  return p;
}

int main ( ) {
  char *p;

  /* This is very bad, not only is p not modified */
  /* by calling the function, the pointer is now */
  /* invalid since it's pointing at free'd memory */
  /* In addition, the memory which was allocated in */
  /* the function is now lost (it's a memory leak) */
  p = malloc ( 5 );
  printf( "p before bad=%p\n", p );
  bad ( p );
  printf( "p after bad=%p\n", p );

  p = malloc ( 5 );
  printf( "p before good_1=%p\n", p );
  good_1 ( &p );
  printf( "p after good_1=%p\n", p );

  printf( "p before good_2=%p\n", p );
  p = good_2 ( p );
  printf( "p after good_2=%p\n", p );
  free ( p );

  return 0;
}

And my output

$ valgrind a.out
==6170== Memcheck, a memory error detector for x86-linux.
==6170== Copyright (C) 2002-2005, and GNU GPL'd, by Julian Seward et al.
==6170== Using valgrind-2.4.0, a program supervision framework for x86-linux.
==6170== Copyright (C) 2000-2005, and GNU GPL'd, by Julian Seward et al.
==6170== For more details, rerun with: -v
==6170==
p before bad=0x1b933028
p after bad=0x1b933028
p before good_1=0x1b9330c8
p after good_1=0x1b933100
p before good_2=0x1b933100
p after good_2=0x1b9331c8
==6170==
==6170== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 13 from 1)
==6170== malloc/free: in use at exit: 50 bytes in 1 blocks.
==6170== malloc/free: 5 allocs, 4 frees, 460 bytes allocated.
==6170== For counts of detected errors, rerun with: -v
==6170== searching for pointers to 1 not-freed blocks.
==6170== checked 49152 bytes.
==6170==
==6170== LEAK SUMMARY:
==6170==    definitely lost: 50 bytes in 1 blocks.
==6170==      possibly lost: 0 bytes in 0 blocks.
==6170==    still reachable: 0 bytes in 0 blocks.
==6170==         suppressed: 0 bytes in 0 blocks.
==6170== Use --leak-check=full to see details of leaked memory.

Observe that bad
- does not update the pointer in main()
- leaks the 50 bytes it did allocate.

> if (strcmp(trigger,"ADD") == 0)
Separate out the functionality, to make each function more readable.

Eg.

if (strcmp(trigger,"ADD") == 0) {
  doAdd( /* the parameters you got */ );
} else if (strcmp( trigger, "REMOVE" ) == 0) {
  doRemove( /* the parameters you got */ );
} else if (strcmp( trigger, "RESET" ) == 0) {
  doReset( /* the parameters you got */ );
}

--