6

I'm relatively new too C++ programming. While I was working on a code about arguments passing with an array of character pointers. I encountered a problem where the value of my pointers are changed after certain operations. Below is my code.

#include <iostream>
using namespace std;

void input(char* argv[], int &i)
{
    char buff[10][20]; //buffer string array
    while (cin.peek() != '\n') {
        cin >> buff[i++];
    }
    for (int j = 0; j < i; j++) {
        argv[j] = buff[j];
    }
    argv[i] = NULL; // putting a NULL at the end 
}

int main(int argc, char* argv[])
{
    char *arg[10];
    int i = 0;
    input(arg, i); //input the arguments

    for (int j = 0; j < i; j++) {
        cout << arg[j] << endl;  //output the arguments entered
    }

    return 0;
}

The sub-function void input(char* argv[], int &i) is supposed to let me input my arguments as many as 9 times or when an enter key is pressed. While i indicates the total number of arguments.

The arguments are then stored as an array of character pointers and then pass it back to the main function's char *arg[10] to hold.

However, I found that after cout << arg[j] << endl; The values of arg are lost, and random values are being printed.

Jack
  • 69
  • 2
  • 9
    Use `std::string` (when possible) for your own sanity. – jweyrich Feb 19 '19 at 13:52
  • 2
    Inside the `input` function the variable `buff` is a local variable. Its life-time ends when the function ends, and in a way cease to exist. All pointers you save to that variable will be *stray* pointers and can't be dereferenced. – Some programmer dude Feb 19 '19 at 13:53
  • Apart from the lifetime of `buff` issue, you'll also have a problem if any input of more than 19 characters (without whitespace) is encountered because `operator >>` with a right hand side argument of type `char *` will then clobber the following memory. This can be prevented by setting the stream's `width()` appropriately, e.g. `cin >> std::setw(20)` before every input to `buff`. The use of `std::string` is still widely considered better, however. (Starting from C++20, the library will be able to use the deduced size of the `char` array – one less thing to worry about.) – Arne Vogel Feb 19 '19 at 16:28

3 Answers3

11

You're creating a two-dimensional array of characters buff on the stack, and then you're returning pointers into that array through the argv parameter. But buff lives on the stack and ceases to exist as soon as the input function exits. The memory used by buff will be overwritten by other functions that you call after calling input.

You should allocate buff in main and then pass it into input so it continues to live in the scope of main after input returns.

Another option would be to allocate heap space for buff in input. In this case the main function would be responsible for freeing the memory after it was done with it.

Obviously there are more advanced C++ features you could use to avoid some of this overhead. Though this is a C++ program, it's effectively written as C. But understanding how memory and pointers work is essential to understanding the problems that the newer C++ features solve.

Willis Blackburn
  • 8,068
  • 19
  • 36
  • I don't quite understand. You said: "buff ceases to exist as soon as the input function exists" From my understanding the variable `buff` lives until the input function returns? – ASMJunkie Feb 19 '19 at 14:52
  • Yes, and once the input function returns, you have an array of pointers into a buffer that no longer exists, and whose storage can be overwritten at any time. This answer uses "exits" to mean the same as "returns", ie, control _exits_ the input function and _returns_ to the caller. – Useless Feb 19 '19 at 14:55
  • @ASMJunkie I wrote that buff ceases to exist as soon as the input function *exits* (not exists). Perhaps a poor choice of words. :-) – Willis Blackburn Feb 20 '19 at 12:17
1

the value of my pointers are changed

The pointers are the only things that weren't damaged. The problem is the memory they point to.

You can prove the first part by printing the value of each of these pointers, or just inspecting them in the debugger. (You can print the address rather than the C-string it points to by casting to void, like cout << static_cast<void*>(arg[j]) << '\n').

So what happened to your C strings? Well, you declared an automatic-scope array variable inside the function input. That array ceases to exist when the function exits, just like any other automatic-scope variable. Accessing the memory where a variable used to live, after the variable ceases to exist, is illegal.

The fact that you returned pointers into this array doesn't make it legal to read through (dereference) them after the array itself goes out of scope, and this is in fact undefined behaviour.

The contents being overwritten is actually the best case, because it meant you noticed the bug: it could legally have crashed or, even worse, appeared to work flawlessly until after you submitted/deployed/sold the program, and crashed every run thereafter.

Useless
  • 64,155
  • 6
  • 88
  • 132
0

Think of the stack as being a large (but not unlimited) amount of memory. It is allocated and freed simply be moving the stack pointer down and up (the directions will depend on the hardware).

Here's your code with some annotations.

input(arg, i);
// when you get here the stack pointer will have been moved up, freeing the space
// that was allocated for 'buf' in 'input'

// the space for 'j' could overwrite the space where 'buf' was
for (int j = 0; j < i; j++) {
    // the calls to 'cout' and 'end;' could overwrite the space where 'buf was'
    cout << arg[j] << endl;
}
Paul Floyd
  • 5,530
  • 5
  • 29
  • 43