libc++ patch for LWG Issue 2232 - dangling reference for regex_iterators

[David Blaikie]

On Wed, Feb 19, 2014 at 9:17 AM, Marshall Clow <mclow.lists at gmail.com>wrote:

> On Feb 14, 2014, at 4:31 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Fri, Feb 14, 2014 at 4:21 PM, Marshall Clow <mclow.lists at gmail.com>
>  wrote:
>
>> Users can write
>>         for(sregex_iterator i(s.begin(), s.end(), regex("meow")), end; i
>> != end; ++i)
>>
>> binding a temporary regex to const regex& and storing a pointer to it.
>> This will compile silently, triggering undefined behavior at runtime.
>>
>> Fixing this involves defining constructors for the various regex iterator
>> types from rvalue regexes, and then marking them as "deleted".
>> And tests.
>>
>
> Would this break (what I assume is) correct code doing things like:
>
> std::find(sregex_iterator(s.begin(), s.end(), regex("meow")), end, "foo");
>  ?
>
>
> (Recapping a discussion on IRC)
>
> Yes, it would.
> And though that code is "technically correct" it (a) is dangerous, and (b)
> doesn't do anything useful.
>
> The return result from std::find is an interator into a destroyed object,
> so you can't really do anything with that.
>
> The "fix" for this code is simple:
>
> regex re{"meow"}
> std::find(sregex_iterator(s.begin(), s.end(), re), end, "foo");
>
> and now the return value of std::find is valid (until the end of the
> lifetime of "re").
>

Fair point about find, though it's not too hard to find examples that take
iterators and don't return temporaries from them.

There are lots of APIs that take references to things and return references
with the same lifetime as the parameter and would break in similar
situations, but perhaps these particular APIs are substantially more prone
to this kind of problem and worthy of a specific safety guard.


> See
> http://cplusplus.github.io/LWG/lwg-active.html#2329
> and http://cplusplus.github.io/LWG/lwg-active.html#2332
>
> for the changes to the (C++14) standard.
>

In any case, this was entirely a misunderstanding on my part - not
realizing (though you'd mentioned in the subject) that this was a new,
deliberately non-strictly-backwards-compatible change mandated by the next
standard. I'd mistakenly thought this was an attempt to add some
standard-allowable-but-not-mandated safety so I was confused why it would
break existing correct callers. I'm still a little surprised at this being
worth a non-backward-compatible change in the standard, but that's up to
you and the rest of the committee and I didn't mean to get in the way of
whatever decision came out of that.

A few minor, optional, comments on the patch itself:

* An extra explicit scope in each of the test cases seems strange, but
perhaps there's a reason for it given how consistently you've done that.

* The "= delete" ctors you've added aren't preprocessor protected (just "//
C++14") - why's that? Won't this break C++11? Or was this change accepted
as a DR to '11?

* The version tests of "> 11" and "<= 11" seem a bit harder to read (to me)
than ">= 14" and "< 14" which speaks to the actual version the feature was
introduced in rather than having to mentally compute that from the prior
version.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140219/096a43ea/attachment.html>