Pointer returned from a function isn't valid

Question

I'm assisting a friend with an assignment, an implementation of a linked list, though my C++ skills are rusty at best these days.

There's a few structs

struct Song{
int id;
string name;
string singerName;
};

struct SongNode {
Song sg;
SongNode *previousNode;
SongNode *nextNode;
};

struct SongDoublyLinkedList{
SongNode *firstElement;
};

And this short piece that runs in main

SongDoublyLinkedList* list = new SongDoublyLinkedList();

SongNode* song1 = new SongNode();

song1->sg.id = 1;
song1->sg.name = "All Night";
song1->sg.singerName = "Parov Stelar";

addSong(list, song1);

SongNode* song2 = new SongNode();

song2->sg.id = 2;
song2->sg.name = "Song 2";
song2->sg.singerName = "Blur";

addSong(list, song2);

Then the two functions

SongNode *getLastSong(SongDoublyLinkedList *songList)
{
  if (songList->firstElement == NULL) return NULL;

  SongNode currentElement = *(songList->firstElement);

  while (currentElement.nextNode != NULL)
  {
      currentElement = *(currentElement.nextNode);
  }

  return &currentElement;
}

void addSong(SongDoublyLinkedList *songList, SongNode *songNd)
{
  songNd->nextNode = NULL;
  songNd->previousNode = NULL;

  SongNode* lastNode = getLastSong(songList);

  if (lastNode == NULL) songList->firstElement = songNd;
  else lastNode->nextNode = songNd;
}

The issue is in return &currentElement; in getLastSong when adding the second element, but I don't understand what the issue is. It's almost acting like the memory is being free, but isn't. This initially made me think I was returning the wrong value, but I ran it through the Visual Studio debugger and the pointer seems to work fine within the getLastSong function, but not outside of its scope.

Value Good:

Value Bad:

Also, if I try to dereference the bad `lastNode' then a get some manner of memory access error, so it really seems like I'm sending a bad value.

I assume I'm missing something silly (or more likely, I've forgotten an element of C++ after having worked with C# for years), why isn't this working the way I expect?

@RaymondChen But isn't currentElement pointing to a memory address for something that exists outside of that function, the SongNode created in main? Or have I confused myself somewhere — cost, May 01 '15 at 23:08
@RaymondChen Wasn't so much a duplicate of the problem, but rather I was misunderstanding part of how C++ works elsewhere — cost, May 01 '15 at 23:13
Can you use `std::map` or `std::list` instead of deriving your own data structure? These have already been debugged, so you won't have to waste your time rewriting and testing them. — Thomas Matthews, May 01 '15 at 23:34
@ThomasMatthews I'm assisting a friend with a programming assignment, which is to make a linked list. — cost, May 02 '15 at 00:24

Peter · Answer 1 · 2015-05-02T03:30:03.960

5

currentElement is an automatic (in the sense of the original meaning of the auto keyword, or non-static) variable that is local to getLastSong() so ceases to exist when that function returns.

To expand, as aschepler pointed out in comments, the declaration SongNode currentElement = *(songList->firstElement); causes currentElement to be a copy of *(songList->firstElement).

The scope in which currentElement exists (i.e. as a copy of the *(songList->firstElement)) is in the function, so it ceases to exist when the function returns.

Returning its address means returning the address of a non-existent object to the caller. If the caller tries to use that address (i.e. dereference it) the result is undefined behaviour.

You have demonstrated - both in your original question, and in how you misinterpreted my reply before editing - the danger of trying to reason about C++ in terms of your C# understanding. C# and C++ are very different languages, even if they have syntactic similarities. And this is one area where, even if the code might look similar, it behaves very differently.

edited May 02 '15 at 03:30

answered May 01 '15 at 23:03

Peter

35,646
4
32
74

But isn't `currentElement` pointing to a memory address for something that exists outside of that function? – cost May 01 '15 at 23:07
4

@cost No, because it's not a pointer. `SongNode currentElement = *(songList->firstElement);` makes a local copy of the `SongNode`. `SongNode* currentElement = songList->firstElement;` would create a pointer to the long-term node, and probably be more useful to that function. – aschepler May 01 '15 at 23:08
1

@aschepler Ahh, I didn't realize it made a copy in that instance, thank you – cost May 01 '15 at 23:10
@aschepler I think I got stuck in the C# mindset that everything is a reference, and I could move back and forth between `SongNode` and `SongNode*` indiscriminately, thanks for setting me straight. That was the answer I needed – cost May 01 '15 at 23:20
@cost Since this seemingly answered your question, mark it as the answer. :-) – Khaldor May 02 '15 at 00:17
@Khaldor It didn't really though, it was aschepler's comment that answered it for me... – cost May 02 '15 at 00:25
@Khaldor I made a mistake in how C++ handles addresses, not losing the variable because it went out of scope. The lost variable was just a side effect of my real mistake, which aschepler pointed out – cost May 02 '15 at 00:30
added in the edit including that comment. Once accepted by @Peter it should thus be possible to consider it as the answer. – Khaldor May 02 '15 at 00:30
Even if `currentElement` were declared as `SongNode* currentElement`, you still cannot `return &currentElement;` - that returns the address of a local variable. – Raymond Chen May 02 '15 at 00:41
@RaymondChen I realized my various other mistakes once aschepler pointed out my big mistake. Now that my understanding is in order, I've fixed the rest of the code too :) – cost May 02 '15 at 00:43
As a matter of fact, cost, my response did answer your question. You misinterpreted my answer because you were thinking in C# terms. You are incorrect in dismissing my observation about the variable passing out of scope, because that is the root cause of your problem. Anyway, I'll update the post to expand shortly. – Peter May 02 '15 at 03:22

Pointer returned from a function isn't valid

1 Answers1