Segmentation Fault in memcpy operation of a struct

Question

I am adding the header file and cpp file (it has main fucntion).

AuctionPrices.h

#ifndef AuctionPrices_h
#define AuctionPrices_h      
/*
 *
 * class AuctionPrices - maintains Buy Order, Sell Order books
 */

#include <bits/stdc++.h> 
#include <map>

//#pragma pack(1)
struct Order 
{
    char * itemId;
    char * auctionId;
    int Side;
};
 

class AuctionPrices 
        {

     public:
         virtual int AddNewOrder(char *itemId, char *auctionId, int Side, int Price) = 0;

             virtual int DeleteOrder(char *itemId, char *auctionId) = 0;
   
             virtual int Print() = 0;

        };


class AuctionPrice_Imp : public AuctionPrices 
    {

    public:
        AuctionPrice_Imp();
        
        ~AuctionPrice_Imp();

        std::map <int, Order, std::greater< int >> BuyMap;

        std::map <int, Order, std::less< int >> SellMap;

        int AddNewOrder(char *itemId, char *auctionId, int Side, int Price);

        int DeleteOrder(char *itemId, char *auctionId);

        int Print();

    };

#endif

AuctionPrices_Imp.cpp

/** 
  * Auction Price Class implementation
  * Constructor, AddNewOrder, DeleteOrder, Print 
  *
  */
#include <bits/stdc++.h> 
#include <map>
#include "AuctionPrices.h"
using namespace std;

AuctionPrice_Imp::AuctionPrice_Imp()
{

}

AuctionPrice_Imp::~AuctionPrice_Imp()
{

}


int AuctionPrice_Imp::AddNewOrder(char *itemId, char *auctionId, int Side, int Price)
{

    Order order;
    memcpy(order.itemId, itemId, strlen(itemId)+1);
    memcpy(order.auctionId, auctionId, strlen(auctionId)+1);
    order.Side = Side;

    if (Side == 1)
    {   
        BuyMap.insert (std::pair<int,Order>(Price,order));
        //buyLevels_.insert( std::pair< OfPrice, Level< OrderEntry > >( price, buyLevel ) );

    }
    else if (Side == 2)
    {
        SellMap.insert (std::pair<int,Order>(Price,order));
    }
    else 
{
    return 0;
}
    


return 1;
}

int AuctionPrice_Imp::DeleteOrder(char *itemId, char *auctionId)
{
    


    return 0;
}

int AuctionPrice_Imp::Print()
{
    std::map <int,Order,std::greater< int >>::iterator buy_it;
    std::map <int,Order,std::less< int >>::iterator sell_it;
    // Print Sell Map
    for ( sell_it = SellMap.begin();sell_it != SellMap.end(); sell_it++)
    {
        std::cout << sell_it->first << '\t' << std::endl;

    }   
    // Print Buy Map
    for ( buy_it = BuyMap.begin();buy_it != BuyMap.end(); buy_it++)
    {
        std::cout << buy_it->first << '\t' << std::endl;

    }   
    
    return 1;
}



int main()
{

    AuctionPrice_Imp * auctionPrice_Imp = new AuctionPrice_Imp();
    
    /*
    AddNewOrder(“item1”, “auction1”, 1, 100)
    AddNewOrder(“item1”, “auction2”, 1, 101)
    AddNewOrder(“item2”, “order3”, 1, 99)
    AddNewOrder(“item2”, “order4”, 2, 100)
    
    */
    
    auctionPrice_Imp->AddNewOrder("item1", "auction1", 1, 100);
    auctionPrice_Imp->AddNewOrder("item1", "auction2", 1, 101);
    auctionPrice_Imp->AddNewOrder("item2", "order3", 1, 99);
    auctionPrice_Imp->AddNewOrder("item2", "order4", 2, 100);
    
    auctionPrice_Imp->Print();

}

When I am running the code its giving segmentation fault at the line:

memcpy(order.auctionId, auctionId, strlen(auctionId)+1);

Please anyone can help or correct the code. The functions I am calling are supposed to add the orders to the Maps: BuyMap and SellMap. Once they have added to those map, I am using a print function to print the values.

Why all the pointers, and `memcpy` and `strlen`? There are better ways to do these things in c++. — cigien, Aug 10 '20 at 04:22
There is a `std::string` class in C++. Why are you not using it? If you did that, then all of those `char*` would not be needed, thus all the issues with `memcpy` go away because that won't be needed. — PaulMcKenzie, Aug 10 '20 at 04:26
don't mix `memcpy` and `strlen`. Use `strcpy` if you need to copy a null-terminated string. If you don't have a null terminated string, using`strlen`is a fatal mistake. — user4581301, Aug 10 '20 at 04:33
Please [don't include ``](https://stackoverflow.com/Questions/31816095/Why-Should-I-Not-Include-Bits-Stdc-H.) — Some programmer dude, Aug 10 '20 at 04:37

Sam Varshavchik · Answer 1 · 2020-08-10T10:27:17.720

Order order;

This creates a new Order object. Order does not have a constructor, so none of its class members, itemId, and auctionId, get initialized to point to anything. These pointers are uninitialized, random garbage. Immediately afterwards:

 memcpy(order.itemId, itemId, strlen(itemId)+1);
 memcpy(order.auctionId, auctionId, strlen(auctionId)+1);

Since neither itemId, nor auctionId, point to sufficient memory that are at least strlen(itemId)+1 or strlen(auctionId)+1 in size, respectively, this results in undefined behavior, and your crash.

In C++, before using a pointer, it is your responsibility to make sure that the pointer is valid, and points to the correct object, objects, or sufficiently-sized memory buffers. C++ will not do that for you, you have to do all that work yourself.

But if your intent is to write modern C++ code, it is much simpler just to use C++ classes, like std::strings instead of plain char * pointers. std::strings automatically handle all these low-level details, manage memory properly, without making it your responsibility to do so. You will find a complete description of std::string and many examples of using it in your C++ textbook.

are there some benefits of using char * string instead of std::string ? I can initialize Order * order = new Order(); this doesn't change the segmentation fault. — dk chouhan, Aug 10 '20 at 05:57
@dkchouhan Don't introduce pointers where thay are not needed `Order order;` is correct. — john, Aug 10 '20 at 06:48
`new Order` also creates a new `Order` object. The new object's member pointers are still uninitialized, random garbage that do not point to anything valid. — Sam Varshavchik, Aug 10 '20 at 10:28

score 1 · Answer 2 · answered Aug 10 '20 at 05:24

1

You should use std::string to avoid having to deal with low level issues of pointers and memory allocation. These are the issues that you are getting wrong in your code. Here's your code rewritten to use std::string

struct Order 
{
    std::string itemId;
    std::string auctionId;
    int Side;
};

int AuctionPrice_Imp::AddNewOrder(std::string itemId, std::string auctionId, int Side, int Price)
{    
    Order order;
    order.itemId = itemId;
    order.auctionId = auctionId;
    order.Side = Side;

See how easy that is? The code to use std::string is no different to the code that handles int.

answered Aug 10 '20 at 05:24

john

85,011
4
57
81

whats the benefit of using char * instead of std::string ? – dk chouhan Aug 10 '20 at 06:02
@dkchouhan For string handling there is no benefit at all. – john Aug 10 '20 at 06:43
1

@dkchouhan I often see a tendency among novice programmers. Because pointers are difficult, beginners often think there must be some 'secret' benefit to using them. Very occaisionally using pointers is the right thing to do but 99% of the time the right thing to do is to avoid the complexity of pointers by using one of the many classes that C++ offers (`std::string` being just one example). As a beginner you should **never** use pointers. – john Aug 10 '20 at 06:52
if we are passing pointers we are passing just an address but in case of string we are passing the whole string, it has some more cost associated ? – dk chouhan Aug 10 '20 at 07:15
1

@dkchouhan Not necessarily. Firstly you can use `const std::string&` to pass by reference (that is recommended for the code I posted above). Secondly you can use *move semantics* and *copy elision* to minimise the number of copies made (but those are two big topics in themselves). Thirdly, most importantly of all, don't prematurely optimise your code. Computers are very fast (that should be obvious) so only when you've timed your code and found that the use of `std::string` is a problem, should you consider something else. Until then it's much more important to write clear bug-free code. – john Aug 10 '20 at 08:16
@dkchouhan: In addition to what john said, having a `char *` pointing to dynamic memory *always* requires a dynamic memory allocation. Small string optimization (SSO) allows small `` objects to reside on the stack only. Also, `char *` can result in resource leaks (e.g. when an exception is thrown past your `free()`). All in all, there is no real-world benefit in using C strings in C++ code. – DevSolar Aug 10 '20 at 10:33

Segmentation Fault in memcpy operation of a struct

AuctionPrices.h

AuctionPrices_Imp.cpp

2 Answers2