Parse function, when called multiple times, gives strange errors

[crh0872]

I'm new to C++, and I have a function, char* parse( char* buf, char* find_this ) which finds an instance of find_this inside of buf . Once it finds that, it looks ahead in the buf text, (which is actually JSON formatted) finds the value in the text, and it returns a pointer to a char array (created with new ) of that value.

When called once, it works fine. But when I call it multiple times, I get strange errors, usually a bus error but sometimes more lengthy ones. I think it's the way I'm using my types...anyone have any suggestions? Thanks

[nucleon]

Can we see the function? Remember to use code tags.

[crh0872]

alright, sorry it's a bit messy

const char * sf_extract_key( const char * buf, const char * find_this )
{
	int x, b, found_value_length, buf_length = 0;
	char found_key[MAX_KEYNAME_CHARLENGTH];
	char delim;
	char* found_value;
	
	while ( buf[buf_length] != '\0' ) buf_length++;
	
	for ( int i=0; i < buf_length; i++ )
	{

		if ( buf[i] == '"' && buf[i-1] != '\\')
		{
			i++; // move past starting "
			for ( b=0; b<MAX_KEYNAME_CHARLENGTH && buf[b+i] != '"' || buf[(b+i)-1] == '\\'; b++ )
				found_key[b] = buf[b+i];
			found_key[b] = '\0';

			if ( strcmp(find_this, found_key) == 0 )
			{

				i=b+i+1; // move past ending "
				if ( buf[i] == ':' && buf[i+1] != '{' && buf[i+1] != '[') // no bracket support.
				{		
					i++; //move past :
					
					if (! isdigit( buf[i]) )
					{
						i++;
						delim = '\"';
					}
						else delim=',';
					
					for ( found_value_length=0; buf[i+found_value_length] != delim || buf[(i+found_value_length)-1] == '\\'; found_value_length++ );
					found_value = new char[found_value_length+1];
					
						for ( x=0; x < found_value_length; x++ )
							found_value[x]=buf[i+x];
						found_value[x]='\0';
					
					if (verbose) cout << "PARSER: parsed " << found_key <<
						" which has value "<< found_value <<endl;

					return found_value;
					i=i+x;
				}
			}
			else
				i=i+b;
		}
	}
	return 0;
}

and I'm calling it like this:

const char* text = sf_extract_key(packet_text, "text");
		const char* from_full = sf_extract_key(packet_text, "from_name");
		const char* from_first_name = sf_extract_key(packet_text, "from_first_name");
	
		cout << from_full << " " << from_first_name << endl << endl << text << endl;

What's so weird is that if "text" is bigger than 20 chars, that's when I get a bus error...yet I dynamically allocated it so it can't be going out of range.

[nucleon]

The problem seems to be that you're incrementing i all over the place but only testing if it exceeds (or equals) buf_length at the top of the loop indexed by i.

There is also one possibility of an access before the beginning of the array, so I added a check for that.

I added parens to a condition.
You had: cond1 && cond2 || cond3 but meant: cond1 && (cond2 || cond3) I added an assert at a critical place, but you will need more tests to ensure that i does not go out-of-range. The rest is up to you.

const char * sf_extract_key( const char * buf, const char * find_this )
{
	int x, b, found_value_length, buf_length = 0;
	char found_key[MAX_KEYNAME_CHARLENGTH];
	char delim;
	char* found_value;

//!	while ( buf[buf_length] != '\0' ) buf_length++;
	buf_length = strlen( buf ); //! same thing!

	for ( int i=0; i < buf_length; i++ )
	{
		//! Added test to protect against buf[i-1] looking before buf[0]
		if ( buf[i] == '"' && (i == 0 || buf[i-1] != '\\'))
		{
			i++; // move past starting "

			//! Added parens around the or clause
			for ( b=0;
				  b<MAX_KEYNAME_CHARLENGTH &&
				  (buf[b+i] != '"' || buf[(b+i)-1] == '\\');
				  b++ )
				found_key[b] = buf[b+i];
			found_key[b] = '\0';

			if ( strcmp(find_this, found_key) == 0 )
			{

				i=b+i+1; // move past ending "

				//! This is just a debugging aid.
				assert(i < buf_length - 1);

				if ( buf[i] == ':' && buf[i+1] != '{' && buf[i+1] != '[') // no bracket support.
				{
					i++; //move past :

					if (! isdigit( buf[i]) )
					{
						i++;
						delim = '\"';
					}
						else delim=',';

					for ( found_value_length=0;
						  buf[i+found_value_length] != delim ||
						  buf[(i+found_value_length)-1] == '\\';
						  found_value_length++ )
						  ;
					found_value = new char[found_value_length+1];

						for ( x=0; x < found_value_length; x++ )
							found_value[x]=buf[i+x];
						found_value[x]='\0';

					if (verbose) cout << "PARSER: parsed " << found_key <<
						" which has value "<< found_value <<endl;

					return found_value;
					i=i+x;
				}
			}
			else
				i=i+b; //! This goes directly to the loop test, so it's okay
		}
	}
	return 0;
}

[crh0872]

Ok, thanks nucleon for the repsonse. I'm going to separate this into smaller functions and try to implement more checks for the size of i. However, I've been testing it with your modifications and it works as it did before: when the "text" value inside of buf is longer than 20 characters, it either gives a "Bus error" or it just crashes. Again, I'm calling it like this:

const char* text = sf_extract_key(packet_text, "text");
		const char* from_full = sf_extract_key(packet_text, "from_name");
		const char* from_first_name = sf_extract_key(packet_text, "from_first_name");
	
		cout << from_full << " " << from_first_name << endl << endl << text << endl;

if i call it like this, though:

const char* text = sf_extract_key(packet_text, "text");
		cout << text << endl;

no problem whatsoever. So I'm thinking it's a variable / type misuse?

[nucleon]

I would've run it if I had any data! Actually, I just made some up and got it to run, but I'm not sure how to make it crash. Could you post a single-file (minimal), runnable (crashable, actually!) example with hardcoded data that makes the program crash?

[crh0872]

alrighty, I made it as simple as possible, the buf string is a lot shorter than what I'm normally throwing into the function, but it still crashes, yay!

[nucleon]

You define a buffer called found_key to hold each key as it is read. It has a length of MAX_KEYNAME_CHARLENGTH (defined, suspiciously, as 20).

If you are sure your keys will never be more than 19 characters long (leaving space for a terminating null), then this is okay, except for one thing: you are also reading values into this same variable as if they were keys! You need to skip the value of keys your not interested in so that you don't read them into found_key and possibly overrunning the buffer or even misinterpretting the value as a key.

Basically you'll have to skip values in exactly the way you do when you want to read them, except you won't be storing them.

[crh0872]

oooooooohhh!!!! thank you so much! I knew the number 20 seemed way too coincidental to match up to MAX_KEYNAME_CHARLENGTH, but I never realized it could be doing that.

awesome, you are the man

[nucleon]

No problem. :)

While I remember, you should change the test: b<MAX_KEYNAME_CHARLENGTH to b<MAX_KEYNAME_CHARLENGTH - 1 , otherwise you might overrun found_key.

And remember to test the return value of your function since it returns 0 when it fails (dereferencing this null pointer is what ultimately caused your crash).

[crh0872]

And remember to test the return value of your function since it returns 0 when it fails (dereferencing this null pointer is what ultimately caused your crash).

Yup, I learned that when I finished the new code yesterday. And to fix what you mentioned, I went ahead and just allocated new memory for the found_value variable.

Everything is working great now