0

I have this function which executes a command through a ssh channel:

std::string ssh::exec_ssh_command(const char* command)
{
    std::string receive = "";
    std::string err;
    int rc, nbytes;
    char buffer[2000];
    MyException errMsg;
    try {
        ssh_channel channel = ssh_channel_new(my_ssh_session);
        if (channel == NULL)
        {
            receive = "Channel allocation failed.";
            throw MyException(receive);
        }
        rc = ssh_channel_open_session(channel);
        if (rc != SSH_OK)
        {
            receive = "Opening session channel failed : ";
            receive += ssh_get_error(my_ssh_session);

            throw MyException(receive);
        }

        rc = ssh_channel_request_exec(channel, command);
        if (rc != SSH_OK) {
            receive = "Channel's request executing failed.";
            throw MyException(receive);
        }
        nbytes = ssh_channel_read(channel, buffer, sizeof(buffer), 0);
        receive = buffer;
        if (nbytes > 0)
        {
            receive.erase(nbytes - 1, 2000);
        }
        else
        {
            receive = "Error in command: not found or wrong syntax";
            throw MyException(receive);
        }

        if (nbytes < 0)
        {
            receive = "Error in reading data from channel ";
            throw MyException(receive);
        }

        ssh_channel_free(channel);
    }
    catch (MyException& err)
    {
        ssh_channel_free(channel);

        throw err;

    }
    return receive;
}

I used this function to run commands before and get a single output. now I want to use this function in a loop, like below:

Service monitoring::osFields()
{
    std::string num;
    int serviceNumbers, active, faild, loaded, not_found;
    serviceNumbers = getServiceNumbers();
    Service *services = new Service[serviceNumbers];

    std::string service_name_command;
    std::string service_load_state_commands;
    std::string service_active_state_commands;
    std::string service_sub_state_commands;
    try
    {
        num = sshObj->exec_ssh_command("systemctl list-units --state active | grep service | wc -l");
        active = std::stoi(num);
        for (int i = 0; i < active; i++)
        {
            service_name_command = "systemctl list-units --state active | grep service | sed -s -n " + std::to_string(i+1) + "p | awk '{print $1}'";
            services[i].name = sshObj->exec_ssh_command(service_name_command);

            service_load_state_commands = "systemctl list-units --state active | grep service | sed -s -n " + std::to_string(i+1) + "p | awk '{print $2}'";
            services[i].load = sshObj->exec_ssh_command(service_load_state_commands);

            service_active_state_commands = "systemctl list-units --state active | grep service | sed -s -n " + std::to_string(i + 1) + "p | awk '{print $3}'";
            services[i].active = sshObj->exec_ssh_command(service_active_state_commands);

            service_sub_state_commands = "systemctl list-units --state active | grep service | sed -s -n " + std::to_string(i + 1) + "p | awk '{print $4}'";
            services[i].sub = sshObj->exec_ssh_command(service_sub_state_commands);

        }
        
    }
    catch (MyException& err)
    {
        throw MyException("in function smc_getNICs ::" + string(err.what()));
    }
    return *services;
}

getServiceNumbers(); is a function to count service numbers.
and this is Service structure:

struct Service {
    const char* name;
    const char* load;
    const char* active;
    const char* sub;
};

I have a connectSession function which is called in constructor of ssh class and makes a ssh session. whenever I run my code, it crashes in i=43. it is about memory leak and mostly stops in ssh_channel_open_session function, but sometime on ssh_channel_request_exec.
EDIT:
exec_ssh_comman overload:


const char * ssh::exec_ssh_command(std::string command)
{
    const char* _retVal = NULL;

    _retVal = stringToConstChar(exec_ssh_command(command.c_str()));

    return _retVal;
}

stringToCharfunction:

const char* stringToConstChar(string str)
{
    const char* command = new char[str.size()];
    strcpy((char*)command, str.c_str());
    const char* cm = command;
    return cm;
}
fa7eme
  • 73
  • 10
  • With which compiler do you compile that code? `services[i].name = sshObj->exec_ssh_command` should not compile, because it tries to convert `std::string` to `const char *`. `osFields` leaks `new Service[serviceNumbers]` because there is no corresponding `delete[]`. And using expectations as conctol flow (`exec_ssh_command`) is cenearlly a bad idea. – t.niese Aug 23 '20 at 11:38
  • I am using visual studio. sorry I didn't mention that I have made overload from `exec_ssh_channel` and there is another function with this name and `const char*` output. about deleting `services` object, where should I delete that ? @t.niese – fa7eme Aug 23 '20 at 11:51
  • `I have made overload from exec_ssh_channel and there is another function with this name and const char*` I really doubt that you implemented that one correctly and you leak in that function memory too, or you point to memory that is already freed. – t.niese Aug 23 '20 at 12:04
  • I need those pointers anyway, but I don't know where to delete them to prevent memory leaking and also get correct result. about char buffer, I didn't have any problem with that really yet, what is wrong with it? @SamVarshavchik – fa7eme Aug 23 '20 at 12:30
  • Is there any particular reason why you use `const char*` instead of `std::string` and `Service *services = new Service[serviceNumbers];` instead of `std::vector`. `std::string` and `std::vector` (and all other types of the std library) exits for a reason and one of them is to minimize the risk of memory leaks. – t.niese Aug 23 '20 at 12:33
  • yes, I want to use output of this code in c program and make a library. @t.niese – fa7eme Aug 23 '20 at 12:46

1 Answers1

0
const char* command = new char[str.size()];
strcpy((char*)command, str.c_str());
const char* cm = command;
return cm;

There's your memory leak. This newed char buffer does not get deleted anywhere.

Additionally, the char buffer is too small, and this results in memory corruption as well. Unless fixed, this program will start randomly crashing, in random, unrelated places as you continue to develop and/or run this code repeatedly.

I don't see anything that's actually accomplished here by doing all of these complicated and elaborate steps of copying the contents of a std::string into a separate char buffer (that's too small), and then leaking it. Looks like the only thing this const char * is used for is constructing a completely separate and independent std::string, at which point the memory gets leaked.

The simplest fix is to just get rid of all of this, completely. This should just return the original std::string. In general, modern C++ code rarely needs to new or delete anything, and this is rarely seen in modern, clean C++ code which uses objects like std::string and various containers to correctly handle all memory allocations and deallocations. The easiest way to avoid bugs related to memory allocations is to simply not do it yourself, and let the C++ library manage memory for you.

Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
  • I changed line which calls `stringToChar` to this: _retVal =(exec_ssh_command(command.c_str())).c_str(); .. but it crashes on `i=2`. – fa7eme Aug 23 '20 at 12:45
  • 1
    That's unfortunate, especially since `i=2` does not appear anywhere in the shown code, and you can't expect anyone here to help you with crashes in code that's not even shown. On stackoverflow.com, all question of this kind must meet all requirements of a [mre]. The above answers the question why the shown code was leaking memory. If now the code crashes, it would be a different question and you should post a new question, that must meet all requirements for a [mre]. See [ask] for more information. – Sam Varshavchik Aug 23 '20 at 12:48
  • by `i=2` I meant when for loop comes to `i=2`. – fa7eme Aug 23 '20 at 12:50
  • Most likely because the number of `new`ed services is less than 2. Your debugger will show what is the value of the `serviceNumber` variable that `new`ed it. That's another problem because of completely unnecessary use of `new` and `delete`. Get rid of them, and replace them with `std::vector`s and `std::list`s. In any case, whatever the reason for the crash is, your debugger should be able to show you all the information needed to determine the reason for the crash. – Sam Varshavchik Aug 23 '20 at 12:52
  • I want to use this program to make a shared library and use it in c :( . I cannot use `vector ` or `std::string` or .. – fa7eme Aug 23 '20 at 12:54
  • So? It is perfectly possible to write C++ code that's callable from C, but uses the C++ library. This is not very common, but there are libraries like that. You do understant that `new` and `delete` are C++ library functions, so your shared library already uses C++-only code, and you're calling it from C. – Sam Varshavchik Aug 23 '20 at 12:56
  • thank you so much. it somehow solved my problem, with some other changes. – fa7eme Aug 24 '20 at 06:12