Stack around the variable is corrupted c++

Question

Contact::Contact(const char* name2, const long long * phone2, int num2) {

    bool safe = name2== nullptr && phone2 == nullptr && num2 == 0;

    if (safe) {

        *this = Contact();

    }
    else {
        strcpy(name, name2);
        name[19] = '\0';
        bool valid = phone2 != nullptr && num2 > 0;
        if (valid) {
            int count = 0;

            for (int i = 0; i < num2; i++) {
                valid = phone2[i] > 10000000000LL && phone2[i] < 999999999999LL;  // PROBABLY A GOOD IDEA TO MAKE A FUNCTION TO CHECK VALIDNESS
                if (valid) count++;
            }

            num = count;
            phone = new long long[num];

            for (int i = 0, j = 0; i < num2; i++) {
                if (phone2[i] > 10000000000LL && phone2[i] < 999999999999LL) {
                    phone[j] = phone2[i];
                    j++;
                }
            }
        }
        else {
            num = 0;
            phone = nullptr;
        }
    }

}

This is my 3 arguments constructor. I keep getting the error message that stack around the variable is corrupted. But when I get rid of the name2==nullptr in line3, it works without the error (though the output is not exactly as what i want). What I am doing wrong there?

Read [How to Debug Small Programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). Compile with all warnings and debug info: `g++ -Wall -Wextra -g` with [GCC](http://gcc.gnu.org/). Then [use the `gdb` debugger](https://sourceware.org/gdb/current/onlinedocs/gdb/) and [valgrind](http://valgrind.org/). Your question don't have any [MCVE] so we cannot reproduce your problem — Basile Starynkevitch, Jul 01 '18 at 04:48
Stack Overflow isn't a free debugging service, and you should show your attempts at debugging the code with a debugger or other simpler methods such as debug print statements. You can also test each part of the code separately to figure out exactly which part of the code is causing the problem, and make a [mcve]. This won't be the only time you end up with a bug in your code, and learning to debug your programs will help you much more than having someone find the bug for you. http://idownvotedbecau.se/nodebugging/ — eesiraed, Jul 01 '18 at 04:53
Read also about the [rule of five](https://en.cppreference.com/w/cpp/language/rule_of_three). It might be relevant (but since we don't have any [MCVE], we can't be sure) — Basile Starynkevitch, Jul 01 '18 at 05:10
`strcpy(name, name2);name[19] = '\0';` -- Why are you not using `std::string`? It's code like this that can cause "stack around variable is corrupted" errors. — PaulMcKenzie, Jul 01 '18 at 05:11
What on earth is `*this = Contact();` supposed to do? You can't do stuff like that! — Paul Sanders, Jul 01 '18 at 05:48
@KillzoneKid - Well, for one, if one wants to delegate to an existing constructor, it's better to use the syntax for delegation. I think that's what the OP was *trying* to do anyway. — StoryTeller - Unslander Monica, Jul 01 '18 at 06:18
@StoryTeller I agree, this is not the way of using delegated constructor, but it doesn't mean one cannot do it. — Killzone Kid, Jul 01 '18 at 06:27
@KillzoneKid - I'm not a native English speaker, but I think "you can't do stuff like that!" is rhetorical device for "you shouldn't be doing stuff like that!". It's not a statement of ability. — StoryTeller - Unslander Monica, Jul 01 '18 at 06:30
@StoryTeller Yes, that's how I intended it. Just a friendly nudge in the right direction. — Paul Sanders, Jul 01 '18 at 08:20
Wow, how did this ever get voted up? OP, @Basile asked for a [MCVE] for a reason. Please include the declaration of the class (so that we can see how `name` is declared) and the calling code that triggers this error. Also, as others have commented, your code has many shortcomings by modern standards, you need to do some background reading, see here: https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list. — Paul Sanders, Jul 01 '18 at 09:20
I am a newbie. I did not know the rules fully, sorry about the problem and thank you all for the comment! — john316, Jul 01 '18 at 14:28

score 0 · Answer 1 · answered Jul 01 '18 at 08:21

0

Let's assume that name2 == nullptr, but phone2 != nullptr. That means that save will be false and the strcpy(name, name2) will be executed, which will try to copy from a nullptr. Depending on the OS this might give you an access violation (read access to address 0 is prohibited) or it might not. My guess is, read access is allowed.

So strcpy will try to copy a totally bogus string at address 0. It will copy char until it encounters a \0 char.

This will in almost all cases be longer than your member variable name, overwriting the memory there.

If you change save = stuff && stuff to save = name2==nullptr || phone2==nullptr the error will probably go away.

But your code is very bad (meaning unsafe, error prone) in general.

You should not use strcpy in 2018 anymore. Use std::string instead. Using *this=Contact() is probably erroneous. You are calling the assignment operator here (operator=) before your object has been constructed fully.

answered Jul 01 '18 at 08:21

Hajo Kirchhoff

1,969
8
17

It's indeed defined behavior, I rather initialize my members in the header so you simply do 'return' – JVApen Jul 01 '18 at 12:37
But is `*this = Contact()` okay when called *inside* the constructor? I don't think so, but the standard may say otherwise. But the object is not fully constructed yet, so it might not be safe to call the `operator=`. – Hajo Kirchhoff Jul 01 '18 at 18:45
Imagine an object with a pointer variable. The `operator=` might release the current pointer and create a deep copy of the other pointer. But the current pointer might not be initialized yet when the call to `operator=` happens. That is why I think that `*this=Contact()` is wrong when called *inside* a constructor. – Hajo Kirchhoff Jul 01 '18 at 18:46

score 0 · Accepted Answer · answered Jul 01 '18 at 12:30

Removing the condition name2== nullptr makes the code go into the safe path where you get a default constructed Contact even if name2 != nullptr.

if name2 != nullptr then you with name2 == nullptr go into the the else part, where you have this code

    strcpy(name, name2);
    name[19] = '\0';

I presume name is

    constexpr int maxNameLength = 20;
    char name[maxNameLength]; // if this is just a pointer you got other problems.

then the code should be

    strncpy(maxNameLength, name, name2);
    name[maxNameLength-1] = '\0';

After I started using std::string, std::vector and std::array I had far fewer problems.

Without seeing the rest of the code I would guess that your class is using pointers and therefore needs copy constructor and copy assignment (and the move variant).

Stack around the variable is corrupted c++

2 Answers2