1

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.

4
  • void set_up_temp(atom (&element_record)[DIM1]) is NOT valid C: the & is ilegal in there.
    – pmg
    Apr 25, 2013 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, 2013 at 17:31
  • 2
    feof probably doesn't work like you think it does.
    – Carl Norum
    Apr 25, 2013 at 17:44
  • @CarlNorum or, for that matter, fgets Apr 25, 2013 at 17:48

3 Answers 3

7
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.

2
  • 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, 2013 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, 2013 at 19:04
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.

4
  • 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, 2013 at 17:41
  • Also, I was initially using (!feof(f)) but as per my question here: stackoverflow.com/questions/16196965/… 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, 2013 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. Apr 25, 2013 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, 2013 at 9:29
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.

3
  • Hi, thanks for the suggestion. I seem to have a problem with keeping my indices consistent.
    – Anteara
    Apr 25, 2013 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, 2013 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, 2013 at 3:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.