const char* in my class has junk character(s) after it's returned from function

Question

Class:

class myclass {
  public:
    myclass(void);

    const char* server;

  private:
    char pidchar[6];
    int pidnum;

};

The function

myclass parseINI(const char* file)
{
    myclass iniOptions;
    CSimpleIniA ini;
    ini.SetUnicode();
    ini.LoadFile(file);
    const char* server = ini.GetValue("", "server", "");
    iniOptions.server = server;
    std::cout << server << "\n"; // Prints the correct value here
    fflush(stdout);
    return iniOptions;


}

Calling it from the main function

int _tmain(int argc, TCHAR* argv[])
{

 myclass options;
 options = parseINI("myapp.ini");
 std::cout << options.server << "\n"; // It prints junk here
 return 0;
}

What did I do wrong?

score 5 · Accepted Answer · answered Dec 02 '10 at 19:43

5

The const char* returned by GetValue() probably belonged to the ini object. When you exited the parseIni() function, ini went out of scope and was destroyed, which could mean your pointer is no longer valid.

Try using a std::string for the server member type instead of const char*.

answered Dec 02 '10 at 19:43

aschepler

70,891
9
107
161

If `CSimpleINIA` is modifiable, this is definitely better. – Steve Townsend Dec 02 '10 at 19:52
The suggested change is to `myclass`, not `CSimpleIniA`. – aschepler Dec 02 '10 at 19:54
+1 for suggesting the use of std::string which will create a local copy. – hookenz Dec 02 '10 at 19:58

Steve Townsend · Answer 2 · 2010-12-02T20:06:54.897

2

It looks like you are using memory that is released when CSimpleIniA goes out of scope in parseINI.

const char* server = ini.GetValue("", "server", "");
iniOptions.server = server;

Copy the value that is returned into a new memory block before you return from the parseINI function.

string server = ini.GetValue("", "server", "");
iniOptions.server = new char[server.length() + 1];
std::copy(server.begin(), server.end(), iniOptions.server);         
iniOptions.server[server.length()] = 0;

edited Dec 02 '10 at 20:06

answered Dec 02 '10 at 19:42

Steve Townsend

53,498
9
91
140

@Martin - just pointing out where the error was while I tried out the alternative. – Steve Townsend Dec 02 '10 at 19:50
@codefrog - edited this in. If you can change the struct to contain a `string`, that's better though (per @aschepler). – Steve Townsend Dec 02 '10 at 19:53
I'm getting this: error C2065: 'data' : undeclared identifier – codefrog Dec 02 '10 at 20:01
A problem in my cut and paste, sorry - check now. – Steve Townsend Dec 02 '10 at 20:06

diwatu · Answer 3 · 2012-05-29T19:21:38.433

The way you are using class as a function returned data type in C++ is totally wrong. In C++ there are 2 kinds of data type: value type, reference type. class belongs to second one; From a function you can return a value type data or a pointer of any data.But you cann't retun a entity of a reference type. Because a entity of a reference type will be released right after the code reached out of the scope which the entity is defined.

You can do in either way:

1: define parseINI as:

     myclass* parseINI(const char* file) 
     {     
           myclass* iniOptions = new myclass();
           ........
           return iniOptions;   
      }

and then use it like this:

      myclass* options = parseINI("myapp.ini");

2: define parseINI as:

       void parseINI(myclass& options, const char* file) 
        {     
           ........//asigne value to options's members
        }

and then use it like this:

        myclass options;
        parseINI(options,"myapp.ini");

3: Do what you did, but add a asignment method (operator=) to myclass

score 1 · Answer 4 · answered Dec 02 '10 at 19:44

const char* server = ini.GetValue("", "server", "");

This value is falling out of scope when the function terminates, so when you assign the value of that pointer to your object's server pointer, the place in memory they point to is having its memory freed off the stack at the end of the function, and it's then overtaken by other things.

Using a std::string or even just a char[] will be preferred to just fix the problem with the least amount of changes, as they will by assigned the actual value and not a location in memory like pointers.

What you really should do is look up referential transparency, though. That will prevent problems like this from occurring ever again

score 1 · Answer 5 · answered Dec 02 '10 at 19:46

I's guess that the lifetime of the data pointed to by the char* returned from CSimpleIniA::GetValue() is the same as the CSimpleIni object itself. So when ini is destructed, the pointer returned from GetValue() becomes invalid. (I've never used CSimpleIni, and haven't looked at the docs carefully enough to know for sure, but that's what the behavior points to).

I'd suggest changing myclass::server to be a std:string object and set it using something like:

iniOptions.server = std::string(server);

which will give the myclass::server object it's own copy of the string data.

score 0 · Answer 6 · edited Dec 30 '13 at 17:35

0

iniOptions is located on the stack and disposed automatically when the function returns. You should allocate it on heap using new()

edited Dec 30 '13 at 17:35

BenMorel

34,448
50
182
322

answered Dec 02 '10 at 19:41

Vladimir Ivanov

42,730
18
77
103

Dear god, don't forget to attempt a delete in the deconstructor, though. – Squirrelsama Dec 02 '10 at 19:44
1

This should not matter, due to RVO. The issue is the use of other stack-allocated memory via `ini`. http://stackoverflow.com/questions/1394229/understanding-return-value-optimization-and-returning-temporaries-c – Steve Townsend Dec 02 '10 at 19:44
Even without RVO, this will not cause a problem (unless a class has a bad copy constructor, which is not the case this time). There is no good reason here not to return a class object by value. – aschepler Dec 02 '10 at 19:49
There are containers already designed for holding strings that create and delete the memory correctly please use them they are refereed to as std::string. – Martin York Dec 02 '10 at 19:49
@Martin York, please use punctuation. :) – Dima Dec 02 '10 at 20:17
@Dima: There is a full stop at the end. – Martin York Dec 02 '10 at 21:04
@Martin You need two more and a comma. – Dima Dec 02 '10 at 21:14

Dima · Answer 7 · 2010-12-02T20:14:31.877

The problem is that the local variable server points to a character buffer returned by ini.GetValue(), which is destroyed when paraseINI() returns.

One way to fix this is to allocate a new buffer yourself and copy the characters.

const char* server = ini.GetValue("", "server", "");
int length = strlen(server) + 1;  // length of the string +1 for the NULL character.
delete [] iniOptions.server; // free the old buffer
iniOptions.server = new char[length]; // allocate your own buffer
strncpy(iniOptions.server, server, length); // copy the characters

For this to work you have to make myclass::server non-const, and you have to initialize it to NULL in the constructor and delete it in the destructor.

A better way to deal with this situation would be use std::string instead of char * for muclass::server. This way std::string would take care of memory management for you, and the code would be exception-safe.

If you make muclass::server an std::string, then you simply do

const char* server = ini.GetValue("", "server", "");
iniOptions.server = std::string(server);

And you do not have to do anything with it in the constructor or the destructor.

error C2664: 'strlen' : cannot convert parameter 1 from 'std::string' to 'const char *' 1> No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called error C2664: 'strncpy' : cannot convert parameter 1 from 'const char *' to 'char *' 1> Conversion loses qualifiers — codefrog, Dec 02 '10 at 20:10
You shouldn't be getting this error if `server` is `const char *`. And if you make it an `std::string` then you do not need to do your own memory allocation, i. e. no `strlen()` and no `new`. — Dima, Dec 02 '10 at 20:12

const char* in my class has junk character(s) after it's returned from function

7 Answers7

Linked