Help with my XOR routine..

I know that this topic has been covered a million times already, but i went through all the XOR topics on the site without figuring out what i am doing wrong in my own program?

I've attached the main program file to my post, the XOR function is defined in it's own file:

Code:

---

/* Source file for the XOR routine */

/* Include the function prototype */
#include "xor.h"

/* XOR-funtion declaration */
std::string XOR(std::string line, std::string key)
{
                unsigned int i = 0, j = 0;                                       
                std::size_t l_length, k_length;                               
                std::string e_line;               
                                       
        l_length = line.length();
        k_length = key.length();
       
        for(;i < l_length; i++, j++)                                       
        {
                e_line[i] = line[i] ^ key[j];                               
               
                if(j == k_length)                                                       
                {
                        j = 0;
                }
        }

        /* Return the encrypted line */
        return e_line;
}

---

Removed most of my comments since the forums are screwing up indentation.


I compile my project from the command line like this:
"g++ -omain.exe -Wall -pedantic xor.cpp main.cpp"

But when i run the program, nothing happens after i enter the key, i don't get any output to the outputfile, and the program just closes. Where in my code is the problem?

EDIT: Accidentally wrote "crashes" rather than "closes", my program doesn't crash or segfault or anything, it just closes without outputting to the outputfile.

What does your input file look like? What is the key you are attempting to use?

Code:

---

std::string inputfilepath, outputfilepath;                /* Used for holding the file paths */

...
       
/* Assign the input from the cmd-line to the filepath strings */
inputfilepath = argv[1];
outputfilepath = argv[2];
       
/* Open the stream to the input file */
std::ifstream inputfile(inputfilepath.c_str(), std::ios::binary);
       
...
       
/* If we got this far, then input file is valid, so we can now start to write to the outputfile */
std::ofstream outputfile(outputfilepath.c_str(), std::ios::binary);

---

Not that it's the issue here, but there appears to be no real reason for the inputfilepath and outputfilepath string variables. You can get rid of them and just use argv[1] and argv[2] directly:

Code:

---

/* Open the stream to the input file */
std::ifstream inputfile(argv[1], std::ios::binary);
       
...
       
/* If we got this far, then input file is valid, so we can now start to write to the outputfile */
std::ofstream outputfile(argv[2], std::ios::binary);

---

Quote:

---

Originally Posted by hk_mp5kpdw

What does your input file look like? What is the key you are attempting to use?

Code:

---

std::string inputfilepath, outputfilepath;                /* Used for holding the file paths */

...
       
/* Assign the input from the cmd-line to the filepath strings */
inputfilepath = argv[1];
outputfilepath = argv[2];
       
/* Open the stream to the input file */
std::ifstream inputfile(inputfilepath.c_str(), std::ios::binary);
       
...
       
/* If we got this far, then input file is valid, so we can now start to write to the outputfile */
std::ofstream outputfile(outputfilepath.c_str(), std::ios::binary);

---

Not that it's the issue here, but there appears to be no real reason for the inputfilepath and outputfilepath string variables. You can get rid of them and just use argv[1] and argv[2] directly:

Code:

---

/* Open the stream to the input file */
std::ifstream inputfile(argv[1], std::ios::binary);
       
...
       
/* If we got this far, then input file is valid, so we can now start to write to the outputfile */
std::ofstream outputfile(argv[2], std::ios::binary);

---

---

The files are called foo.txt and bar.txt.

Foo contains the string: "This doesnt work!", and bar is empty, the key im attempting to use is simply "neo1", i've tried alot of other combinations but to no prevail.

I am using the strings for the file names because it makes the code more descriptive imo, i won't need to explain everything to whoever reads the code, if the variable names do it for me. And besides, speed/size is not important at all in this small application.

Code:

---

std::string XOR(std::string line, std::string key)
{
    unsigned int i = 0, j = 0;                                       
    std::size_t l_length, k_length;                               
    std::string e_line;               
                                       
    l_length = line.length();
    k_length = key.length();
       
    for(;i < l_length; i++, j++)                                       
    {
        e_line[i] = line[i] ^ key[j];                               
               
        if(j == k_length)                                                       
        {
            j = 0;
        }
    }

    /* Return the encrypted line */
    return e_line;
}

---

I put all your code into a test project and ran it in the debugger (Visual Studio 2005). The e_line[i] = line[i] ^ key[j] line is where the code stopped with an error and it's now obvious what the issue is. Do you see?

Quote:

---

Originally Posted by hk_mp5kpdw

Code:

---

std::string XOR(std::string line, std::string key)
{
    unsigned int i = 0, j = 0;                                       
    std::size_t l_length, k_length;                               
    std::string e_line;               
                                       
    l_length = line.length();
    k_length = key.length();
       
    for(;i < l_length; i++, j++)                                       
    {
        e_line[i] = line[i] ^ key[j];                               
               
        if(j == k_length)                                                       
        {
            j = 0;
        }
    }

    /* Return the encrypted line */
    return e_line;
}

---

I put all your code into a test project and ran it in the debugger (Visual Studio 2005). The e_line[i] = line[i] ^ key[j] line is where the code stopped with an error and it's now obvious what the issue is. Do you see?

---

Yea, the e_line string is empty, so it crashes when i use the [] operator on the string, right?

What can i do to fix this? Should i fill the string with '0's or something? Can i use the += operator?

Yeah, a couple, one:

Code:

---

e_line.push_back(line[i] ^ key[j]);

---

Or maybe use resize on the string or append or when first declaring the string you can set it's size and fill it with spaces to start off? += would probably work as well.

Quote:

---

Originally Posted by hk_mp5kpdw

Yeah, a couple, one:

Code:

---

e_line.push_back(line[i] ^ key[j]);

---

Or maybe use resize on the string or append or when first declaring the string you can set it's size and fill it with spaces to start off? += would probably work as well.

---

Awesome, i'm now getting stuff written to the file :)

Still not quite done yet though, when i try to encrypt and then decrypt "this doesnt work!", the output i get is "This d~yHT"~yH (- And more stuff that that the forums can't show without bugging). Something is still wrong with the XOR function?

First, you had a subtle problem:

Code:

---

std::string XOR(std::string line, std::string key)
{
    unsigned int i = 0, j = 0;                                       
    std::size_t l_length, k_length;                               
    std::string e_line;               
                                       
    l_length = line.length();
    k_length = key.length();
       
    for(;i < l_length; i++, j++)                                       
    {
        e_line.push_back(line[i] ^ key[j]);
               
        if(j == k_length)                                                       
        {
            j = 0;
        }
    }

    /* Return the encrypted line */
    return e_line;
}

---

The problem is you increment j and use it with the key as an index to encrypt the string, then you check and make sure it's within bounds. The bounds check should be before you use it, so try this instead:

Code:

---

std::string XOR(std::string line, std::string key)
{
    unsigned int i = 0, j = 0;                                       
    std::size_t l_length, k_length;                               
    std::string e_line;               
                                       
    l_length = line.length();
    k_length = key.length();
       
    for(;i < l_length; i++, j++)                                       
    {
        if(j == k_length)                                                       
        {
            j = 0;
        }

        e_line.push_back(line[i] ^ key[j]);
    }

    /* Return the encrypted line */
    return e_line;
}

---

Now the main event. That's a problem with the way you are doing the encryption/decryption using getline. getline looks for a trailing newline character (or end of file) and then stops. When you first encrypt your string, the getline successfully reads the whole string and the 'o' in "work!" happens to get encrypted as a ASCII value 10 which is either a carriage return or line feed (I forget which). When it comes time to decrypt the string, getline stops at that character prematurely (extracts and throws away). Because of this, a second getline call is needed to read and decrypt the rest of the file. Since your encrypt/decrypt method works on whole lines, this second round of decryption starts using the wrong key index and the wrong cypher text character from what was used in the encryption step. Bottom line here is I don't think you should be using getline when doing something like this. Getting the characters one-by-one or getting all the characters at once (using an intermediate stringstream object and rdbuf) and then encrypting them with the key might be best here.

I'd do something like this for the reading instead of the while loop and getline calls:

Code:

---

#include <sstream>

...

std::stringstream sstr;
sstr << inputfile.rdbuf();  // Read whole file in one go, no loop needed
line = sstr.str();  // Convert stringstream to string
       
line = XOR(line, key);

outputfile << line;

---

Quote:

---

Originally Posted by hk_mp5kpdw

First, you had a subtle problem:

Code:

---

std::string XOR(std::string line, std::string key)
{
    unsigned int i = 0, j = 0;                                       
    std::size_t l_length, k_length;                               
    std::string e_line;               
                                       
    l_length = line.length();
    k_length = key.length();
       
    for(;i < l_length; i++, j++)                                       
    {
        e_line.push_back(line[i] ^ key[j]);
               
        if(j == k_length)                                                       
        {
            j = 0;
        }
    }

    /* Return the encrypted line */
    return e_line;
}

---

The problem is you increment j and use it with the key as an index to encrypt the string, then you check and make sure it's within bounds. The bounds check should be before you use it, so try this instead:

Code:

---

std::string XOR(std::string line, std::string key)
{
    unsigned int i = 0, j = 0;                                       
    std::size_t l_length, k_length;                               
    std::string e_line;               
                                       
    l_length = line.length();
    k_length = key.length();
       
    for(;i < l_length; i++, j++)                                       
    {
        if(j == k_length)                                                       
        {
            j = 0;
        }

        e_line.push_back(line[i] ^ key[j]);
    }

    /* Return the encrypted line */
    return e_line;
}

---

Now the main event. That's a problem with the way you are doing the encryption/decryption using getline. getline looks for a trailing newline character (or end of file) and then stops. When you first encrypt your string, the getline successfully reads the whole string and the 'o' in "work!" happens to get encrypted as a ASCII value 10 which is either a carriage return or line feed (I forget which). When it comes time to decrypt the string, getline stops at that character prematurely (extracts and throws away). Because of this, a second getline call is needed to read and decrypt the rest of the file. Since your encrypt/decrypt method works on whole lines, this second round of decryption starts using the wrong key index and the wrong cypher text character from what was used in the encryption step. Bottom line here is I don't think you should be using getline when doing something like this. Getting the characters one-by-one or getting all the characters at once (using an intermediate stringstream object and rdbuf) and then encrypting them with the key might be best here.

I'd do something like this for the reading instead of the while loop and getline calls:

Code:

---

#include <sstream>

...

std::stringstream sstr;
sstr << inputfile.rdbuf();  // Read whole file in one go, no loop needed
line = sstr.str();  // Convert stringstream to string
       
line = XOR(line, key);

outputfile << line;

---

---

Thanks alot, it's working perfectly now :)!

Would you mind explaining what exactly "rdbuf()" does? Also, is there anything else in my code that could be improved?

Alternatively you could do the following:

Code:

---

string LoadText(string fileName)
{
        int length;
        string outputString;
        char * outputChar;
       
        ifstream is;
        is.open (fileName.c_str(), ios::binary );

        // get length of file:
        is.seekg (0, ios::end);
        length = is.tellg();
        is.seekg (0, ios::beg);

  // allocate memory:
        outputChar= new char [length];

  // read data as a block:
        is.read (outputChar,length);
        is.close();
        outputString.assign(outputChar, length);
        return outputString;
}

---

This is to read, the next code is to write:

Code:

---

void SaveText(string fileName, string text)
{
        ofstream out(fileName.c_str(), ios::binary);
        int i = text.size();
        out.write(text.c_str(), i);
        out.close();
}

---

For both, fileName is obviously the file to read/write, and in the SaveText the string text is the info to write.
I assume that they do the same thing (hk's example), but I thought this might be useful as this is what I use for Xor encryption, to encrypt/decrypt files other than text files (such as a video or something). Hope this helps.