1

I am writing some code that loads the contents of an XML file into a string, and parsing it using RapidXML. I can successfully do this, but I have came back to clean my code up and came across a problem when making sure I have no memory leaks.

The RapidXML parse function wants a char*, so I cannot pass string.c_str(), so I am copying the contents of the string into a char* array. If I delete the array using delete[], and then pass the array into the parsing function I can access all the data inside the string as if it was never deleted. I can even print the array out, surely this isn't possible? Am I not seeing something glaringly obvious? 

    std::string file = loadFile(filePath);

    char* buffer = new char[file.size() + 1];
    std::copy(file.begin(), file.end(), buffer);
    buffer[file.size()] = '\0';

    delete[] buffer;

    rapidxml::xml_document<char> m_currentFile;
    m_currentMap.parse<0>(buffer);
martingrant
  • 164
  • 1
  • 12
  • Keep in mind that RapidXML does not make copies of the data you pass it, it just maintains a pointers and size information. Once you delete the data you pass parse `parse` the document becomes invalid. – Captain Obvlious Oct 08 '15 at 19:32
  • 2
    Just because you *can* doesn't mean you *should*. This is how you create catastrophic bugs. **Don't do it**. Deleting an object doesn't erase the memory. It just invalidates the pointer. That data *may* be overwritten by another operation. It's like keeping the keys to an apartment after you've terminated the lease and moved out. You can still go back in there and have a shower, but you could get in trouble. – tadman Oct 08 '15 at 19:32
  • I know it is not the same situation but it explains why – NathanOliver Oct 08 '15 at 19:32
  • the worst thing about undefined behaviour is that sometimes you dont notice it and everything seems to work fine, but still it is undefined behaviour – 463035818_is_not_an_ai Oct 08 '15 at 19:35
  • @tadman Yeah I understand that, I had never come across that situation before. I was testing the delete was properly working, and assumed that if I attempted to access the array either I would have no access, or there would be nothing in it! Good to know though, thanks. – martingrant Oct 08 '15 at 19:39
  • One of the things that makes writing correct C++ code challenging is keeping track of the state of your pointers and iterators, that when they're invalidated you're not prevented from using them by the compiler so mistakes are possible. You need to be extremely disciplined about never using a pointer to a deleted object or an iterator you've invalidated. Many of these slips lead to *undefined behaviour* which is a way of saying "often infuriatingly difficult to reproduce bugs". – tadman Oct 08 '15 at 19:43
  • If you wonder why people will brow-beat you about using Standard Library containers like `std::string` instead of character pointers this is why. I'd try and use `c_str()` even if it involves a potentially ugly recast from `const char*` because it's not going to lead to out-of-order de-allocation problems. Objects that embrace the philosophy of being inexpensive to copy and passing references instead will largely avoid all this fuss with pointers. – tadman Oct 08 '15 at 19:47
  • For sure! In my example code, once I am finished using that array and call delete[] can I safely assume I am avoiding any memory leaks? (the delete is before the parse function, but obviously it would be moved to come after the parse) and move on? If I attempted to access it is possible the data would be still there, but that will be cleaned up by the OS at some point later on? – martingrant Oct 08 '15 at 19:49
  • @tadman: Instead of `const_cast(s.c_str())`, just use `&s[0]` which is already non-const – Ben Voigt Oct 08 '15 at 19:54
  • @BenVoigt Yeah, there's a few ways to do that, and if that works, certainly an option worth considering. Honestly, the XML parser should get with the program and take a `const char*` argument, but you know, portable C tends to be very conservative about that sort of thing. – tadman Oct 08 '15 at 19:56
  • I think I attempted to cast from const char to char* and I done it incorrectly and assumed I couldn't in my situation. I just checked again and it's working, man I could have avoided all this with such a simple solution! Thanks all for the answers though, I learned a lot from it! :) – martingrant Oct 08 '15 at 19:59

1 Answers1

2

When you delete an array, all you do is mark that part of memory as available, but the data isn't actually removed. Here, you're getting lucky: nothing has overwritten that data yet but you can't rely on that. At some point malloc/new will grant some other part of your program access to that memory and it will get overwritten.

Oliver Dain
  • 9,617
  • 3
  • 35
  • 48
  • Okay. So after I am done parsing the file, I can call delete[] safely and move on? I was just trying to clean my code up and put the delete call before a function call that uses the array to assure myself it was actually deleting correctly, which let me to post about the behaviour here. – martingrant Oct 08 '15 at 19:38
  • yes. Once you're done using the array you can, and should, delete it. – Oliver Dain Oct 08 '15 at 19:49
  • BTW: I recommend using smart pointers (unique_ptr mostly, but shared_ptr when it makes sense). That keeps the semantics really clear about who owns an object and when it can be freed and it frees you from having to remember to free things to prevent memory leaks. – Oliver Dain Oct 08 '15 at 19:51
  • Great, good to know - thanks! Yeah I am a big fan of smart pointers. I can never remember the code to read from files so always use one of the top results from a google search. Pretty much all file reading examples always use a more C style way of accessing files so I usually just go with that. – martingrant Oct 08 '15 at 19:54