Segmentation fault help

[daveed]

I just got my code to compile, but got hit with the segmentation fault. I am just helpless trying to find what is wrong, and messing up my code even more.
Please help

Book.cpp is a program that will read the components of a book from a text file, and Warehouse.cpp is a program that will store Books in a linked list manner.

Book.cpp

  #include "Book.h"
#include <iostream>
#include <string>
#include<cstring>

using namespace std;

Book :: Book() {}

Book :: Book(string title, string authors[], int authorCount, string publisher, short yearPublish, bool hardcover, float price, string isbn, long copies){
    this->title = title;
    this ->authorCount = authorCount;
    this ->authors[MAX_AUTHORS-1] = authors[MAX_AUTHORS];
    this -> publisher = publisher;
    this -> yearPublish = yearPublish;
    this-> hardcover = hardcover;
    this-> price = price;
    this-> isbn = isbn;
    this-> copies = copies;
    this->next = nullptr;
}

void Book:: setTitle(string title){
    this-> title = title;
}
string Book:: getTitle()const{
    return title;
}
void Book:: setISBN(string isbn){
    this-> isbn = isbn;
}
string Book::getISBN()const{
    return isbn;
}
void Book::setNext(Book* next){
    this-> next = next;
}
Book* Book:: getNext() const{
    return next;
}

istream& operator >> (istream& is, Book& book){
    string input;
    int counter = 1;
    while (counter==1 && !is.eof()){
        getline(is, book.title);
        getline(is, input);
        book.authorCount = atoi(input.c_str());
        for (int i=0; i<book.authorCount; i++){
            getline(is, book.authors[i]);
        }
        getline(is, book.publisher);
        is >> book.yearPublish;
        is >> book.hardcover;
        is >> book.price;
        is >> book.isbn;
        getline(is, input);
        counter++;
    }
    return is;
}

ostream& operator << (ostream& os, const Book& book){
    os<< "Title: " << book.title << endl;
    for (int i=0; i<book.authorCount; i++){
        os<< "Authors: " << book.authors[i] << endl;
    }
    os<< "Publisher: " << book.publisher << endl;
    os<< "Year Published: " << book.yearPublish << endl;
    os<<"Cover: ";
    if (book.hardcover ==0) os << "Paperback" << endl;
    else if (book.hardcover==1) os << "Hardcover" << endl;
    os<< "Price: $" << book.price << endl;
    os<< "ISBN: " << book.isbn << endl;
    os<<"Copies: " << book.copies << endl<< endl;
    return os;
}

Warehouse.cpp

#include "Warehouse.h"
#include "Book.h"
#include <iostream>
#include <string>

using namespace std;

Warehouse:: Warehouse() {}

Book* Warehouse:: find(string isbn) const{
    Book* testBook = new Book();
    Book* correctBook = new Book();
    bool result = false;
       Book* temp;

    while (result == false){
        if (testBook->getISBN() == isbn){
            result = true;
            correctBook = testBook;
            break;
        }
        else if(testBook->getISBN() != isbn){
            temp = testBook;
            testBook = temp->getNext();
        }
    }
    return correctBook;
}

void Warehouse :: list() const{
    Book* temp = head;
    for(int i=0;i<this->bookCount;i++){
        cout << temp;
        temp = head->getNext();
    }           
}
    

istream& operator >> (istream& is, Warehouse& warehouse){
    
    int counter = 0;
    Book* temp = new Book();
    Book inputNode;
    Book* headNode = new Book();
       headNode = new Book(); 
          while (!is.eof()&&is >> inputNode){
              if (!warehouse.head){
                  headNode = &inputNode;
                  temp = headNode;
                  counter=1;
              }
              else{
                  temp->setNext(&inputNode);
                  temp = temp->getNext();
                  counter++;
              }
         }
        warehouse.head = headNode;
        warehouse.bookCount = counter;   
              return is;              
}

ostream& operator << (ostream& os, const Warehouse& warehouse){
    os << warehouse.bookCount << endl;
    return os;
}

[Repeater]

This code is full of memory leaks.

  headNode = new Book(); 
          while (!is.eof()&&is >> inputNode){
                       Book* node = new Book();
                       node = &inputNode; // LEAK HERE!
              if (!warehouse.head){
                  headNode = node; //ANOTHER LEAK HERE! 

 Book* temp = new Book();
    temp = head; // LEAK! 

Anyway, apart from the memory leaks :

You create a local variable, Book inputNode;
Then you make some pointers point at it.
Then the function ends and the Book object is destroyed.
But your pointers still point that memory. This is very bad. Your pointers are pointing at objects that have already been destroyed.

It seems clear that you do not understand memory or pointers well enough to use them yet. You should go back and learn about memory and pointers before using them. This sounds harsh but there it is; we'd be doing you no favours if we didn't tell you that you don't understand pointers and memory well enough to use them.

[daveed]

Could that be the reason for the segmentation faults?

Also, you are absolutely right. I am not too comfortable with pointers or memory, but unfortunately I do not have time to go back and learn everything since the project is due tomorrow.

if I were to use delete function would that fix the problem?

Thank You so much for replying to this forum. I really appreciate it.

Yea, you're absolutely right. I'll go try to understand the memory and pointer before going back to the project.

Thank You

[Repeater]

Trying to use a pointer that is pointing at an object that doesn't exist can ofter cause a segfault.

Look at this code:

Book* Warehouse:: find(string isbn) const{
    Book* testBook ;
    Book* correctBook;
    bool result = false;
       Book* temp;

    while (result == false){
        if (testBook->getISBN() == isbn){ // SEGFAULT HERE! 

What is testBook pointing at? Random memory. THere is no Book object here. Just pointers, that are pointing at random memory. This causes segFaults.

[daveed]

Oh my... thank you so much for the lead. I really mean it. I was just about to kinda give up on the project and start learning the actual concepts. I will give it a try immediately.

Thank You so much again!

Hi, I just tried fixing it, but I do not know if I did the right job. I will edit the code shortly after this comment.

What does it mean if I am getting outputs like 0x5574e3dcba90?

[Repeater]

It means you're reading random nonsense values; whatever happens to be left over in memory when you read memory you shouldn't be.

Or you're outputting pointers, which are just memory address. Just numbers.

[daveed]

void Warehouse :: list() const{
    Book* node = head;
    for(int i=0;i<this->bookCount;i++){
        Book* temp = node;
        cout << temp;
        node = node->getNext();
        delete temp;
    }           
}

I changed my list method like this. Would this still leak memory?

I am trying to take one problem at a time, hopefully it will solve my problems in the end.

[Repeater]

This code... goes over an entire linked list, calling delete on every node in the list. If every node was created using new, then this will not crash. If any of those nodes have already been deleted, this will crash.

It's an oddly named function; list. Usually a function that deletes every node in a list is called something like "deleteList".

[daveed]

I must be doing something very wrong because this is a method that is supposed to list all the books inside the warehouse.

I think I have found my problem to be in Book.cpp in the operator >> method.

istream& operator >> (istream& is, Book& book){
    string input;
    int counter = 1;
    while (counter==1 && !is.eof()){
        getline(is, book.title);
        getline(is, input);
        book.authorCount = atoi(input.c_str());
        for (int i=0; i<book.authorCount; i++){
            getline(is, book.authors[i]);
        }
        getline(is, book.publisher);
        is >> book.yearPublish;
        is >> book.hardcover;
        is >> book.price;
        is >> book.isbn;
        getline(is, input);
        counter++;
    }
    return is;
}

I have been struggling with these kinds of code all year long, mainly because some variables just cannot be read straight from the file.
from the top
Title = string
authorCount = int
authors[] = string
publisher = string
yearPublish = short
hardcover = bool
price = float
copies = long.

I am pretty sure this is where I am messing up because I tried writing a method just to print out the head node, but it still gave me 0x55c034bcd190.

I think I should be using cin.ignore() inside my operator method, but I just do not know where it should go.

[Repeater]

I am pretty sure this is where I am messing up because I tried writing a method just to print out the head node, but it still gave me 0x55c034bcd190.

You're sending a pointer to output. A pointer. A pointer is just a memory address, so you're outputting just a memory address.

    int counter = 1;
    while (counter==1 && !is.eof()){
        getline(is, book.title);
        getline(is, input);
        book.authorCount = atoi(input.c_str());
        for (int i=0; i<book.authorCount; i++){
            getline(is, book.authors[i]);
        }
        getline(is, book.publisher);
        is >> book.yearPublish;
        is >> book.hardcover;
        is >> book.price;
        is >> book.isbn;
        getline(is, input);
        counter++;
    }

This while loop will only execute once, because counter is 1 only once. Why write a loop that only loops once?

[dhayden]

There's an old saying: "garbage in, garbage out." It means that if you start with junk, you'll never end with proper input.

To make sure you aren't starting with junk, you should test your read and write methods. Temporarily put this at the top of your main function:

book b;
while (cin >> b) {
    cout << "\nRead a book:\n" << b;
}
return 0;

Debug this code until it reads and writes books the way it should. Note that a problem could be in the input or the output function. You can narrow it down by looking at b with a debugger after you read it.

It's hard to tell where you might be going wrong without the full code. Please post Book.h. If possible, post all the code needed to make a working program.

[jlb]

What do your Book and Warehouse definitions look like? Also a small complete program would probably help us help you as well.

Are you using input files to get the Book information? If so please post a small sample of your input file.

By the way using eof() to control a data entry loop is usually not a good idea. You should consider using an actual read instead.

Have you studied things like std::vector, std::stringstream? A std::vector would be a good replacement for a couple of your arrays. A stringstream could making your read operations easier by keeping the underlying stream protected from most bad read operations.

And speaking of containers, your container (your homegrown list) should probably be a separate class. A Book should really only be concerned about one Book, it really shouldn't be concerned about the number of books in the Warehouse or the "location" of the next book. By separating your container out to it's own class you can test that the container is locating, inserting, removing, and any other container specific jobs outside of your current program.