100% of array correct in function, 75% of array correct in CALLING function - C

Question

Note: i'm using the c++ compiler, hence why I can use pass by reference

i have a strange problem, and I don't really know what's going on.

Basically, I have a text file: http://pastebin.com/mCp6K3HB and I'm reading the contents of the text file in to an array of atoms:

typedef struct{
    char * name;
    char * symbol;
    int atomic_number;
    double atomic_weight;
    int electrons;
    int neutrons;
    int protons;
} atom;

this is my type definition of atom.

void set_up_temp(atom (&element_record)[DIM1])
{
    char temp_array[826][20];
    char temp2[128][20];
    int i=0;
    int j=0;
    int ctr=0;

    FILE *f=fopen("atoms.txt","r");

    for (i = 0; f && !feof(f) && i < 827; i++ ) 
    {
        fgets(temp_array[i],sizeof(temp_array[0]),f);
    }

    for (j = 0; j < 128; j++)
    {
        element_record[j].name = temp_array[ctr];
        element_record[j].symbol = temp_array[ctr+1];
        element_record[j].atomic_number = atol(temp_array[ctr+2]);
        element_record[j].atomic_weight = atol(temp_array[ctr+3]);
        element_record[j].electrons = atol(temp_array[ctr+4]);
        element_record[j].neutrons = atol(temp_array[ctr+5]);
        element_record[j].protons = atol(temp_array[ctr+6]);
        ctr = ctr + 7;
    }

    //Close the file to free up memory and prevent leaks
    fclose(f);
} //AT THIS POINT THE DATA IS FINE

Here is the function I'm using to read the data. When i debug this function, and let it run right up to the end, I use the debugger to check it's contents, and the array has 100% correct data, that is, all elements are what they should be relative to the text file. https://i.stack.imgur.com/hsMrg.png This image shows what I'm talking about. On the left, all the elements, 0, up to 127, are perfect. Then, I go down to the function I'm calling it from.

atom myAtoms[118];
set_up_temp(myAtoms); //AT THIS POINT DATA IS FINE
region current_button_pressed; // NOW IT'S BROKEN
load_font_named("arial", "cour.ttf", 20); 
panel p1 = load_panel("atomicpanel.txt");   
panel p2 = load_panel("NumberPanel.txt");

As soon as ANYTHING is called, after i call set_up_temp, the elements 103 to 127 of my array turn in to jibberish. As more things get called, EVEN MORE of the array turns to jibberish. This is weird, I don't know what's happening... Does anyone have any idea? Thanks.

`void set_up_temp(atom (&element_record)[DIM1])` is NOT valid C: the `&` is ilegal in there. — pmg, Apr 25 '13 at 17:29
@pmg sorry, i probably should have added that i'm using the c++ compiler. I've added a note at the top of the OP — Anteara, Apr 25 '13 at 17:31

zwol · Accepted Answer · 2013-04-25T17:41:13.907

for (j = 0; j < 128; j++)
{
    element_record[j].name = temp_array[ctr];

You are storing, and then returning, pointers into temp_array, which is on the stack. The moment you return from the function, all of temp_array becomes invalid -- it's undefined behavior to dereference any of those pointers after that point. "Undefined behavior" includes the possibility that you can still read elements 0 through 102 with no trouble, but 103 through 127 turn to gibberish, as you say. You need to allocate space for these strings that will live as long as the atom object. Since as you say you are using C++, the easiest fix is to change both char * members to std::string. (If you don't want to use std::string, the second easiest fix is to use strdup, but then you have to free that memory explicitly.)

This may not be the only bug in this code, but it's probably the one causing your immediate problem.

In case you're curious, the reason the high end of the data is getting corrupted is that on most (but not all) computers, including the one you're using, the stack grows downward, i.e. from high addresses to low. Arrays, however, always index from low addresses to high. So the high end of the memory area that used to be temp_array is the part that's closest to the stack pointer in the caller, and thus most likely to be overwritten by subsequent function calls.

Thanks a lot zack, I went with the strdup method, as even though I'm using the c++ compiler, the course I'm doing is related to C; we're only using the C++ compiler to allow pass by reference (since we were taught pascal first, which allows it). Is this correct usage for strdup? ` element_record[j].name = strdup(temp_array[ctr]);` `element_record[j].symbol = strdup(temp_array[ctr+1]);` This prevents the data from corrupting, but I just want to make sure it's correct usage :) — Anteara, Apr 25 '13 at 17:48
Yes, that is correct `strdup` usage. Please tell your instructor that people on the Internet think that his or her pedagogical approach is wrongheaded -- either teach proper C++ or teach proper C, don't teach some subset of C with random C++ features thrown in. — zwol, Apr 25 '13 at 19:04

Nik Bougalis · Answer 2 · 2013-04-25T17:46:37.447

3

Casual inspection yields this:

char temp_array[826][20];

...

for (i = 0; f && !feof(f) && i < 827; i++ )

Your code potentially allows i to become 826. Which means you're accessing the 827th element of temp_array. Which is one past the end. Oops.

Additionally, you are allocating an array of 118 atoms (atom myAtoms[118];) but you are setting 128 of them inside of set_up_temp in the for (j = 0; j < 128; j++) loop.

The moral of this story: Mind your indices and since you use C++ leverage things like std::vector and std::string and avoid playing with arrays directly.

Update As Zack pointed out, you're returning pointers to stack-allocated variables which will go away when the set_up_temp function returns. Additionally, the fgets you use doesn't do what you think it does and it's HORRIBLE code to begin with. Please read the documentation for fgets and ask yourself what your code does.

edited Apr 25 '13 at 17:46

answered Apr 25 '13 at 17:33

Nik Bougalis

10,495
1
21
37

Thanks for pointing out my problem with my indices. I seem to keep making this problem with a lot of my code for some reason. I really need to pay more attention to them. – Anteara Apr 25 '13 at 17:41
Also, I was initially using `(!feof(f))` but as per my question here: http://stackoverflow.com/questions/16196965/function-to-change-array-data-data-not-changing-c It was changed, by the poster of my accepted answer to: `"Note that in the solution I proposed, I added a test for the row exceeding DIM1 -- that prevents the condition of trying to read an extra block from occurring."` However, I will read up on fgets and thanks for pointing it out. – Anteara Apr 25 '13 at 17:56
@Anteara it might help to have a file with only *one* element, and create a function that reads that element and populates *one* atom structure, that you can then display (or view in the debugger) to verify it was read back correctly. – Nik Bougalis Apr 25 '13 at 18:00
I reworked my code, that part that you said was bad code. I don't have the option of using `string::getline` as I'm not allowed using string. This is what i came up with: `while (fscanf(f, "%s", temp_array[i]) != EOF)` and inside i do `i++;`. I wasn't able to get fscanf working with a for loop. Is this any better code, though, or is it still terrible? – Anteara Apr 26 '13 at 09:29

score 2 · Answer 3 · edited May 23 '17 at 12:28

2

You are allocating an array with space for 118 elements but the code sets 128 of them, thus overwriting whatever happens to live right after the array.

Also as other noted you're storing in the array pointers to data that is temporary to the function (a no-no).

My suggestion is to start by reading a good book about C++ before programming because otherwise you're making your life harder for no reason. C++ is not a language in which you can hope to make serious progress by experimentation.

edited May 23 '17 at 12:28

Community

1
1

answered Apr 25 '13 at 17:34

6502

112,025
15
165
265

Hi, thanks for the suggestion. I seem to have a problem with keeping my indices consistent. – Anteara Apr 25 '13 at 17:49
@Anteara: The reason C++ cannot be learned by experimentation is that mistakes are very often punished with undefined behaviour, not runtime errors. Moreover in some parts the language is somewhat "illogical" for historical reasons so guessing sometimes will not take you to the correct path. The only sensible way to learn C++ is by reading good books. – 6502 Apr 25 '13 at 18:13
thanks, I mistakenly thought that passing it by reference would mean everything would be 'remembered' by the caller. Also, I'm doing a bachelors of computer science degree right now, but we've been learning pascal, and in a week or two we'll begin C. I may invest in the book you suggested, as I do plan on doing C++ after C. Thanks. – Anteara Apr 26 '13 at 03:54

100% of array correct in function, 75% of array correct in CALLING function - C

3 Answers3