incrementing a char pointer

well, I think I might have posted it a while back, but incase I didn't, I wrote this function some time ago:

Code:

void EncryptRecord(char *szRec, unsigned long nLen, char *szKey)
{
	unsigned long i;
	char *p;

	p = szKey;

	for(i = 0; i < nLen; i++) {
		if(!(*p))
			p = szKey;

		*szRec ^= *p;
		*szRec += *p;

		szRec++;
		p++;
	}
}

szRec in this function is passed as a char array (string) without its index (generating the address of the first element). so I have something looking like this:

Code:

char buf[256];

/* buf gets some data somewhere in here */

EncryptRecord(buf, dwBytesRead, szKey);

which works fine. now that I have written DecryptRecord:

Code:

void DecryptRecord(char *szRec, unsigned long nLen, char *szKey)
{
	unsigned long i;
	char *p;

	p = szKey;

	for(i = 0; i < nLen; i++) {
		if(!(*p))
			p = szKey;

		*szRec -= *szKey;
		*szRec ^= *szKey;

		szRec++;
		szKey++;
	}
}

which isn't much different, except for the fact that I am passing it a slightly different value for szRec. I am passing it a true char pointer (that is one that is declared as char* with its memory allocated with malloc). so I have something like this:

Code:

char *p = malloc(dwFileSize);

/* p gets some data somewhere in here */

DecryptRecord(p, dwBytesRead, szKey);

however when I execute the DecryptRecord, I get what looks like an error caused by me playing in memory that I do not own (one of those Windows errors stating that the application has to be closed). when I comment out the DecryptRecord statement, everything works fine, so it is obviously causing the error (and don't let the different variables dwFileSize and dwBytesRead fool you, I can assure you that they both hold the same value, or execution won't get to DecryptRecord).

I'm fairly new with pointers, so I don't see what the real difference is here, however there must be one.

can anyone please explain to me why this is happening and how I can fix it?


any help is greatly appreciated, thank you in advance.

Well given
void foo ( char *ptr );

Calling it with either of these is identical as far as the called function is concerned
char arr[10]; foo( arr );
char *p = malloc(10); foo( p );

Show the code which determines the file size and reads the file.
I see two different variables (filesize and bytesread).

hmm. thats what I thought.

here is the code that does the reading/writing of the files (sorry about the Win32 API functions, I know this isn't the Windows Programming forum).

this is the declaration of buf:

Code:

char *buf = "";

here is the code that does the reading/writing with buf, note the call to DecryptRecord commented out:

Code:

hFile = CreateFile(szPath, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
		if(hFile == INVALID_HANDLE_VALUE)
			return 1;

		wsprintf(szKey, "%d", pfile_data->key);

		MessageBox(NULL, szKey, NULL, MB_OK);

		buf = malloc(pfile_data->size); /* I know I should add some error handling here */

		ReadFile(hStub, buf, pfile_data->size, &dwBytesRead, NULL);
		wsprintf(tstbuf, "read %d bytes", dwBytesRead);
		MessageBox(NULL, tstbuf, NULL, MB_OK);
		/*
		DecryptRecord(buf, dwBytesRead, szKey);
		*/
		WriteFile(hFile, buf, dwBytesRead, &dwBytesWritten, NULL);
		if(dwBytesWritten != dwBytesRead) {
			wsprintf(tstbuf, "%x", GetLastError());
			MessageBox(NULL, tstbuf, NULL, MB_OK);
                           free(buf);
                           CloseHandle(hFile);

			return 2;
		}

		free(buf);
		CloseHandle(hFile);

ignore the wsprintf and MessageBox statements, they are just still there from some debugging I was doing earlier (they all print successfully, except for of course the last one handling the write error). just for the first file this code encounters when the DecryptRecord statement is uncommented however, as as soon as it gets executed I get the error from Windows, and the application gets closed before anything else can happen. but with it commented out like shown, the message boxes print successfully for every file that this code encounters (it is in a loop, incase that sounds loopy to you ).

also note that when it is commented out like shown not only do the message boxes print successfully for all files, but all the files are actually written successfully. with it uncommented, only the first file is written, and it contains 0 bytes (probably because execution isn't making it to WriteFile with the DecryptRecord error).

How did you declare szKey ?
Is it an array, is it allocated, does it have enough space?

If you put this into a separate program, does it work?
The problem with memory faults is that where it fails often has nothing to do with where the problem originated, and no amount of inspecting the code where it's noticed helps.

well pfile_data->key is an int. hence this statement:

Code:

wsprintf(szKey, "%d", pfile_data->key);

this int will only be 4 numbers (or characters, whatever you wanna call them). it will no more, or no less than 4 characters. wsprintf should terminate the string with a NULL, making my DecryptRecord function work properly. szKey is declared as an array, and its declaration looks like this:

Code:

char szKey[5];

that is 4 characters for the number, and 1 character for the terminating NULL that will get added by wsprintf.

but here is something interesting from msdn on the description of wsprintf:

Security Alert Using this function incorrectly can compromise the security of your application. The string returned in lpOut is not guaranteed to be NULL-terminated.

so I tried adding this after the wsprintf statement that prints pfile_data->key into szKey:

Code:

szKey[5] = '\0';

but it still doesn't seem to solve the problem.

ran the program in ollydbg, here are some messages it gives me about the error:

DS:[00133000]=???
CL=AB
Jump from 0040104A

0040104E Access violation when reading [00133000]

0040104E > 8A0A MOV CL,BYTE PTR DS:[EDX]

however all of that means nothing to me, I don't know asm.

I guess I'll just have to rewrite the DecryptData function to use array indexing instead of pointer arithmetic.

Originally Posted by sl34k

this int will only be 4 numbers (or characters, whatever you wanna call them). it will no more, or no less than 4 characters.

Hm? The size depends on the value. A 10 digit number would obviously require more than 4 characters.

Originally Posted by sl34k

Code:

char szKey[5];

Code:

szKey[5] = '\0';

but it still doesn't seem to solve the problem.

I can't imagine that writing off the end of the array would solve anything.

this number is guaranteed to be no more or no less than 4 digits. I know this because I wrote a function to generate this random key with those specifications. this is the function used to generate the key:

Code:

void RandCryptKey(char *szIn)
{
	int i;

	do i = rand();
	while(i < 1000 || i > 10000);

	wsprintf(szIn, "%d", i);
}

and how is that writing off the end of the array? it is writing a null to the last character in the array. this would be writing off the end of the array:

Code:

szKey[6] = '\0';

and this would write a null as the last character of the key (which will screw up the decrypt process, since its using XOR, the key will be different than that used to encrypt the data):

Code:

szKey[4] = '\0';

Originally Posted by sl34k

and how is that writing off the end of the array? it is writing a null to the last character in the array. this would be writing off the end of the array:

Code:

szKey[6] = '\0';

If your array is declared as Dave quoted:

Code:

char szKey[5];

Then you have valid indexes of: 0, 1, 2 ,3 ,4.

Code:

szKey[5] = '\0';

Thus, the above is, as Dave already said, writing off the end of the array.


Quzah.

ok, so I've changed the declaration of szKey:

Code:

char szKey[4];

and added this below the wsprintf statement that prints pfile_data->key into szKey:

Code:

szKey[3] = '\0';

the good news is that this fixes the problem, I no longer get the message from Windows stating that the application needs to be closed, and the files are written successfully.

the bad news is that this does exactly what I thought it would, which is overwriting the last character in the key. the files are being written with their correct sizes, however the MessageBox statement that prints szKey now only prints 3 digit numbers, so the files are being written, however the decryption is getting screwed up by the loss of the last character in the key (the key is no longer the same key used to encrypt the data, thus it isn't really decrypting).

so I've gotten one step further and then one step back (nowhere). the only benefit here is that I can now call DecryptData without getting some error from Windows, however the call to DecryptData is now useless, as its not working to do its purpose.

so you need 5 bytes arrray
char szKey[5];

end overwrite the last charachter in the array
szKey[4] = 0;

And why are you using loop instead of simple linear modification of the rand() value to bring it into desired interval?

yeah I realized that.

And why are you using loop instead of simple linear modification of the rand() value to bring it into desired interval?

what do you mean? the way I'm doing it seems the most simple...

it works now, however someway, somehow, my encryption/decryption is getting screwed up.

here is the code I am using to encrypt the data:

Code:

while(ReadFile(hFile, buf, BUF_SIZE, &dwBytesRead, NULL) && dwBytesRead > 0) {
			EncryptRecord(buf, dwBytesRead, szKey);
			WriteFile(hStub, buf, dwBytesRead, &dwBytesWritten, NULL);
			if(dwBytesWritten != dwBytesRead)
				return 0;
		}

it is in a loop, but note that there is not a new random key generated for each iterate, this random key is generated before this loop is encountered and will remain the same for each iterate.

and here is the code I am using to decrypt it:

Code:

ReadFile(hStub, buf, pfile_data->size, &dwBytesRead, NULL);
		wsprintf(tstbuf, "read %d bytes", dwBytesRead);
		MessageBox(NULL, tstbuf, NULL, MB_OK);
		DecryptRecord(buf, dwBytesRead, szKey);
		WriteFile(hFile, buf, dwBytesRead, &dwBytesWritten, NULL);
		if(dwBytesWritten != dwBytesRead) {
			wsprintf(tstbuf, "%x", GetLastError());
			MessageBox(NULL, tstbuf, NULL, MB_OK);
			return 2;
		}

I try this with simple text files and it only decrypts the first 4 bytes . I see absolutely no reason why this shouldn't work, I have already posted the EncryptRecord and DecryptRecord functions here. I really can't see how these functions could encounter different bytes. both functions work the same way, they should both be encrypting/decrypting the same 4 bytes of data with the same 4 bytes of the key.

szKey in both the encryptor and decryptor is declared as:

Code:

char szKey[5];

and gets null terminated before the key even gets added into it:

Code:

szKey[4] = '\0';

the for loop in both EncryptRecord and DecryptRecord should be encountering the same bytes at the same location, as is happening with the first 4 bytes, but I literately can't see how the next 4 bytes could be any different here .

EDIT: also note that the messagebox statements I inserted everywhere print the same keys for the files in both the encrypter and the decrypter. not the same key for every file (each file has its own random key), but the correct keys for their corresponding files. for example:

key for file1 in encrypter prints 7683
key for file1 in decrypter prints 7683

key for file2 in encrypter prints 8721
key for file2 in decrypter prints 8721

I wouldn't be getting the first 4 bytes of the file decrypted if they were different.

I try this with simple text files and it only decrypts the first 4 bytes

It is because you are using szKey, but should use p

...
Also szKey in both functions (and p) should be declared as const char * because this string is not modified by the function... In this case it will be possible to call it with "test" string as parameter for example

ohh yeah, I never even noticed that in DecryptRecord:

Code:

szKey++;

thanks vart, without even having to test it I know thats definitely whats happening (the key isn't being reset), it should be:

Code:

void DecryptRecord(char *szRec, unsigned long nLen, char *szKey)
{
	unsigned long i;
	char *p;

	p = szKey;

	for(i = 0; i < nLen; i++) {
		if(!(*p))
			p = szKey;

		*szRec -= *p;
		*szRec ^= *p;

		szRec++;
		p++;
	}
}

like it is in my EncryptRecord . and yes I could pass szKey as a const char*, good idea. I dunno about p though, I don't think that should be a constant, since the way I am incrementing it by pointer. maybe though, its worth a try.

thanks again.

I don't think that should be a constant

I didn't say it sould be a constant

Code:

char* const p;

cannot be used because p is to be changed

Code:

const char* p;

is not const - it is pointing to the const string - and the string is actually const 'cause it is not modified.