C++ - printing objects in statistically allocated array causes segmentation fault

Question

So I'm creating a program that implements several classes representing a school, and its students and courses. I'm getting a segmentation fault when I try to prints out all the Taken objects in the studentCoursePairs[] array which represents Student objects taking a particular Course. I think my segmentation fault comes from the addTaken() function in School.cc where its job is to find the student object and course object with the given student number and course id, and then creates a new Taken object with the found student and course objects as well as a grade. I then try to add this new object to the back of the Taken collection which is studentCoursePairs.

When I comment out studentCoursePairs[i]->print() the segmentation fault goes away. I'm not exactly sure what I'm doing wrong and would appreciate some help.

I'm not sure if the other classes besides School.cc are needed but I included them anyways to help with understanding.

School.cc:

#include <iostream>
#include <iomanip>
using namespace std;
#include <string.h> 

#include "School.h"

School::School(string s1) : name(s1){ 
    numTaken = 0;
}

void School::addTaken(string number, int code, string grade){
    Student* s = nullptr;
    Course* c = nullptr;
    for(int i = 0; i < numTaken; ++i){
        if((studentsCollection->find(number, &s)) && (coursesCollection->find(code, &c))){
        Taken* taken = new Taken(s, c, grade);
          studentCoursePairs[i] = taken;            
          ++numTaken;
        }
    }
}

void School::printTaken(){
    cout << name << " === TAKEN: "<< endl;
    for(int i = 0; i < sizeof(studentCoursePairs)/sizeof(studentCoursePairs[0]); ++i){
        studentCoursePairs[i]->print(); //seg fault
    }   
}

Additional files:

StudentCollection.cc

bool StudentCollection::find(string num, Student** s){
    for(int i = 0; i < size; ++i){
        if(students[i]->getNumber() == num){ //find student number
            *s = students[i];
        }
    }
}

CoursesCollection.cc

bool CoursesCollection::find(int id, Course** c){
    for(int i = 0; i < numCourses; ++i){
        if(courses[i]->getId() == id){ //find course id
            *c = courses[i];
        }
    }
}

I also have a Student class and Course class which just declare and initializes information like the name, program, gpa of a student as well as the course code, instructor, name, year of a course.

Don't guess, instead run your program under a debugger and study the program state at the point of the crash. — Botje, Oct 15 '20 at 07:51
Now that I have read your code, try to answer the following question: what is in `studentCoursePairs[10]` if there are only 3 student--course pairs? What is the effect on the `studentCoursePairs[10]->print()` call? — Botje, Oct 15 '20 at 07:54
Why are you using owning pointers and `new` for everything? Use values and collections by default and smart pointers if necessary. — Quimby, Oct 15 '20 at 07:54
The for loop is there to only print the number of elements that the studentCoursePairs array has. So if there are only 3 pairs then sizeof(studentCoursePairs)/sizeof(studentCoursePairs[0]) wouldn't let it print past the first 3 pairs. — Nicepeach, Oct 15 '20 at 07:56
That is not how arrays work. You declared it as `Taken* studentCoursePairs[MAX_PAIRS];` so that expression will *always* return `MAX_PAIRS`. You will need another way to track the number of slots taken, OR a way to mark slots as empty (and then check for that marker as you walk the array). — Botje, Oct 15 '20 at 07:57
Unrelated: The class definitions in your header files depends on that `using namespace std;` has been done. This requires a user of your classes to do `using namespace std;` which is [considered bad practice](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice). Spell out `std::` in your header files. It doesn't cost you that much. — Ted Lyngmo, Oct 15 '20 at 07:58
Urelated 2: Why use the C header `string.h` (`cstring`)? It doesn't provide the definition for the `std::string` class you are using. Include `string` instead. It's a totally different header file. — Ted Lyngmo, Oct 15 '20 at 08:01
Unrelated 3: Use [`vector`](https://en.cppreference.com/w/cpp/container/vector), [`set`](https://en.cppreference.com/w/cpp/container/set) and [`map`](https://en.cppreference.com/w/cpp/container/map) to simplify your code. — Ted Lyngmo, Oct 15 '20 at 08:04
`Course* courses = new Course();` -- This is a memory leak. Why are you resorting to using pointers and `new`, when there are container classes that are designed for these things? It looks like the way C++ is being taught is to slap together anything using pointers and `new` and hope that it doesn't crash, memory leaks and all. — PaulMcKenzie, Oct 15 '20 at 08:06
I agree that OP should have stated this up front, but let's assume this is homework and they either they do not know these things yet or are not allowed to use them. — Botje, Oct 15 '20 at 08:07
Your `find` functions always return on the first iteration of the loop. — molbdnilo, Oct 15 '20 at 09:34

Botje · Accepted Answer · 2020-10-15T08:21:47.550

Your School object has two major problems. Let us start with the one you posted in your question:

void School::printTaken(){
    cout << name << " === TAKEN: "<< endl;
    for(int i = 0; i < sizeof(studentCoursePairs)/sizeof(studentCoursePairs[0]); ++i){
        studentCoursePairs[i]->print(); //seg fault
    }   
}

This for loop will always run exactly MAX_PAIRS times, as this variable was defined as

Taken* studentCoursePairs[MAX_PAIRS];

so sizeof(studentCoursePairs) === MAX_PAIRS * sizeof(studentCoursePairs[0]).

Instead, you want to loop only over the first few slots that actually contain valid pointers. You have a variable for that: numTaken. So change the condition to i < numTaken and your print loop will work.

The second major problem is in addTaken:

void School::addTaken(string number, int code, string grade){
    Student* s = nullptr;
    Course* c = nullptr;
    for(int i = 0; i < numTaken; ++i){
        if((studentsCollection->find(number, &s)) && (coursesCollection->find(code, &c))){
        Taken* taken = new Taken(s, c, grade);
          studentCoursePairs[i] = taken;            
          ++numTaken;
        }
    }
}

Let us play computer and work out what happens if the passed in number and code are valid:

If numTaken is 0, the loop immediately stops (as 0 < 0 is false) and numTaken is not incremented. You can call addTaken as much as you want, it will never change numTaken
Assuming you fix that, let us assume numTaken = 5. On the first iteration, you check the condition and agree this is a valid number-code combination. Thus, you create a new Taken object and .. overwrite studentCoursePairs[0] with the new object. On the second iteration you do the same and overwrite studentCoursePairs[1] with an equivalent object.

That is probably not the intended behavior. Instead, you probably want to place a new object in studentCoursePairs[numTaken] and bump numTaken:

void School::addTaken(string number, int code, string grade){
    Student* s = nullptr;
    Course* c = nullptr;
    if((studentsCollection->find(number, &s)) && (coursesCollection->find(code, &c))){
        Taken* taken = new Taken(s, c, grade);
        studentCoursePairs[numTaken] = taken;            
        ++numTaken;
    }
}

Figuring out how to handle the case where the passed combination is NOT valid or when you exceed MAX_PAIRS combinations is left as an exercise to you.

EDIT: There is a third major problem in your CoursesCollection: you allocate space for one object new Course() while you treat it as an array, and you store the result in a local variable instead of a member. Your constructor should probably look like:

CoursesCollection::CoursesCollection(){
    courses = new Course*[MAX_COURSES];
    numCourses = 0;
}

or, using a member initializer list:

CoursesCollection::CoursesCollection() 
  : courses(new Course*[MAX_COURSES]), numCourses(0) {}

`Course* courses = new Course();` -- Did you forget this obvious issue? — PaulMcKenzie, Oct 15 '20 at 08:11
I was writing my answer while the comments were spotting other problems. — Botje, Oct 15 '20 at 08:13
And of course, the other major problem is that all of these objects leak memory. — PaulMcKenzie, Oct 15 '20 at 08:19
Hi! So I edited the add and print functions in School.cc but for some reason numTaken never gets incremented. The value always remains 0 so no Taken objects actually get printed. — Nicepeach, Oct 15 '20 at 08:19
Run your code under a debugger and put a breakpoint on addTaken, and step through it. Once you verify that `addTaken` properly increments `numTaken`, check that you're actually calling add and print on the *same* School object. — Botje, Oct 15 '20 at 08:21

C++ - printing objects in statistically allocated array causes segmentation fault

1 Answers1