Memory access violation. What's wrong with this seemingly simple program?

Question

This is a quick program I just wrote up to see if I even remembered how to start a c++ program from scratch. It's just reversing a string (in place), and looks generally correct to me. Why doesn't this work?

#include <iostream>
using namespace std;

void strReverse(char *original)
{
    char temp;
    int i;
    int j;
    for (i = 0, j = strlen(original) - 1; i < j; i++, j--)
    {
     temp = original[i];
     original[i] = original[j];
     original[j] = temp;
    }
}

void main()
{
    char *someString = "Hi there, I'm bad at this.";
    strReverse(someString);

}

Answer 1

You're trying to modify a string literal - a string allocated in static storage. That's undefiend behaviour (usually crashes the program).

You should allocate memory and copy the string literal there prior to reversing, for example:

char *someString = "Hi there, I'm bad at this.";
char* stringCopy = new char[strlen( someString ) + 1];
strcpy( stringCopy, someString );
strReverse( stringCopy );
delete[] stringCopy;//deallocate the copy when no longer needed

Answer 2

You can't change string literals (staticly allocated). To do what you want, you need to use something like:

int main()
{
    char *str = new char[a_value];
    sprintf(str, "%s", <your string here>);
    strReverse(str);
    delete [] str;
    return 0;
}

[edit] strdup also works, also strncpy... i'm sure there's a variety of other methods :)

Answer 3

See sharptooth for explanation.

Try this instead:

#include <cstring>

void main()
{
    char someString[27];
    std::strcpy( someString, "Hi there, I'm bad at this." );
    strReverse( someString );
}

Better yet, forget about char * and use <string> instead. This is C++, not C, after all.

Answer 4

If you change this, which makes someString a pointer to a read-only string literal:

char *someString = "Hi there, I'm bad at this.";

to this, which makes someString a modifiable array of char, initialized from a string literal:

char someString[] = "Hi there, I'm bad at this.";

You should have better results.

While the type of someString in the original code (char*) allows modification to the chars that it points to, because it was actually pointing at a string literal (which are not permitted to be modified) attempting to do any modification through the pointer resulted in what is technically known as undefined behaviour, which in your case was a memory access violation.

Answer 5

The line

char *someString = "Hi there, I'm bad at this.";

makes someString point to a string literal, which cannot be modified. Instead of using a raw pointer, use a character array:

char someString[] = "Hi there, I'm bad at this.";

Answer 6

When using more strict compiler settings, this code shouldn't even compile:

char* str = "Constant string";

because it should be constant:

const char* str = "Now correct";
const char str[] = "Also correct";

This allows you to catch these errors faster. Or you can just use a character array:

char str[] = "You can write to me, but don't try to write something longer!";

To be perfectly safe, just use std::string. You're using C++ after all, and raw string manipulation is extremely error-prone.

Answer 7

If this isn't homework, the C++ tag demands you do this by using the C++ standard library:

std::string s("This is easier.");
std::reverse(s.begin(), s.end());

Oh, and it's int main(), always int main(), dammit!