C++ copy of char* data in struct to another struct

Question

I have an issue when I try to copy char* data from one struct to another. Here is the struct

struct connection_details
{
    char *server;
    char *user;
    char *password;
    char *database;
};

And basically what I am trying to do is to copy the data from one object to another (connection_setup is a private connection_details object) this is the code from the constructor of another object:

settings *tmp = new settings();
this->connection_setup.server = strdup(tmp->getSQLSettings().server);
delete tmp;

I keep getting segmentation fault which is understandable since I probably touch stuff that I should not be doing.

Basically both the settings object and the object I am in contains a private member variable of the type connection_details. I.e

class settings {

public:
    settings();
    ~settings();
    connection_details getSQLSettings();
private:
    connection_details sqldetails;
};

Thanks for any advise!

Why aren't you using std::string? Would make that whole problem (and quite a few more) essentially vanish. — Mat
– Mat, Commented Apr 26, 2014 at 11:13
new and free?! Also, can you share the settings constructor and getter for the sql settings? I mean the implementations. — László Papp
– László Papp, Commented Apr 26, 2014 at 11:13
I bet that if you change connection_details getSQLSettings() to connection_details& getSQLSettings() then it will work out fine. Alternatively, you can add a proper copy-constructor to struct connection_details. — barak manos
– barak manos, Commented Apr 26, 2014 at 11:16
Using std::strings for each of the 4 data members of connection_details generates some overhead due to the four-fold allocation and de-allocation. If this is performance critical, you should only make one allocation (and hence not use std::string). std::string is great if you don't care about efficiency. — Walter
– Walter, Commented Apr 26, 2014 at 11:40

László Papp · Accepted Answer · 2014-04-26 11:21:15Z

2

First of all, you are using new and free together. That is a very bad idea because the destructor will not be called in general. You should replace free with delete.

Moreover, you are getting the ownership for that return value of strdup, yet you do not free it anywhere, but hopefully that is done somewhere in the unshown code.

The problem here seems to be that there is a copy done for the return of the get sql method which will copy the pointer so now you have got two pointers pointing to the same.

This is a problem because when the first gets destructed, it will also destruct the pointer hopefully in your destructor, and when the second would run, you will be trying to delete a dangling pointer... That is probably what is causing the crash for you.

There are several approaches to address this issue:

Return reference to avoid the copy.
Make a proper copy constructor to copy the data, and not the pointer.
Use higher level C++ elements, like std::string, or at least smart pointers, etc.

edited Apr 26, 2014 at 11:21

answered Apr 26, 2014 at 11:15

László Papp

53.5k40 gold badges118 silver badges142 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

davvy Over a year ago

Yea. I meant to delete it. However, I found the issue. The reason why the segmentation fault occurred was that the value was not set on the settings class. So I basically did a copy of nothing.

László Papp Over a year ago

@user3575725: you are a good magician to hide the important parts of the code. :)

davvy Over a year ago

I think you have convinced me to use std::string instead. I will change the implementations. Thank you for your help.

Collectives™ on Stack Overflow

C++ copy of char* data in struct to another struct

1 Answer 1

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related