Subject: Breaking: Deadly parasite infects thousands

From: Disease Control <[email-masked]>
Date: Tue, 10 Jun 2014 11:16:28 -0700

..No one thought it could happen in North America.

Reports indicate that more then (250-million) Americans have this deadly parasite.

The truth the government does NOT want you to know:
http://www.agencyright.com/ama-breaking-US-yahoo-news/6543734564.html

It explains how this parasite is responsible for many of your common and persistent health conditions.

Stay Healthy,
Craig Cappetta
Director of nutrition and science

PS- This is something every American needs to know about.
Please spread the word ASAP: The crisis is real.
http://www.agencyright.com/ama-breaking-US-yahoo-news/6543734564.html

Inf-from| AMA Disease Control 1.0.8.1.5. s. Lowell | Bahama
nc 2 7 5 0 3 - settgs/pref- http://www.agencyright.com/i7.i7-out ]

/
/
/
/
/
/
,
,
,
,
Other-Internal Top Stories | 
Death toll from Ebola in Sierra Leone more than doubles

||||||||||||||||||||||||||||||
But what about the following example?

int main()
{
  // initialises an empty vector.
  std::vector<int> v = {};

  // removes all elements with the value 5
  v.erase( std::remove( v.begin(), v.end(), 5 ), v.end() );

  return 0;
}
This seems to work as expected (not erasing anything, not segfaulting, etc.) on all platforms I run it on, but I know that just because something is working, doesn't mean it's not undefined behavior.

The quick reference for vector.erase says this (emphasis mine):

iterator erase (const_iterator first, const_iterator last);
first, last are

Iterators specifying a range within the vector] to be removed: [first,last). i.e., the range includes all the elements between first and last, including the element pointed by first but not the one pointed by last. Member types iterator and const_iterator are random access iterator types that point to elements.

So is vector.erase(vector.end(),vector.end()) undefined behavior?

Here's what the quick reference says about exception safety:

If the removed elements include the last element in the container, no exceptions are thrown (no-throw guarantee). Otherwise, the container is guaranteed to end in a valid state (basic guarantee). An invalid position or range causes undefined behavior.

So, the answer, at least to me appears to be "YES", and this StackOverflow answer seems to support it.

Therefore, is the common idiom wrong?

Assuming it's undefined behavior, then any call to remove could return an iterator to vector.end() which should be checked before calling vector.erase, and calling remove on an empty vector does seem to return vector.end: (IDEOne for code below)

#include <iostream>
#include <algorithm>
#include <vector>
using namespace std;

int main() {
   vector<int> myInts;
   auto anIter = std::remove(myInts.begin(),myInts.end(),5);
   if (anIter == myInts.end())
      std::cout << "iterator = myInts.end()";
}
Finally, my question:

Should the actual remove/erase idiom be this?

auto endOfRangeIterator = std::remove(vector.begin(), vector.end(), <value>);
if (endOfRangeIterator != vector.end())
   vector.erase(endOfRangeIterator, vector.end())
c++ algorithm c++11 vector stl
share|improve this question
edited May 30 at 0:03

asked May 20 at 13:31

AndyG
3,2611526
1	  	
No, an empty range is fine. [first, first) is an empty set.   BoBTFish May 20 at 13:36
9	  	
cplusplus.com I think we've found the source of your problem.   user657267 May 20 at 13:51
1	  	
@privatedatapublicchannel2: Actually, the quotation is not correct. In the case where first==last, the quote contradicts itself, specifying that an element is removed, but not removed. Which explains why AndyG came to believe it was undefined behavior.   Benjamin Lindley May 20 at 14:13 
1	  	
@privatedatapublicchannel2: It's not the same. The standard doesn't actually say that the element pointed to by i is included in the range. It just says that's where the range starts. It does, however, definitely say that the element pointed to by j is not included. Also, the second iterator passed to erase does not necessarily have to be the end iterator, so if first==last, that does not necessarily imply that first points to end.   Benjamin Lindley May 20 at 14:24 
1	  	
@privatedatapublicchannel2: To be clear, read the quote from cplusplus.com, replacing "last" with "first". "including the element pointed to by first, but not the one pointed to by first". The statement is clearly a contradiction, in that case.   Benjamin Lindley May 20 at 14:29 
show 11 more comments
3 Answers
activeoldestvotes
up vote
24
down vote
accepted
24.2.1/7 Most of the librarys algorithmic templates that operate on data structures have interfaces that use ranges. A range is a pair of iterators that designate the beginning and end of the computation. A range [i,i) is an empty range; in general, a range [i,j) refers to the elements in the data structure starting with the element pointed to by i and up to but not including the element pointed to by j.

Emphasis mine.

Further, the description of erase you cite is not the normative text in the standard. The standard has this to say (Table 100):

a.erase(q1,q2)

Effects: Erases the elements in the range [q1, q2).

This doesn't require that q1 be dereferenceable. If [q1, q2) is an empty range (per 24.2.1/7), then no elements are in the range, and so none are erased.

share|improve this answer
answered May 20 at 13:43

Igor Tandetnik
10.2k1617
  	  	
Thank you for the clarification. The definition of empty range according to the standard does wonders to clear things up; otherwise I think it would be ambiguous.   AndyG May 20 at 14:14
add comment

up vote
4
down vote
So is vector.erase(vector.end(),vector.end()) undefined behavior?

No. Because of the statement right next to the one you emphesized:

Iterators specifying a range within the vector] to be removed: [first,last). i.e., the range includes all the elements between first and last, including the element pointed by first but not the one pointed by last.

So, vector.erase(vector.end(),vector.end()) does not attempt to erase vector.end() because it is pointed to by parameter last.

Granted, this definition is ambiguous and those statements can be interpreted as contradictory. The quoted wording is not used by the standard.

share|improve this answer
edited May 20 at 13:53

answered May 20 at 13:43

user2079303
2,512514
add comment
up vote
4
down vote
I think the more important in your cite is:

Iterators specifying a range within the vector] to be removed: [first,last). i.e., the range includes all the elements between first and last, including the element pointed by first but not the one pointed by last. Member types iterator and const_iterator are random access iterator types that point to elements.

As we have found in comments, this citation from cpluspluc.com is incorrect. This will not violate the rules in case of ( v.end, v.end) but will be incorrect in case of

#include <vector>

int main()
{
    std::vector<int> v = { 1, 2, 3 };

    v.erase( v.begin(), v.begin());
}
because statement that contradicts itself with

the range includes (...), including the element pointed by v.begin() but not the one pointed by v.begin().

cannot be a valid statement.

C++ Standard n3337 in  23.2.2 Sequence containers requirements Table 100 specifies that

a.erase(q1,q2) returns iterator . And note is:

Requires: For vector and deque, T shall be MoveAssignable. Effects: Erases the elements in the range [q1, q2).

And this is what it says about the range [i,j) in  24.2.1/7 Iterator requirements

Most of the librarys algorithmic templates that operate on data structures have interfaces that use ranges. A range is a pair of iterators that designate the beginning and end of the computation. A range [i,i) is an empty range; in general, a range [i,j) refers to the elements in the data structure starting with the element pointed to by i and up to but not including the element pointed to by j. Range [i,j) is valid if and only if j is reachable from i. The result of the application of functions in the library to invalid ranges is undefined.

Thus to answer your questions

But what about the following example?

cplusplus.com is wrong in this case

So is vector.erase(vector.end(),vector.end()) undefined behavior?

No, no undefined behavior is triggered.

Therefore, is the common idiom wrong?

No, it is correct.

Should the actual remove/erase idiom be this?

There is no need for this, though it is also fine.
n 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. Regards.

c++ stl
share|improve this question
asked May 27 at 0:26

Peter
95741025
  	  	
There are similar discussions here: stackoverflow.com/questions/10540157/   Ben May 27 at 0:33
3	  	
The solution is for the developers to think carefully before calling c_str(), and stop using char * in their code. Perhaps you could review why they are using pointers in their code instead of just using strings, and advise other solutions.   Matt McNabb May 27 at 0:50 
add comment
6 Answers
activeoldestvotes
up vote
7
down vote
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.

share|improve this answer
edited May 27 at 1:57

answered May 27 at 0:45

isarandi
619210
add comment

up vote
6
down vote
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 *".

share|improve this answer
answered May 27 at 0:46

o11c
1462
5	  	
note: requires C++11   Matt McNabb May 27 at 0:51
1	  	
And requires providing a deleted && variant if a const& variant is provided. And VS2013 doesn't handle it. But I like it for preventing this at compile-time.   Michael Urman May 27 at 1:23
  	  	
Excuse my ignorance, but how would you add this, without modifying the string header file itself?   LordAro May 27 at 9:02
1	  	
@LordAro And why not modify it? The OP states they're using their own string class, not std::string.   Angew May 27 at 11:37
add comment
up vote
5
down vote
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.)

share|improve this answer
answered May 27 at 7:08

CaptainCodeman
3881310
add comment
up vote
2
down vote
Consider using Valgrind or Electric Fence to test your code. Either of these tools should trivially and immediately find these errors.

share|improve this answer
answered May 27 at 1:53

Steger
28717
add comment
up vote
1
down vote
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.

share|improve this answer
answered May 27 at 0:45

Ben
3,32541553
add comment
up vote
1
down vote
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.

share|improve this answer
edited May 27 at 1:22

answered May 27 at 1:09

mclaassen
1,206316
1	  	
Give an example of how you remove c_str function?   texasbruce May 27 at 1:19
3	  	
I thought he was looking to modify the class definition, I think the c_str function is his own class's function, not the actual std::string c_str