Sign in to follow this  

Access violations

This topic is 3723 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

char* convert(const unsigned char* name) {
	char buf[512], *tt = NULL, *p = NULL;
	int i, val;

	tt = buf;
	while( 1 ) {
		val = *name++;
		if( val == 0 )
			break;
		for(i=0; i<val; i++) {
			*tt++ = *name++;
		}
		*tt++ = '.';
	}

	*(tt-1) = 0;

	p = (char*)malloc(strlen(buf)+1);
	strcpy(p, buf);
	return p;
}

unsigned char* decompress(unsigned short len, unsigned char* data, unsigned char* buf) {
	unsigned char temp[512];
	int j, offset=0;
	for(j=0; j<len; j++) {
		if( (data[j] >> 4 & 0xc) == 0xc ) {
			unsigned char* ins = &buf[data[j+1]];
			memcpy(temp+j, &buf[data[j+1]], strlen((char*)ins));
			offset += strlen((char*)ins);
			j++;
		}
		else
			temp[j+offset] = data[j];
	}
	temp[j+offset] = 0;
	return temp;
}

...
struct RR {
unsigned char* name; 
}
char inBuf[65536];
read(inBuf);
RR rr;
rr.name = &inBuf;
unsigned char * pop = decompress(strlen((char*)rr.name), rr.name, inBuf); char* out = convert(pop); In the above code, when I call decompress, it stores the correct value in pop. However when I call convert(pop), I get an access violation, because the value of the input parameter to convert is all garbled, instead of the value of pop.

Share this post


Link to post
Share on other sites
Quote:
Original post by Sfpiano
unsigned char* decompress(unsigned short len, unsigned char* data, unsigned char* buf) {
unsigned char temp[512];
//....
return temp;
}


You're returning a local variable. After calling decompress, temp becomes invalid. You may be able to still use it, but surely after calling another function you'll get an access violation.
Too strange that your compiler didn't warn you about that
You should change:
unsigned char temp[512];
for
unsigned char *temp = new char[512];
(or it's C counter part, unsigned char *temp = (char*)malloc( 512*sizeof(char) );

Note that after calling convert(), this is, after finishing using pop; you should destroy memory:
delete [] pop;
(or it's C counter part, free(pop); )

You may also consider if your app is not very complex, to make pop a global variable (not recommended)

If you're using C++; you may consider using smart-pointers from the STD library. Learn about them, they will free you from the ugly work of dealing with memory management.

Hope this helps
Dark Sylinc

Share this post


Link to post
Share on other sites
Quote:
Original post by Matias Goldberg
Quote:
Original post by Sfpiano
unsigned char* decompress(unsigned short len, unsigned char* data, unsigned char* buf) {
unsigned char temp[512];
//....
return temp;
}


You're returning a local variable. After calling decompress, temp becomes invalid. You may be able to still use it, but surely after calling another function you'll get an access violation.
Too strange that your compiler didn't warn you about that
You should change:
unsigned char temp[512];
for
unsigned char *temp = new char[512];
(or it's C counter part, unsigned char *temp = (char*)malloc( 512*sizeof(char) );

Note that after calling convert(), this is, after finishing using pop; you should destroy memory:
delete [] pop;
(or it's C counter part, free(pop); )

You may also consider if your app is not very complex, to make pop a global variable (not recommended)

If you're using C++; you may consider using smart-pointers from the STD library. Learn about them, they will free you from the ugly work of dealing with memory management.

Hope this helps
Dark Sylinc


Uh, in C++ we represent strings with std::string and avoid all that nonsense. The only smart pointer in the *standard* library is std::auto_ptr, and although they are useful here, they're a very roundabout way of addressing the problem (by insisting on still thinking about the data at too low a level).

I tried to start cleaning up the code, but got stuck, because I can't figure out what you're really trying to do. It seems like "convert" is intended to replace length-counted substrings with period-terminated ones, but I can't make any sense out of the "decompress" algorithm at all. (In particular, you memcpy() to 'temp + j', but then increment j by 1 to prepare for the next write - isn't that going to scribble over the second and subsequent characters that were copied?)

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
Uh, in C++ we represent strings with std::string and avoid all that nonsense.


Believe me, I would if I could.

Quote:
You should change:
unsigned char temp[512];

I though that might be the problem, but I was trying to avoid having a bunch of malloc'd things to keep track of. Thanks guys.

Share this post


Link to post
Share on other sites
Quote:
Original post by Sfpiano
Quote:
Original post by Zahlman
Uh, in C++ we represent strings with std::string and avoid all that nonsense.


Believe me, I would if I could.


Why is it that you can't? Also, convert is unsafe (it has a potential buffer overrun). I suspect it's not the only thing that is, but it's all I've really looked at so far.

Share this post


Link to post
Share on other sites
Btw, the VS compiler should give you a warning for "returning address of temporary" which would have pointed you at the problem quicker. If you're using VS you might want to see if you can crank up the warning level to something higher.

(If you're using gcc, then ignore this).

Share this post


Link to post
Share on other sites

This topic is 3723 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this