C++ Program crashes

Question

Why is the following program stuck in a following loop after "Done!", and then ends with a crash?

#include <iostream>
using namespace std;
#include <cstring>

struct stringy{
    char * str;
    int ct;
};

void set(stringy & s, const char * cs);
void show(const char * s, int times=1);
void show(const stringy & s, int times=1);

int main(){
    stringy beany;
    char testing[] = "Reality isn't what it is used to be.";
    set(beany, testing);
    show(beany);
    show(beany, 2);
    testing[0] = 'D';
    testing[1] = 'u';

    show(testing);
    show(testing, 3);
    show("Done!");

    return 0;
}

void set(stringy & s, const char * cs){
    s.ct = strlen(cs);
    strcpy(s.str, cs);
}

void show(const char * s, int times){
    while (times-- > 0){
        cout << s << endl;
    }
}

void show(const stringy & s, int times){
    show(s.str, times);
}

@ultimatebuster: By the time you get to the `show("Done!");` call, your program has already exhibitted undefined behavior. Fix the earlier errors first, before trying to tackle whatever defects there may (or may not) be in `Show()`. — John Dibling, May 13 '11 at 14:58

score 5 · Answer 1 · answered May 13 '11 at 14:46

5

You do a lot of strcpy, but where are your mallocs?!

If you use C++ you should really use std::string to avoid things like this.

answered May 13 '11 at 14:46

Bo Persson

90,663
31
146
203

I know, I'm just learning and the book's exercise is asking me specifically to use `char *` – Pwnna May 13 '11 at 14:49
There are no needs for mallocs. He's basically pointing to char arrays that are within the code-segment. – Vite Falcon May 13 '11 at 14:50
@Vita: Bo was talking about `s.str`. The pointer is uninitialized so the behavior of strcpying a buffer to it is undefined. This is very likely the reason for the crash. – Boaz Yaniv May 13 '11 at 14:53
@Ultimatebuster - Use another book, perhaps? :-) – Bo Persson May 13 '11 at 14:53
@Vite: See my answer. I point out the wild pointer explicity. – John Dibling May 13 '11 at 14:55

score 4 · Accepted Answer · answered May 13 '11 at 14:56

4

Allocate memory for string before copying

void set(stringy & s, const char * cs){
    s.ct = strlen(cs);
    s.str = new char[s.ct+1];
    strcpy(s.str, cs);
}

Free the memory

delete[] beany.str;

answered May 13 '11 at 14:56

cpx

17,009
20
87
142

Yeah i realized that and did that. Thanks – Pwnna May 13 '11 at 14:57

score 1 · Answer 3 · answered May 13 '11 at 14:50

1

A variety of problems:

struct stringy{
    char * str;
    int ct;
};

// [snip]

stringy beany;
char testing[] = "Reality isn't what it is used to be.";
set(beany, testing);

void set(stringy & s, const char * cs){
    s.ct = strlen(cs);
    strcpy(s.str, cs);
}

At the point of the call to strcpy, s.str is just a wild pointer. It doesn't point to anything valid.

This might be homework, but if not you should be using std::string instead of raw char*s. If you need to use raw char*s, then you need to allocate the space -- using new or, better yet, a smart pointer -- to which the pointers point. You can't just copy a string in to hyperspace and expect it to work.

answered May 13 '11 at 14:50

John Dibling

99,718
31
186
324

Not exactly homework, I'm learning on my own so I assigned it to myself. – Pwnna May 13 '11 at 14:52
Also, there is a grammar mistake in testing. [evil laugh] – Tom Macdonald May 13 '11 at 14:52
@ravloony: What grammar mistake? – John Dibling May 13 '11 at 14:53
@John - It is not a C++ grammar mistake, but bad English in the text that is copied. :-) – Bo Persson May 13 '11 at 14:58

score 1 · Answer 4 · answered May 13 '11 at 14:56

1

I get a segmentation fault when I run your program. You need to allocate memory for s.str before the call to strcpy.

answered May 13 '11 at 14:56

August Karlstrom

10,773
7
38
60

A different strategy is of course to use a fixed length buffer, e.g. char str[64]; – August Karlstrom May 13 '11 at 15:36

score -1 · Answer 5 · answered May 13 '11 at 14:50

-1

 void set(stringy & s, const char * cs){
     s.ct = strlen(cs);
     strcpy(s.str, cs);
 }

s.str is unallocated. You need to do a

s.str = malloc(s.ct);

answered May 13 '11 at 14:50

Tom Macdonald

6,433
7
39
59

`malloc(s.ct + 1)`, or just `strdup`. – detunized May 13 '11 at 14:52
-1: for malloc in C++ code. Use `new` and `delete`, not `malloc` and `free`. – John Dibling May 13 '11 at 14:54
On the other hand, apart form the first 3 lines, this isn't _really_ C++ at all... – Tom Macdonald May 13 '11 at 15:02

C++ Program crashes

5 Answers5