CODEWITHSUNDEEP
c++stl
Peter
Peter

Reputation: 11920

C++ strings - How to avoid obtaining invalid pointer?

In our C++ code, we have our own string class (for legacy reasons). It supports a method c_str() much like std::string. What I noticed is that many developers are using it incorrectly. I have reduced the problem to the following line:

 const char* x = std::string("abc").c_str();

This seemingly innocent code is quite dangerous in the sense that the destructor on std::string gets invoked immediately after the call to c_str(). As a result, you are holding a pointer to a de-allocated memory location.

Here is another example:

  std::string x("abc");
  const char* y = x.substr(0,1).c_str();

Here too, we are using a pointer to de-allocated location.

These problems are not easy to find during testing as the memory location still contains valid data (although the memory location itself is invalid).

I am wondering if you have any suggestions on how I can modify class/method definition such that developers can never make such a mistake.

Upvotes: 17

Views: 2710

Answers (6)

CaptainCodeman
CaptainCodeman

Reputation: 2221

The simplest solution would be to change your destructor to write a null at the beginning of the string at destruction time. (Alternatively, fill the entire string with an error message or 0's; you can have a flag to disable this for release code.)

While it doesn't directly prevent programmers from making the mistake of using invalid pointers, it will definitely draw attention to the problem when the code doesn't do what it should do. This should help you flush out the problem in your code.

(As you mentioned, at the moment the errors go unnoticed because for the most part the code will happily run with the invalid memory.)

Upvotes: 6

isarandi
isarandi

Reputation: 3349

The modern part of the code should not deal with raw pointers like that. Call c_str only when providing an argument to a legacy function that takes const char*. Like:

legacy_print(x.substr(0,1).c_str())

Why would you want to create a local variable of type const char*? Even if you write a copying version c_str_copy() you will just get more headache because now the client code is responsible for deleting the resulting pointer.

And if you need to keep the data around for a longer time (e.g. because you want to pass the data to multiple legacy functions) then just keep the data wrapped in a string instance the whole time.

Upvotes: 9

Steger
Steger

Reputation: 906

Consider using Valgrind or Electric Fence to test your code. Either of these tools should trivially and immediately find these errors.

Upvotes: 3

mclaassen
mclaassen

Reputation: 5138

You could remove the c_str() function and instead provide a function that accepts a reference to an already created empty smart pointer that resets the value of the smart pointer to a new copy of the string. This would force the user to create a non temporary object which they could then use to get the raw c string and it would be destructed and free the memory when exiting the method scope.

This assumes though that your library and its users would be sharing the same heap.

EDIT

Even better, create your own smart pointer class for this purpose whose destructor calls a library function in your library to free the memory so it can be used across DLL boundaries.

Upvotes: 1

o11c
o11c

Reputation: 16126

For the basic case, you can add a ref qualifier on the "this" object, to make sure that .c_str() is never immediately called on a temporary. Of course, this can't stop them from storing in a variable that leaves scope before the pointer does.

const char *c_str() & { return ...; }

But the bigger-picture solution is to replace all functions from taking a "const char *" in your codebase with functions that take one of your string classes (at the very least, you need two: an owning string and a borrowed slice) - and make sure that none of your string class does cannot be implicitly constructed from a "const char *".

Upvotes: 7

Fantastic Mr Fox
Fantastic Mr Fox

Reputation: 33904

I am not sure that there is much you can do about people using your library incorrectly if you warn them about it. Consider the actual stl string library. If i do this:

const char * lala = std::string("lala").c_str();
std::cout << lala << std::endl;
const char * lala2 = std::string("lalb").c_str();
std::cout << lala << std::endl;
std::cout << lala2 << std::endl;

I am basically creating undefined behavior. In the case where i run it on ideone.com i get the following output:

lala
lalb
lalb

So clearly the memory of the original lala has been overwritten. I would just make it very clear to the user in the documentation that this sort of coding is bad practice.

Upvotes: 1

Related Questions