Problems with dereferencing a pointer (and returning it)

Question

Here I have a function that creates a string, assigns it to a string pointer, and returns it. I tried returning a regular string and it worked fine. But then when when I integrated the pointers and de-referenced them, my program crashed. When I tried to debug it, this is the message I got:

Unhandled exception at 0x00024cbf in Assignment 2.exe: 0xC0000005: Access violation reading location 0xcccccce4.

Here's my code:

string* Recipe::getCookingTime()
// @intput: none
// @output: cooking time as a string
{
    string temp;
    string displayHrs;
    string displayMins;
    if( cookingTime_->numHours < 10 ) 
        displayHrs = intToString(0) + intToString(cookingTime_->numHours );
    else 
        displayHrs = intToString(cookingTime_->numHours );
    if( cookingTime_->numMinutes < 10 ) 
        displayMins = intToString(0) + intToString(cookingTime_->numMinutes);
    else 
        displayMins = intToString(cookingTime_->numMinutes);

    temp = "The time to cook the recipe is " + displayHrs + ":" + displayMins;
    *cTime_ = temp;
    return cTime_;
}

Thanks you so much for all the responses, now I fixed it and it's working. I forgot to allocate memory :S. I know that there are easier wasy to do this, but I have to write a program that meets certain specs. being implementing this function one of them. — Jota, Mar 19 '11 at 06:51

drewag · Accepted Answer · 2011-03-19T15:20:38.593

The problem is that you are dereferencing the cTime_ variable without actually allocating the memory first. I am not sure if that is a global variable or a member variable but you need to use the "new" operator first to allocate it's memory. So you return the pointer to (address of) this variable back to the caller of the function, but as soon as this function exits, it is deleting the "temp" variable and therefore, the pointer you returned will be pointing at invalid memory.

The solution would be to use the "new" operator:

string* Recipe::getCookingTime()
// @intput: none
// @output: cooking time as a string
{
    string displayHrs;
    string displayMins;
    if( cookingTime_->numHours < 10 ) 
        displayHrs = intToString(0) + intToString(cookingTime_->numHours );
    else 
        displayHrs = intToString(cookingTime_->numHours );
    if( cookingTime_->numMinutes < 10 ) 
        displayMins = intToString(0) + intToString(cookingTime_->numMinutes);
    else 
        displayMins = intToString(cookingTime_->numMinutes);

    if( NULL == cTime_ )
    {
        cTime_ = new string();
    }

    *cTime_ = "The time to cook the recipe is " + displayHrs + ":" + displayMins;
    return cTime_;
}

However, I have to warn you that this is not good design because here you are allocating memory and require that the call knows they have to free it when they are done with it. A preferable way to do it would be to have the caller allocate the variable and then pass in the pointer:

bool Recipe::getCookingTime( string* str )
// @intput: none
// @output: cooking time as a string
{
    if( NULL == str )
    {
        // Received invalid pointer
        return false;
    }
    string displayHrs;
    string displayMins;
    if( cookingTime_->numHours < 10 ) 
        displayHrs = intToString(0) + intToString(cookingTime_->numHours );
    else 
        displayHrs = intToString(cookingTime_->numHours );
    if( cookingTime_->numMinutes < 10 ) 
        displayMins = intToString(0) + intToString(cookingTime_->numMinutes);
    else 
        displayMins = intToString(cookingTime_->numMinutes);

    *str = "The time to cook the recipe is " + displayHrs + ":" + displayMins;
    return true;
}

Then when the caller wants to use the function they can do this:

cTime_ = new string();
getCookingTime( cTime_ );

Summary The important thing to remember here is that you must allocate memory the a pointer is referencing before trying to assign to it. Also, it is generally bad design to allocate memory (using the new operator) within a function and not explicitly delete it. Whoever allocates memory should almost always be the one to free it

When you re-read the question and realize your original answer was wrong, but you think it's still good advice, best thing to do is surround it with `~~` and `~~` so it's crossed out but still readable. Your first code sample is also a disaster... `*string = ...`? `string` is a type! — Ben Voigt, Mar 19 '11 at 05:20
Good point about the edit, I will fix it, but I am not dereferencing the type, that is a pointer to a string type that is passed in. — drewag, Mar 19 '11 at 15:14
We can all see your edit history, so it makes you look much better to acknowledge a typo and fix it rather than denying problems raised in the comments. — Ben Voigt, Mar 19 '11 at 16:12
I didn't intend to ignore a problem, I only wanted to make it a more clear answer for anyone else who reads it. You could probably be a little more respectful as well. I am new to the forum and still learning the standard practices. All I am trying to do is learn and help out other people. — drewag, Mar 19 '11 at 16:21

score 2 · Answer 2 · answered Mar 19 '11 at 03:51

2

*cTime_ = temp;

It seems you've not allocated memory for cTime_.

I'm wondering why you're returning pointer to std::string. Why don't you simply return std::string as shown below:

std::string Recipe::getCookingTime()
{
   //your same code
   return temp; //this is fine!
}

Note that the type of return type is changed from std::string* to std::string.

answered Mar 19 '11 at 03:51

Nawaz

353,942
115
666
851

The only reason not to do it this way is to prevent the copying of the returned string that happens. (Which is only really an issue in embedded environment, a really intensive application, or for a function that gets called relatively frequently). – drewag Mar 19 '11 at 04:01
@drewag: Nowadays most compilers implements RVO : http://en.wikipedia.org/wiki/Return_value_optimization ... Beside, if you allocate memory and return it, and after having done with it, deallocate the memory << this approach also takes time. One thing can be done, one can pass `std::string &cTime` as parameter, and update the `cTime` in the function! – Nawaz Mar 19 '11 at 04:06

score -2 · Answer 3 · answered Mar 19 '11 at 05:09

I'll just focus on where your question addresses and not anywhere else in your code. First, I don't think you posted your complete code, because with what you posted, I don't see where cTime_ is defined in that method, so your code won't even compile. Second, assume you define cTime_ as a pointer to a string, and assign that pointer to a the memory occupied by the string temp. When that method exits, temp goes out of scope, now cTime_ no longer points to a valid memory location, thus you get the access violation. You might consider something like this:

void Recipe::getCookingTime( string& str )
{
    string displayHrs;
    string displayMins;
    if( cookingTime_->numHours < 10 ) 
        displayHrs = intToString(0) + intToString(cookingTime_->numHours );
    else 
        displayHrs = intToString(cookingTime_->numHours );
    if( cookingTime_->numMinutes < 10 ) 
        displayMins = intToString(0) + intToString(cookingTime_->numMinutes);
    else 
        displayMins = intToString(cookingTime_->numMinutes);

    str = "The time to cook the recipe is " + displayHrs + ":" + displayMins;
}

Then to call getCookingTime():

string s;
getCookingTime(s);

Instead of dealing with pointer, you would now deal with reference. The code would be more straightforward.

No, `cTime_` is not pointed at `temp`. `temp` is copied into whereever `cTime_` points (apparently nowhere, based on the error message). — Ben Voigt, Mar 19 '11 at 05:20
You are doing this the hard way, for for no good reason. `string s = getCookingTime();` is just as efficient, and probably does the same thing if you return the string by value. Trust your compiler! — Bo Persson, Mar 19 '11 at 06:04

Problems with dereferencing a pointer (and returning it)

3 Answers3