Construction of an object inside a vector with emplace_back wipes previous value attribute data in map of corresponding object pointers

Question

I am writing a function that creates a vector of Product objects from .csv file data and emplaces the object pointer value into a map with its corresponding Product ID as the key. When I run a test that should print each objects attributes from the map, I get a runtime error std::bad_alloc.

Here's the code:

#include <string>
#include <map>
#include <vector>
#include <fstream>
#include <iostream>
#include <sstream>

class Product {
    private:
        std::string productID;
        std::string productName;
        double productPrice;
    public:
        Product(std::string id, std::string name, double price) {
            productID = id;
            productName = name;
            productPrice = price;
        }
        std::string getProductID() {
            return productID;
        }
        std::string getProductName() {
            return productName;
        }
        double getProductPrice() {
            return productPrice;
        }
};
class Reader {
    public:
        static std::vector<std::vector<std::string>> readCSV(std::string fileName) {
            std::vector<std::vector<std::string>> content;
            std::vector<std::string> row;
            std::string line, word;
            std::fstream file (fileName, std::ios::in);
            if (file.is_open()) {
                while (getline(file, line)) {
                    row.clear();
                    std::stringstream str(line);
                    while(getline(str, word, ',')) {
                        row.push_back(word);
                    }
                    content.push_back(row);
                }
            } else {
                std::cout << "Could not open the file" << std::endl;
            }
            return content;
        }
};
std::map<std::string, Product*> loadProductMap(std::string fileName) {
    std::vector<std::vector<std::string>> productMapFile = Reader::readCSV(fileName);
    std::vector<Product> products;
    std::map<std::string, Product*> productMap;
    for (size_t i = 0; i < productMapFile.size(); i++) {
        products.emplace_back(productMapFile.at(i).at(0), productMapFile.at(i).at(1), stod(productMapFile.at(i).at(2)));
        productMap.emplace(products.at(i).getProductID(), &products.back());
    }
    return productMap;
}
int main() {
    std::map<std::string, Product*> productMap = loadProductMap("product_list.csv");
    for (auto &i : productMap) {
        std::cout << i.second->getProductID() << " " << i.second->getProductName() << " " << i.second->getProductPrice() << std::endl;
    }
    return 0;
}

Here is product_list.csv:

Meat01,T-Bone Steak,7.99
Meat02,Tyson Fresh Chicken Wings,10.00
Icecream01,Chocolate Ice Cream,2.50
Iceceam02,Vanilla Ice Cream,2.50
Corn01,Fresh Sweet Corn,2.00
Casewater01,24 Bottles 16-Oz of Deer Park Water,4.99
Potatochips01,Plain Potato Chips,2.00
Potatochips02,Green Onion Potato Chips,2.00
Donuts01,Glazed Donuts One-Dozen,4.99
Saugage01,8-Sausage Pack,4.99
Eggs01,Dozen Eggs,3.00
Milk01,Gallon Milk,4.00

The output:

  4.99
  2
Donuts01 �SRm�U�ce-Dozen 4.99
Eggs01 Dozen Eggs 3
  2.5
  2.5
terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
Aborted

Expected output:

Meat01 T-Bone Steak 7.99
Meat02 Tyson Fresh Chicken Wings 10
Icecream01 Chocolate Ice Cream 2.5
Iceceam02 Vanilla Ice Cream 2.5
Corn01 Fresh Sweet Corn 2
Casewater01 24 Bottles 16-Oz of Deer Park Water 4.99
Potatochips01 Plain Potato Chips 2
Potatochips02 Green Onion Potato Chips 2
Donuts01 Glazed Donuts One-Dozen 4.99
Saugage01 8-Sausage Pack 4.99
Eggs01 Dozen Eggs 3
Milk01 Gallon Milk 4

After inserting a breakpoint, I noticed that products.emplace_back() seemingly works correctly, and then productMap.emplace() works correctly, but then on the loop back around, the creation of the second Product object from products.emplace_back() wipes the previous Product's attribute data inside the map. The productMap.emplace() then works correctly again, copying over the new Products data... and so on...

After the first product is added to the map, but before the second product is created in the vector

After the second product is created in the vector

I've tried changing products.emplace_back to the push_back method and productMap.emplace to the insert method. Also, I tried breaking the loop down into two separate loops like this:

for (auto productData : productMapFile) {
        products.emplace_back(productData.at(0), productData.at(1), productData.at(2));
    }
    for (auto product : products) {
        productMap.emplace(product.getProductID(), &product);
    }

What is causing this? I'd love to understand it more. A work around is fine too.

When you insert elements in a vector, it risks causing the vector to have to reallocate to grow its internal storage. When that happens, all existing references and pointers to its elements (such as those in `productMap`) are invalidated. You could make sure the vector never grows by using `reserve` ahead of time with enough capacity, or you could try storing vector index in your map instead of pointers. — François Andrieux, Aug 28 '23 at 16:59
`productMap.emplace(products.at(i).getProductID(), &products.back())` -- Do not store addresses of vector elements such as `&products.back()`. As noted in the previous comments, if the vector is resized and a reallocation is done, that address is invalidated. — PaulMcKenzie, Aug 28 '23 at 17:02

score 0 · Answer 1 · answered Aug 29 '23 at 23:43

There are two major issues with your loadProductMap function:

Resizing the vector can result in reallocation which invalidates existing pointers into that vector. This has been identified already in the comments section. A typical way to avoid this is to reserve storage up front:
```
products.reserve(productMapFile.size());
```
Provided you don't push more elements into the vector than this reserved size, it's guaranteed no reallocation will take place. However, this will not resolve your problem, because of the other issue...
The products vector is local to that function, and the returned map stores pointers into the vector. That means all pointers become invalid when the function returns, because the products vector is destroyed before the caller can get access to the data.

In both cases, you have pointers to destroyed objects, resulting in undefined behavior. Your product map represents an indexed lookup into a data structure that no longer exists.

Simple solution

You can avoid all this by just storing Product values in the map instead of pointers. In the program as currently shown, there's no clear reason to use pointers in the first place.

Here's what that looks like:

std::map<std::string, Product> loadProductMap(const std::string& fileName) {
    auto csvRows = Reader::readCSV(fileName);
    std::map<std::string, Product> productMap;
    for (auto& row : csvRows) {
        Product product(row.at(0), row.at(1), stod(row.at(2)));
        productMap.emplace(product.getProductID(), std::move(product));
    }
    return productMap;
}

int main() {
    auto productMap = loadProductMap("product_list.csv");
    for (auto& entry : productMap) {
        auto& product = entry.second;
        std::cout << product.getProductID() << " "
                  << product.getProductName() << " "
                  << product.getProductPrice() << "\n";
    }
}

Notes

When designing a class with accessor functions, always think about whether that function needs to modify the underlying object. If not, the function should be qualified as const.

If you don't do this, then the following would cause a compilation error:

const Product& product = entry.second;
std::cout << product.getProductID() << " "
          << product.getProductName() << " "
          << product.getProductPrice() << "\n";

Error (only first one shown):

error: passing 'const Product' as 'this' argument discards qualifiers [-fpermissive]
std::cout << product .getProductID() << " "
             ~~~~~~~~~~~~~~~~~~~~~^~

note:   in call to 'std::string Product::getProductID()'
std::string getProductID() {
            ^~~~~~~~~~~~

Furthermore, your string accessors are returning a copy of the string. This is usually undesirable, and you should instead return a const reference to the string.

Similarly, your constructor should take references instead of copying its string parameters. And, you may consider using initializer list instead of directly assigning within the constructor's body.

Fixing all of these things results in Product defined as follows:

class Product {
    private:
        std::string productID;
        std::string productName;
        double productPrice;
    public:
        Product(const std::string& id, const std::string& name, double price)
            : productID(id)
            , productName(name)
            , productPrice(price)
        {
        }
        const std::string& getProductID() const {
            return productID;
        }
        const std::string& getProductName() const {
            return productName;
        }
        double getProductPrice() const {
            return productPrice;
        }
};

Construction of an object inside a vector with emplace_back wipes previous value attribute data in map of corresponding object pointers

1 Answers1

Simple solution

Notes