以下のrange-based forは危険でしょうか。

Question

以下の記事を読んでいたところ
https://qiita.com/rinse_/items/ad0cc7e351e836595c94

// 危険なコード
for( auto e : something().get_vector() ) // get_vectorは内部に持つvectorへの参照を返す
{
  // e is destroyed
}

// 安全なコード
for( auto e : std::vector < int > { 1,2,3,4,5,6,7,8,9,0 } )
{
  // e is 1,2,3,...,0
}

range-based forでは、コンテナへの参照をキャプチャします。上の危険な例ではキャプチャしたものがすぐに破棄されてしまいますが、下の安全な例では、一時オブジェクトをキャプチャして延命しているので安全に利用することができるのです。

という文言があったため、試してみたところ、正しく動いているように見えました。
これはなぜ動いてしまうのでしょう。
環境はVisual Studio 2015です。

また、

#include <iostream>	
#include <fstream>
#include <string>
#include <vector>

using namespace std;

auto retVec(vector<string> original)
{
	return original;
}

struct str
{
	vector<string> _vec
	{
		"first",
		"second",
		"third"
	};
public:
	auto get_vec() { return _vec; };
};

int main()
{
	for (auto e : str().get_vec())	// 危険?
	{
		cout << e << endl;
	}

	str str_exist;
	for (auto e : str_exist.get_vec())	// これも危険?
	{
		cout << e << endl;
	}
}

2つ目のrange-based forは危険でしょうか？

【追記】

#include <iostream>	
#include <fstream>
#include <string>
#include <vector>

using namespace std;


struct str
{
	vector<int> _vec
	{
		0,
		1,
		2
	};
public:
	auto& get_vec() { return _vec; }
};

int main()
{
	for (auto e : str().get_vec())
	{
		cout << e << endl;
	}
}

上記なら危険なのですね。

Comments

[t_obara]

とりあえず、参照されているページと同様、デストラクタを印字して確かめてみてはいかがですか？

Answer 1

前提

まずRange-based forがどのように展開されるか確認しましょう。

§9.5.4 [stmt.ranged]/1
The range-based for statement

for ( for-range-declaration : for-range-initializer ) statement

is equivalent to

{
auto &&__range = for-range-initializer ;
auto __begin = begin-expr ;
auto __end = end-expr ;
for ( ; __begin != __end; ++__begin ) {
for-range-declaration = *__begin;
statement
}
}

そしてそれぞれ見ていきましょう。

1つ目の例

for( auto e : something().get_vector() ) // get_vectorは内部に持つvectorへの参照を返す
{
  // e is destroyed
}

危険です。__range の型はstd::vector<int>&になります。このときsomething()の寿命は尽きています。つまり存在しないオブジェクトを束縛しています。

https://wandbox.org/permlink/v0c1cR7NzOfaPydd

Linux上でAddressSanitizerに掛けると

./a.out 
destructor
=================================================================
==5022==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7ffd5b3fe1e0 at pc 0x0000004c70dd bp 0x7ffd5b3fe070 sp 0x7ffd5b3fe068
READ of size 8 at 0x7ffd5b3fe1e0 thread T0
    #0 0x4c70dc in __gnu_cxx::__normal_iterator<int*, std::vector<int, std::allocator<int> > >::__normal_iterator(int* const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/stl_iterator.h:783:20
    #1 0x4c5eed in std::vector<int, std::allocator<int> >::begin() /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/stl_vector.h:564:16
    #2 0x4c57f2 in main /home/yumetodo/teratail/229984/example1/main.cpp:17:15
    #3 0x7f69dc193b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #4 0x41b659 in _start (/home/yumetodo/teratail/229984/example1/a.out+0x41b659)

Address 0x7ffd5b3fe1e0 is located in stack of thread T0 at offset 32 in frame
    #0 0x4c558f in main /home/yumetodo/teratail/229984/example1/main.cpp:15

  This frame has 5 object(s):
    [32, 56) 'ref.tmp' (line 17) <== Memory access at offset 32 is inside this variable
    [96, 112) 'ref.tmp1' (line 17)
    [128, 168) 'ref.tmp2' (line 17)
    [208, 216) '__begin1' (line 17)
    [240, 248) '__end1' (line 17)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-scope /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/stl_iterator.h:783:20 in __gnu_cxx::__normal_iterator<int*, std::vector<int, std::allocator<int> > >::__normal_iterator(int* const&)
Shadow bytes around the buggy address:
  0x10002b677be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002b677bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002b677c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002b677c10: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f3 f3 f3
  0x10002b677c20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x10002b677c30: 00 00 00 00 00 00 00 00 f1 f1 f1 f1[f8]f8 f8 f2
  0x10002b677c40: f2 f2 f2 f2 f8 f8 f2 f2 f8 f8 f8 f8 f8 f2 f2 f2
  0x10002b677c50: f2 f2 00 f2 f2 f2 f8 f3 f3 f3 f3 f3 00 00 00 00
  0x10002b677c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002b677c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10002b677c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==5022==ABORTING

のように言われます。(MicrosoftがMSVCに試験実装したASanは何も言ってこない、Releaseビルドでしか使えないから最適化でUBだから吹き飛んでるので見つからないんじゃないかと推測)

ちなみにメンバー関数の宣言時にref qualifierをつけるとコンパイル時にこのようなことを防げます。

std::vector < int >& get_vector() & { return ( _Value ); }

prog.cc: In function 'int main()':
prog.cc:17:62: error: passing 'something' as 'this' argument discards qualifiers [-fpermissive]
   17 |   for( auto e : something { 1,2,3,4,5,6,7,8,9,0 }.get_vector() ) // get_vectorは内部に持つvectorへの参照を返す
      |                                                              ^
prog.cc:10:24: note:   in call to 'std::vector<int>& something::get_vector() &'
   10 |   std::vector < int >& get_vector() & { return ( _Value ); }
      |                        ^~~~~~~~~~

https://wandbox.org/permlink/eRayizx8KL07dgja

2つ目の例

  for( auto e : std::vector < int > { 1,2,3,4,5,6,7,8,9,0 } )
  { // 一時オブジェクトはキャプチャされ延命されている
  	std::cout << e;
  }

安全です。__range の型はstd::vector<int>&&になります。このとき一時オブジェクトはrvalue referenceによって束縛されているので寿命が延長されます。

https://wandbox.org/permlink/C6vnwrPNcDHazhot

3つ目の例

    for (auto e : str().get_vec())    // 危険?
    {
        cout << e << endl;
    }

安全です。ここでget_vecの戻り値の型がstd::vector<std::string>であることに注意してください。つまりこの関数はメンバー変数の参照ではなくてコピーを返しています。したがって2つめの例と同様のことが起こります。

https://wandbox.org/permlink/cUE16XoExbGq6X1T

4つ目の例

    str str_exist;
    for (auto e : str_exist.get_vec())    // これも危険?
    {
        cout << e << endl;
    }

安全です。3つ目と同様なので割愛します。

Comments

[yohhoy]

> ちなみにメンバー関数の宣言時にref qualifierをつけるとコンパイル時にこのようなことを防げます。
好ましいかといわれると微妙(やりすぎ)な設計な気もしますが、ref qualifierによるオーバーロードを活用して左辺値からはデータメンバへの左辺値参照を／右辺値からはムーブされたデータメンバを返すこともできますね。
https://wandbox.org/permlink/7fFSaBCQm81kOKsP

[yumetodo]

ああ、そういえばreturn文でのstd:moveは常に間違ってるってずっと説明してきたのですが、この例では正しい用法になるのか・・・。

[yohhoy]

「return文での"通常ローカル変数に対する"std:moveは常に間違ってる」なら妥当とは思います。
暗黙のムーブになる条件やNRVO(copy elision)阻害条件はそれなりに複雑ですね。
https://timsong-cpp.github.io/cppwp/n4659/class.copy.elision

[__ook]

回答ありがとうございます…
なるほど、僕が真似た(つもりだった)コードはそもそもコピーを返しているから安全なのですね、読み違えていました。

[yumetodo]

まあそういうわけで、
https://teratail.com/questions/228746
での回答は一部撤回ですね。

Answer 2

結論からいえば、質問文中コードの場合は両range-based forともに「安全」です。

struct str
{
   // (略)
   auto get_vec() { return _vec; };
};

strクラス(構造体)get_vecメンバ関数が、データメンバ_vecへの参照ではなくコピー結果を一時オブジェクトとして返すインタフェースのため、参考にされたQiita解説記事とは異なる結果になっています。

---

もし、get_vecメンバ関数がデータメンバへの参照を返すインタフェース（例えば戻り値型がauto&）だとすると：

• 前者str().get_vec()は「危険」です。一時オブジェクトstr()とともに参照先vector<string>実体も破棄されてしまうため、走査中のイテレータは無効オブジェクトを指してしまいます。
• 後者str_exist.get_vec()は「安全」です。変数str_existの生存期間中は参照先も生存していますから、安全にvector<string>実体へとアクセスできます。

余談：個人の感性にも左右されますが、get_vecというメンバ関数名からはデータメンバへの参照を返すインタフェースを想起させますし、効率的なC++クラス設計としても参照を返す方がパフォーマンス上好ましいです。本当に「コピーされたデータを返す」のであれば、cloneやcopyなどの名称を含めたほうがよいかもしれません。

Comments

[__ook]

すごく理解できました。ありがとうございます。
> 余談：個人の感性にも左右されますが、get_vecというメンバ関数名からはデータメンバへの参照を返すインタフェースを想起させますし、効率的なC++クラス設計としても参照を返す方がパフォーマンス上好ましいです。
なるほど…参照を返却すべきなのですね、ただその場合はrange-based forを使用することは望ましくない、と…

[yohhoy]

これはクラス設計の問題ですから、明確な正解は存在しないと思います。
yumetodoさん回答のコメント部に書いたように、例えば https://wandbox.org/permlink/7fFSaBCQm81kOKsP という解法もありえます。
上記では参照返却による効率性と、ムーブされたオブジェクト返却による安全性を両立させたものになっています。（代償として、より高度なC++の知識を要求することにはなりますが...）

[__ook]

なるほど…たしかにこれなら安全に一時オブジェクトを参照できますね…

Answer 3

こんにちは。

私もあれ？っと思ったのですが、どうも範囲ベースforのコンテナへの参照は右辺値参照で参照されるからのようです。
この旨のコードが範囲for文に記載されていました。

従って、ここで回答したのと同じ原理で破棄されてしまうということのようです。

wandboxで実験のコードの前者は上記質問のNG1と同じ原理です。

2つ目のrange-based forは危険でしょうか？

str_existの寿命はmain関数の終わりまであるので、危険ではないです。
これに対して、for文の「コンテナを指定する式の一時オブジェクト」の寿命はその式が終わるまでですので、for文のループ・ブロックに入る前に破棄されます。

Comments

[__ook]

ああ…なるほど…ありがとうございます。
つまり…参照で返却した場合iteratorで回すのが正、でしょうか？

[Chironian]

iteratorだろうが範囲ベースだろうが、ループ対象のコンテナ・オブジェクトがfor文の終わりまで生きているようなコードならOKです。
iteratorで、beginとendのオブジェクトについて、それぞれ異なる一時オブジェクトを指定する人はいないとは思いますが、油断するとやるかもですね。例えば下記はダメです。
for( auto it = something().get_vector().begin(); it != something().get_vector().end(); ++it)
{
}

[__ook]

やったことがあるやつでした…
ありがとうございます。