Thread: XOR on char buffer fails?

You assume that the d parameter of XOR_buffer will point to a null terminated string. This is not necessarily true when you're calling it on a buffer that stores the output of a previous call to XOR_buffer.

I suggest that you declare XOR_buffer to modify the buffer in-place, i.e., instead of calling malloc, operate on the existing buffer, passing its length to XOR_buffer. For example:

Code:

#include <stdio.h>
#include <string.h>

static inline char *XOR_buffer(char *buffer, size_t buffer_size, const char *key, size_t key_size) {
    /* ... this is just a simple for loop ... */
    return buffer;
}

int main(void) {
    char buffer[] = "Now is the time for all good men to came and aid their party\nThe quick brown fox jumpped iver the lazy dog";
    const size_t buffer_size = strlen(buffer); /* exclude null terminator from size */
    const char *key = "abc";
    const size_t key_size = strlen(key); /* exclude null terminator from size */

    printf("Before: '%s'\n", buffer);

    XOR_buffer(buffer, buffer_size, key, key_size);
    printf("XORed : '");
    for (size_t i = 0; i < buffer_size; ++i) {
        printf("%.2x", (int)buffer[i]);
    }
    printf("'\n");

    XOR_buffer(buffer, buffer_size, key, key_size);
    printf("After : '%s'\n", buffer);

    return 0;
}

Originally Posted by heatblazer

It appeared that XOR - CAN XOR to '\0' and return unexpected null terminated buffer.

That is what I told you earlier: "You assume that the d parameter of XOR_buffer will point to a null terminated string. This is not necessarily true when you're calling it on a buffer that stores the output of a previous call to XOR_buffer."

I actually wrote the program that I posted in post #2 such that it compiles and runs as expected. I replaced a for loop having a single line as the loop body with the comment. That line appears in some form in your code in post #1. Really, it is that simple. If you really want to use malloc with my suggested approach, you can still do so, e.g., in the main function (and remember to call free).

If you want to continue over-complicating with malloc in XOR_buffer, trying to handle a null terminated string input instead of a buffer determined by a size parameter, go ahead and good luck debugging.

EDIT:

Originally Posted by heatblazer

Why, tho, I can`t tell but is working.

Oh, you did manage to get it to work, but you don't know why? That's a Bad Thing.

• You have a memory leak from post #1 that you probably have not fixed (you assigned to nstr when it points to memory returned by malloc, before calling free).
• In this version that you say works, you call Utils.getStringFromTo, but why on earth that is necessary, I have no clue.
• If you really want to treat privateXorBuff as a null terminated string, then you forgot to allocate space for the null character.
• Why do you have privateXorBuff, begin, and ret? Your only use for begin is to use it to initialise ret, and your only use of ret is to return it, which would not be wrong if you modify the privateXorBuff pointer itself, but you don't.
• Why do you need both j and h?

In summary, your loop is inexplicably complicated: you're just iterating over two arrays of char simultaneously. If you know the sizes of the arrays (or the part thereof that you want to iterate over), doing the iteration is trivial. The portion of the buffer that you want to iterate over is determined by the null character since it is initially a null terminated string, but once you've past that, you just need to retain this size and you're good to go.

Assume that I will recieve char pointers, not char arrays. Here I`ve reworked it a bit:

Code:

#ifdef XORVER1
static inline char* XOR_buffer(const char* d,const char* k) {
    const int stringl = strlen(d);
    const int klen = strlen(k);
    char* privateXorBuff = (char*) malloc(sizeof(char)* stringl);
    myHeap[hIterator++] = privateXorBuff; //add to heap
    int i; char c;
    for (i=0; i < stringl; i++) {
        c = d[i] ^ k[i%klen];
        if ( c == '\0') c ^= k[i%klen];
        privateXorBuff[i] = c;
    }
    return privateXorBuff;
}
#else

But really, I`d like to stick with the idea, that the parameter char buff will be copied then returned. I`ve freed the 2 leaks I`ve made. Valgrind reported 0 leaks.
[EDIT] I`ve added test heap to prevent leaks in the utils.c source:

Code:

...
#define HSIZE 512000
static char* myHeap[HSIZE];
static int hIterator = 0;

Each call to malloc assigns the a copy ptr to the heap, and I have a function that iterrates in the end to free all. As you`ve said in one of my posts "poor man algorithm" but prevents the leaks now.

Originally Posted by heatblazer

OK, so the code you proposed in your first post, actually must be called for char array. Is that so?

Yes, or to a dynamic array of char.

Originally Posted by heatblazer

I still have to copy the returning buffer. I can`t really see how to avoid malloc. Imagine I can`t use the char array you proposed as the mutable string.

Then call malloc:

Code:

char *buffer = malloc(sizeof("bla bla bla"));
if (buffer)
{
    strcpy(buffer, "bla bla bla");
    const size_t buffer_size = strlen(buffer);
    XOR_buffer(buffer, buffer_size, "abc", strlen("abc"));
    // ...
    free(buffer);
}

You might ask why I bother with having a separate size parameter for the key when it does not change: well, consider that this then allows you to specify a key with embedded null characters, similiar to how the buffer might contain embedded null characters.

Originally Posted by heatblazer

I`d like to stick with the idea, that the parameter char buff will be copied then returned.

Check out the functions from <string.h>. They don't do that. This way, you can choose to use them with malloc, or not. Perhaps you would use something more low level than malloc instead, e.g., for some special performance reasons. In other words, there is separation of concerns: the functionality to perform this "XOR transform" is loosely coupled to the functionality to obtain the storage required for the buffer.

Originally Posted by heatblazer

Each call to malloc assigns the a copy ptr to the heap, and I have a function that iterrates in the end to free all. As you`ve said in one of my posts "poor man algorithm" but prevents the leaks now.

While this solution works, it means that you have global variables. Global variables make it more difficult to reason about your program because you have to consider global state. Your XOR_buffer function becomes tightly coupled with this poor man's "garbage collector", so it becomes less reusable, which is a Bad Thing for a utility library.