0

I was writing a shell program and wrote a test for conveyor. Every time I launch it, it crashes on the third vector.push_back(), not producing any exceptions and writing a lot of ununderstanable words. Please tell me what did I do wrong.

#include <stdio.h>
#include <vector>
#include "Program.cpp"
#include "Conveyor.cpp"
#include <stdlib.h>

using namespace std;

int main(){
    vector <Program> programs;
    char *argv[2];
    argv[0] = "./increaser";
    argv[1] = NULL;
    Program program1(argv[0], argv);
    Program program2(argv[0], argv);
    Program program3(argv[0], argv);

    printf("conveyor_test - PUSH 1\n");
    programs.push_back(program1);
    printf("conveyor_test - PUSH 2\n");
    programs.push_back(program2);
    printf("conveyor_test - PUSH 3\n");
    try{
        programs.push_back(program3);
        printf("conveyor_test - PUSHED 3\n");
    }
    catch (...){
        printf("Wild exception was caught.\n");
        exit(1);
    }
    printf("conveyor_test - pushed programs into vector\n");
    fflush(stdout);

    printf("---------START-----------\n");
    fflush(stdout);
    conveyor(programs);
    printf("---------END-------------\n");
    return 0;
}

This is what is written into output:

conveyor_test - PUSH 1
conveyor_test - PUSH 2
conveyor_test - PUSH 3
*** glibc detected *** ./conveyor_test: free(): invalid pointer: 0x0000000001c75040 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x7eb96)[0x7f357c463b96]
./conveyor_test[0x401806]
./conveyor_test[0x40388a]
./conveyor_test[0x4034c0]
./conveyor_test[0x402e57]
./conveyor_test[0x4024c5]
./conveyor_test[0x402718]
./conveyor_test[0x401dce]
./conveyor_test[0x40125d]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x7f357c40676d]
./conveyor_test[0x400d69]
======= Memory map: ========
00400000-00406000 r-xp 00000000 00:15 265077                             /home/crabman/Dropbox/Projects/C++/shell/conveyor_test
00605000-00606000 r--p 00005000 00:15 265077                             /home/crabman/Dropbox/Projects/C++/shell/conveyor_test
00606000-00607000 rw-p 00006000 00:15 265077                             /home/crabman/Dropbox/Projects/C++/shell/conveyor_test
01c75000-01c96000 rw-p 00000000 00:00 0                                  [heap]
7f357c0e9000-7f357c1e4000 r-xp 00000000 08:01 790941                     /lib/x86_64-linux-gnu/libm-2.15.so
7f357c1e4000-7f357c3e3000 ---p 000fb000 08:01 790941                     /lib/x86_64-linux-gnu/libm-2.15.so
7f357c3e3000-7f357c3e4000 r--p 000fa000 08:01 790941                     /lib/x86_64-linux-gnu/libm-2.15.so
7f357c3e4000-7f357c3e5000 rw-p 000fb000 08:01 790941                     /lib/x86_64-linux-gnu/libm-2.15.so
7f357c3e5000-7f357c59a000 r-xp 00000000 08:01 790899                     /lib/x86_64-linux-gnu/libc-2.15.so
7f357c59a000-7f357c799000 ---p 001b5000 08:01 790899                     /lib/x86_64-linux-gnu/libc-2.15.so
7f357c799000-7f357c79d000 r--p 001b4000 08:01 790899                     /lib/x86_64-linux-gnu/libc-2.15.so
7f357c79d000-7f357c79f000 rw-p 001b8000 08:01 790899                     /lib/x86_64-linux-gnu/libc-2.15.so
7f357c79f000-7f357c7a4000 rw-p 00000000 00:00 0 
7f357c7a4000-7f357c7b9000 r-xp 00000000 08:01 790924                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f357c7b9000-7f357c9b8000 ---p 00015000 08:01 790924                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f357c9b8000-7f357c9b9000 r--p 00014000 08:01 790924                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f357c9b9000-7f357c9ba000 rw-p 00015000 08:01 790924                     /lib/x86_64-linux-gnu/libgcc_s.so.1
7f357c9ba000-7f357ca9f000 r-xp 00000000 08:01 536749                     /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.17
7f357ca9f000-7f357cc9e000 ---p 000e5000 08:01 536749                     /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.17
7f357cc9e000-7f357cca6000 r--p 000e4000 08:01 536749                     /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.17
7f357cca6000-7f357cca8000 rw-p 000ec000 08:01 536749                     /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.17
7f357cca8000-7f357ccbd000 rw-p 00000000 00:00 0 
7f357ccbd000-7f357ccdf000 r-xp 00000000 08:01 790877                     /lib/x86_64-linux-gnu/ld-2.15.so
7f357cec2000-7f357cec7000 rw-p 00000000 00:00 0 
7f357cedb000-7f357cedf000 rw-p 00000000 00:00 0 
7f357cedf000-7f357cee0000 r--p 00022000 08:01 790877                     /lib/x86_64-linux-gnu/ld-2.15.so
7f357cee0000-7f357cee2000 rw-p 00023000 08:01 790877                     /lib/x86_64-linux-gnu/ld-2.15.so
7fff2df40000-7fff2df61000 rw-p 00000000 00:00 0                          [stack]
7fff2df6e000-7fff2df6f000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
Aborted

Here is Program class important code:

class Program{
private:
    char *path;
    int argc;
    char **argv;
public:
        Program(char *path, char **argv) {
        printf("Program::Program start\n");
        // вычисляем argc
        argc = 0;
        while (argv[argc] != NULL){
            argc++;
        }
        if (argc < 1){
            throw "Program::Program - argc < 1";
        }
        printf("\targc is calculated\n");
        // копируем path
        if (path == NULL){
            throw "Program::Program - path is NULL";
        }
        this -> path = new char[strlen(path) + 1];
        strcpy(this -> path, path);
        printf("\tpath is calculated\n");
        // копируем argv
        if (argv == NULL){
            throw "Program::Program - argv is NULL";
        }
        this -> argv = new char*[argc];
        for (int i = 0; i < argc; i++){
            this -> argv[i] = new char[strlen(argv[i]) + 1];
            strcpy(this -> argv[i], argv[i]);
        }
        printf("Program::Program end\n");
    }
    ~Program(){
        // printf("Program::~Program start\n");
        delete[] path;
        // printf("\tpath deleted\n");
        size_t size = sizeof(argv) / sizeof(argv[0]);
        // printf("\tsizeof(argv) = %d\n", size);
        for (size_t i = 0; i < size; i++){
            delete[] argv[i];
            // printf("\t\targv[%d] deleted\n", i);
        }
        // printf("\tall argv[i] deleted\n");
        delete[] argv;
        // printf("Program::~Program end\n");
    }
CrabMan
  • 1,578
  • 18
  • 37
  • 3
    Does `Program` have a pointer member that has memory allocated to it with `new` and freed with `delete`? – chris Dec 18 '12 at 05:09
  • 3
    @CrabMan Could you add the code for `Program`, esp. constructors and destructor, to the question? – jogojapan Dec 18 '12 at 05:11
  • 4
    Well, before I go to bed, here's what I'm probably going to end up saying later: *You aren't following the Rule of Three, but you should actually be using RAII instead. Read [this](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) and [this](http://dl.dropbox.com/u/6101039/Modern%20C++.pdf).* – chris Dec 18 '12 at 05:13
  • @chris Yes, it does have a pointer member, created with new and freed with delete. – CrabMan Dec 18 '12 at 06:39
  • @jogojapan I added it to the question. – CrabMan Dec 18 '12 at 06:42
  • Joachim has the real answer below. You also have a bug in your destructor. The variable size always has the value 4 (or 8 if pointers are 64-bit). This is because sizeof(a pointer) returns the size of the pointer in bytes, not what the pointer points to. – Sean Dec 18 '12 at 07:05
  • Stop using pointers. path should be `std::string` and argv (inside Program) should be `std::vector`. Then it will work correctly. – Martin York Dec 18 '12 at 07:33

4 Answers4

5

When you use push_back, what is actually put into the vector is a copy. This copy is a shallow copy, meaning the compiler just copies the pointers not what they points to.

Actually a couple of copies are made, and when one of the copies goes out of scope the copies desctructor is called which frees the memory in the pointers. But since all copies have pointers that points to the exact same memory, that memory is now marked as free so you can't access them.

There are links in the comment section of your question to something called "The Rule of Three", meaning that if you have either a destructor, a copy constructor or an assignment operator, then you should implement all three. This is to make sure that when an instance is copies you do a deep copy, copying the actual data as well.

Some programmer dude
  • 400,186
  • 35
  • 402
  • 621
  • 2
    Or, if one only has a destructor, the other two (copy & assignment) should be disabled by providing non-implemented private declarations. – Macke Dec 18 '12 at 07:12
  • The rule of three is the wrong solution to this problem. It is the reason it is crashing. But implementing the copy constructor and assignment operator is not the correct solution. A class that contains more than one owned pointer is not a good candidate for the rule of three as getting the memory management correct is extremely hard. An object doing memory management should manage a single pointer. – Martin York Dec 18 '12 at 07:50
2

I added a copying constructor and it doesn't crash anymore. Thank you all.

Program (const Program& that){

        // copying argc

        argc = that.getargc();

        // copying path

        path = new char[argc + 1];

        strcpy(path, that.getpath());

        // copying argv

        int len = 0;

        while (that.getargv()[len] != NULL){

            len++;

        }

        argv = new char*[len + 1];

        for (int i = 0; i < len; i++){

            argv[i] = new char[strlen(that.getargv()[i]) + 1];

            strcpy(argv[i], that.getargv()[i]);

        }

    }
CrabMan
  • 1,578
  • 18
  • 37
2

The problem is that you have owned pointers and don't implement the rule of three (rule of five in C++11).

Others have advised that you should implement the rule of three.
I disagree with this opinion. This is the wrong solution to this problem. An class should not contain multiple owned pointers (it is very difficult to do correctly). What you should do is use the correct types for the pointers.

In this case path should be a std::string. This is because std::string correctly handles the memory management of a string.

In this case argv (inside Program). Should be a std::vector<std::string>. This is because std::vector<> correctly handles the memory management of a dynamically sizable array. In this case each element in the array is a string (which needs to be handled separately).

Once you make these corrections the implementation of Program becomes much more trivial because you are correctly moving the memory management to classes designed specifically for this task (this is called separation of concerns: A class either handles business logic or resource management (your class Program does business logic so it should not do resource management (memory management)).

The new version of Program now is much simpler:

class Program{
private:
    std::string              path;
    int                      argc;
    std::vector<std::string> argv;
public:
    Program(char *path, char **argv)
        :path(path ? path : "")
    {
        printf("Program::Program start\n");
        // вычисляем argc
        argc = 0;
        // What happens if argv is NULL?
        while (argv[argc] != NULL){
            argc++;
        }
        if (argc < 1){
            throw "Program::Program - argc < 1";
        }
        printf("\targc is calculated\n");
        // копируем path
        if (path == NULL){
            throw "Program::Program - path is NULL";
        }
        printf("\tpath is calculated\n");
        // копируем argv
        if (argv == NULL){
            throw "Program::Program - argv is NULL";
        }
        for (int i = 0; i < argc; i++){
            this->argv.push_back() = argv[i];
        }
        printf("Program::Program end\n");
    }
    /* Don't need this
    ~Program(){
    } */
}
Martin York
  • 257,169
  • 86
  • 333
  • 562
1

add copy constructor where you allocate the memory and copy the content. Whenever you add a object in the the vector, a copy constructor will be called on the vector object. Here its a reason of crash.

CrazyC
  • 1,840
  • 6
  • 39
  • 60