Opened 9 years ago

Last modified 9 years ago

#8489 new Bugs

Unnecessary Definition of iterator_adaptor.hpp class Has Dangerous Return of Temporary Object

Reported by: Jeffrey D. Oldham <oldham@…> Owned by: jeffrey.hellrung
Milestone: To Be Determined Component: iterator
Version: Boost 1.53.0 Severity: Problem
Keywords: Cc: gromer@…

Description

Change definition of private iterator_adaptor<...>::dereference to a declaration, avoiding illegal return of local temporary object:

error: returning reference to local temporary object [-Werror,-Wreturn-stack-address]

{ return *m_iterator; }

Changing to a declaration is semantically acceptable because the preceding comment and a code search shows that the function was made private to ensure it is not used in this base class.

This was revealed using Clang++ 3.0.6. It is a showstopper for people required to use Clang with warnings turned in errors.

Attachments (2)

iterator_adaptor.hpp.20130423.patch (489 bytes ) - added by Jeffrey D. Oldham <oldham@…> 9 years ago.
A patch resolving the problem.
iterator-adaptor.error-program.20130423.cc (761 bytes ) - added by Jeffrey D. Oldham <oldham@…> 9 years ago.
Program illustrating the error.

Download all attachments as: .zip

Change History (10)

by Jeffrey D. Oldham <oldham@…>, 9 years ago

A patch resolving the problem.

by Jeffrey D. Oldham <oldham@…>, 9 years ago

Program illustrating the error.

comment:1 by Jeffrey D. Oldham <oldham@…>, 9 years ago

Summary: Unnecessary Definition of ??? Has Dangerous Return of Temporary ObjectUnnecessary Definition of iterator_adaptor.hpp class Has Dangerous Return of Temporary Object

comment:2 by gromer@…, 9 years ago

Cc: gromer@… added

comment:3 by oldham@…, 9 years ago

The patch breaks iterator_facade.hpp::iterator_core_access::dereference() so it should not be used. However, the problem still exists for the test case.

comment:4 by oldham@…, 9 years ago

It is not clear if the program showing the error is itself correct.

comment:5 by oldham@…, 9 years ago

If my logic is correct, the return type for iterator_adaptor::dereference() is:

  • Reference if Reference != use_default,
  • iterator_reference<Base> if Reference == use_default and Value == use_default,
  • add_reference<Value> if Reference == use_default and Value != use_default.

I conjecture

  • the second case is not causing problems and
  • the third case is causing problems for the example program. For a slight variant of the program, I believe Value == const char and Reference == use_default. Thus, it tries to return a const char& which causes a local temporary to be created and then a reference returned.

One possible implementation solution is to specialize iterator_adaptor<> so dereference has a declaration with no definition for the third case. Another is to modify iterator_adaptor_base so problematic third cases do not occur.

It is not clear what to do.

comment:6 by james.hirschorn@…, 9 years ago

I originally thought this was a bug too, but I do not believe it is.

First of all, the dereference member function of iterator_adaptor does not need to be valid (it is clearly invalid in your specific example). You can simply redefine dereference in your iterator class which inherits from iterator_adaptor, and the one from the base class will be ignored by the compiler.

Secondly, I supsect there may be a problem with the operator*() of your Base class. It is returning by value, but normally operator*() returns a reference.

comment:7 by Dave Abrahams, 9 years ago

The problem here is that the underlying iterator of the iterator_adaptor is an istreambuf_iterator<char>, which quite legitimately (for an input iterator) returns a non-reference type from its operator*. This should be filed as a bug against Spirit.

To debug it, I applied this patch to iterator_adaptor.hpp, and ran the failing program through stlfilt:

  • iterator_adaptor.hpp

    old new  
    1818#include <boost/mpl/and.hpp>
    1919#include <boost/mpl/not.hpp>
    2020#include <boost/mpl/or.hpp>
     21#include <boost/mpl/assert.hpp>
    2122
    2223#include <boost/type_traits/is_same.hpp>
    2324#include <boost/type_traits/is_convertible.hpp>
     25#include <boost/type_traits/is_reference.hpp>
    2426
    2527#ifdef BOOST_ITERATOR_REF_CONSTNESS_KILLS_WRITABILITY
    2628# include <boost/type_traits/remove_reference.hpp>
     
    303305      // base_reference(), above, to get direct access to m_iterator.
    304306      //
    305307      typename super_t::reference dereference() const
    306         { return *m_iterator; }
     308      {
     309          BOOST_MPL_ASSERT((
     310              mpl::or_<
     311                  mpl::not_<is_reference<typename super_t::reference> >
     312                , is_reference<
     313#ifdef BOOST_NO_CXX11_DECLTYPE
     314                    typename boost::iterator_reference<Base>::type
     315#else
     316                    decltype(*m_iterator)
     317#endif
     318                  >
     319              >));
     320          return *m_iterator;
     321      }
    307322
    308323      template <
    309324      class OtherDerived, class OtherIterator, class V, class C, class R, class D

comment:8 by Dave Abrahams, 9 years ago

I take it back; this isn't a Spirit bug… yet. The error is in the user program. As http://www.boost.org/doc/libs/1_53_0/libs/spirit/doc/html/spirit/support/multi_pass.html makes quite clear, Spirit works only with forward iterators. It's not clear at all, however, why I'm unable to solve the problem by wrapping the istreambuf_iterator with Spirit's multi_pass. See #3999

Last edited 9 years ago by Dave Abrahams (previous) (diff)
Note: See TracTickets for help on using tickets.