-2

I have an unused variable in my function. If I remove it, the program crashes. I'm having trouble understanding why. I assume its because I'm accessing out of bounds for dataArr.

For definitions: userFile is an argv input text file to be read dataArr is the lines of that text file, stored into a string array. n[80] was a previously used array of pointers that stored individual values (wrote it into a different function) strdup duplicates the string (non-standard library)

If it helps, switching the value of n[80] to n[20] gives errors, but n[21] does not.

char ** read_file(char * userFile)
{
    char * n[80];  // DO NOT REMOVE THIS LINE UNDER ANY CIRCUMSTANCES.  IT IS BLACK VOODOO MAGIC.
    char currLine[256];
    char * dataArr[80];
    char * eof;
    int lineCount = 0;

    int i = 0;
    int j = 0;

    FILE * fp;
    fp = fopen(userFile, "r");

    while((eof = fgets(currLine, sizeof(currLine), fp)) != NULL)
        /* Stores the file into an array */
    {
        if (fp == NULL)
        {
            fprintf(stderr, "Can't open input file in.list!\n");
            exit(1);
        }
        dataArr[i] = strdup(eof);
        i++;
    }

    return dataArr;
}

EDIT:

I call the function using 

    dataArr = read_file(argv[1]);

I have a bad habit of using the same variable name for functions.

Alex Wittwer
  • 33
  • 4
  • Where exactly does the program crash? – Angew is no longer proud of SO May 29 '18 at 09:07
  • It hangs up whenever I try to call the function. After a few seconds, it returns: – Alex Wittwer May 29 '18 at 09:08
  • 8
    `char * dataArr[80]; ..... return dataArr;` This is a local variable that gets invalid after leaving the function. Returning the address of that array will cause lots of trouble. – Gerhardh May 29 '18 at 09:08
  • "whenever I call the function" is not a good description. Be more precise and provide a [mcve] – klutt May 29 '18 at 09:09
  • 2
    *IT IS BLACK VOODOO MAGIC.* No, it's undefined behavior caused by returning the address of a local variable. – Andrew Henle May 29 '18 at 09:09
  • 1
    `if (fp == NULL)` You check for `NULL` *after* you used it for reading. – Gerhardh May 29 '18 at 09:10
  • I love black voodoo magic in C code. Fully understanding what is behind it is the best way to master C language. – mouviciel May 29 '18 at 09:14
  • @AlexWittwer: Despite the answers, I am still interested in seeing how you called this function. Can you amend your question? – andreee May 29 '18 at 09:16
  • Done. Also, using static fixed it, but its bad practice or so I'm told. – Alex Wittwer May 29 '18 at 09:21
  • Please [edit] your question to show us what kind of debugging you've done. I expect you to have run your [mcve] within Valgrind or a similar checker, and to have investigated with a debugger such as GDB, for example. Ensure you've enabled a full set of compiler warnings, too. What did the tools tell you, and what information are they missing? And read Eric Lippert's [How to debug small programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). – Toby Speight May 29 '18 at 10:50

2 Answers2

5

This happens because the array allocates memory and modifies the way your program is stored.

You cause Undefined Behavior when you do:

return dataArr;

since this array is a local variable:

char * dataArr[80];

thus it will go out of scope when the function terminates. That means that when the caller tries to use it, it will be most likely have gone out of scope.

By the way, you first read the file and then check if it opened. You should it like this instead:

fp = fopen(userFile, "r");
if (fp == NULL)
{
    fprintf(stderr, "Can't open input file in.list!\n");
     exit(1);
}
while((eof = fgets(currLine, sizeof(currLine), fp)) != NULL) {
   ...
gsamaras
  • 71,951
  • 46
  • 188
  • 305
  • Good catch. I remember changing it a while back, and then losing that revision in a ctrl+z spree. Would changing the variable char * dataArr[80] to static char * dataArr[80] be good practice? – Alex Wittwer May 29 '18 at 09:16
  • _"[...] changing it a while back, and then losing that revision in a ctrl+z spree."_ - You should start using [git](https://git-scm.com/) :-) – andreee May 29 '18 at 09:17
  • 2
    No @AlexWittwer. You need to dynamically allocate the array (see my [example](https://gsamaras.wordpress.com/code/dynamic-array-in-c/)). That way the array will be valid even when the function terminates. It will go out of scope, only when you de-allocate the memory. You *must* free the memory by the way, do not forget that. – gsamaras May 29 '18 at 09:18
  • What do you mean "git" ? – Alex Wittwer May 29 '18 at 09:19
  • @AlexWittwer [Git](https://git-scm.com/) is the most commonly-used version control system. Saving versions of your source code helps to prevent damage that can be caused by ctrl-z sprees. If your code gets screwed up beyond recognition, just roll back to a previous version :) – Michael Dodd May 29 '18 at 09:48
  • @AlexWittwer is there a reason for not accepting my answer? :/ – gsamaras May 30 '18 at 12:52
3

This line:

return dataArr;

will cause undefined behaviour which is among the worst problems you can face. The reason why this is so bad is that very often it is hard to pinpoint. UD very often manifests itself in strange ways like this.

This has absolutely nothing to do with char * n[80] at all.

In short, UD means that anything can happen. See this link for more info: Undefined, unspecified and implementation-defined behavior

klutt
  • 30,332
  • 17
  • 55
  • 95