Coding own shell, error "exec: bad adress"-CodePudding

I'm trying to code my own shell in c and have stumbled upon an error, I don't know how to fix. You have to type in a command in the terminal, most of them work as well, but if I try to include more than one argument or spaces between letters (example: echo 1 2 3) the shell says "exec: bad adress". I'm coding the shell on kali linux in a virtual machine and use g to compile the programm, I've tried using cpp, but when I try to execute the file, it says "namespace: not found". Help would be greatly appreciated.

#include <iostream>
#include <stdio.h>
#include <unistd.h>
#include <sys/wait.h>
#include <sys/types.h>
#include <stdlib.h>
#include <string>
#include <vector>
#include <regex>

bool handleLogout()
{
    std::cout<<"Are you sure you want to log out?<y/n>"<<std::endl;
    char eingabe;
    std::cin>>eingabe;
    if(eingabe=='y'){
        std::cout << "Bye";
        exit(0);
        return true;
    }else {
        return false;
    }
}

void processArgs(std::string command, std::string delimiter, std::vector<std::string> &argsBefore)
{
    size_t pos =0;
    std::string teilWort;
    while((pos= command.find(delimiter))!=std::string::npos){
        teilWort=command.substr(0,pos);
        command.erase(0,pos delimiter.length());
        argsBefore.push_back(teilWort);
    }
    argsBefore.push_back(command);
}

int main()
{
    while(1){

        while(waitpid(-1, 0, WNOHANG) > 0) {}

        bool executeInBackground = false; //checks if command should be activated in background
        std::vector<std::string> argsBefore;
        std::string command;
        std::string delimiter=(" "); //used to seperate the different arguments

        std::cout << getlogin() << ":$ ";
        std::getline(std::cin, command);


        if(command.back() == '&') {
            executeInBackground = true;
            command.replace(command.length() - 1, 1, "");
            command = std::regex_replace(command, std::regex("  $"), ""); //trim trailing spaces
        }

        processArgs(command, delimiter, argsBefore);

        char* args[argsBefore.size()];

        for(size_t i =0; i<=(argsBefore.size());i  ){
            args[i]=(char*)argsBefore[i].c_str();
        }

        args[sizeof(args) 1]=NULL; // null terminated array for execvp

        if(command=="logout"){
            if(handleLogout())
                break;
            else
                continue;
        }

        pid_t pid = fork();
        pid_t w_pid;

        // Error
        if(pid<0){
            std::cout<<"Error, fork failed!"<<std::endl;
            return 1;
        }
        // Child
        if(pid==0){
            if(execvp(args[0], args)==-1){
                perror("exec");
                exit(0);
            }

        }
        // Parent
        if(pid>0){
            if(executeInBackground==true){
                std::cout<<"Child spwan with PID: " << pid << std::endl;
                continue;
            }

            w_pid = waitpid(pid, 0, WUNTRACED | WCONTINUED);

            if(w_pid == -1){
                perror ("Wait");
            }
        }
    }
    return 0;
}

CodePudding user response：

    char* args[argsBefore.size()];

    for(size_t i =0; i<=(argsBefore.size());i  ){
        args[i]=(char*)argsBefore[i].c_str();
    }
    args[sizeof(args) 1]=NULL;

Firstly char* args[argsBefore.size()]; is not legal C (but g will accept it). In C array sizes must be compile time constants.

More importantly you have an array out of bound access. i<=(argsBefore.size()) should be i < argsBefore.size(). Array indexes go up to but do not include the array size. So use < not <=.

Also your attempt to null terminate the args array is incorrect. To do that you need to make the array one bigger.

char* args[argsBefore.size()   1]; //  1 for NULL terminator

And then you can add the NULL like this

args[argsBefore.size()] = NULL;

CodePudding user response：

You access argsBefore out of bounds:

i <= (argsBefore.size())

should be

i < (argsBefore.size())

You also have this out of bounds (by 1 element 1 byte) assignment:

args[sizeof(args)   1] = NULL;

You should have made the array big enough to start with and then have assigned the last element args[sizeof(args) - sizeof(char*)] = NULL;.

There's also no need to cast from const char* to char*. In C 17, you can simply do:

args[i] = argsBefore[i].data();

Prior to C 17:

args[i] = &argsBefore[i][0];

I also suggest that you make args a vector<char*> instead of using a variable length array:

std::vector<char*> args(argsBefore.size()   1); //  1 for `NULL`

// this is redundant but shows intent:
args.back() = nullptr; // or NULL

Then

if(execvp(args[0], args.data()) == -1)