make copy of chain head's signed_block_ptr to avoid dangling reference warning

[spoonincode]

This line is being flagged with a possibly dangling reference to a temporary warning in gcc14. Making a copy of the returned signed_block_ptr resolves the warning.

Really what happens here is that a reference is returned to contents of a non-owned shared_ptr -- this is equally dangerous from a object lifetime standpoint; at least as a general rule. Though as written in this particular case there isn't actually a risk afaict because the non-owned object is guaranteed to stay alive where it is used from the nature of how controller & chain_head work.

I'm not sure what other changes could be considered.. head() and fork_db_head() might be a little hazardous here because they return a copy but then may encourage you to access parts of it via references,

spring/libraries/chain/include/eosio/chain/block_handle.hpp

Lines 32 to 35 in 64a154a

	const block_id_type& id() const { return std::visit<const block_id_type&>([](const auto& bsp) -> const block_id_type& { return bsp->id(); }, _bsp); }
	const block_id_type& previous() const { return std::visit<const block_id_type&>([](const auto& bsp) -> const block_id_type& { return bsp->previous(); }, _bsp); }
	const signed_block_ptr& block() const { return std::visit<const signed_block_ptr&>([](const auto& bsp) -> const signed_block_ptr& { return bsp->block; }, _bsp); }
	const block_header& header() const { return std::visit<const block_header&>([](const auto& bsp) -> const block_header& { return bsp->header; }, _bsp); };

making it very easy to fall in to the trap of taking a reference to some temporary -- what happened here indirectly.

It may be that's just how it has to be.

Though could consider changing head() to return a block_handle& so that something like head().block() could continue to be used as-is and doesn't throw a warning; presumably anyone accessing chain head is aware of its lifetime rules wrt being mutated on main thread only.

[heifner]

In this particular case, I wonder if it would be better to replace the usages of new_b with inline calls to chain.head().xxx.
Also why does head() return a block_handle rather than a const block_handle&. Seems like a pointless copy of block_handle on every access. I think both head() and fork_db_head() should return a const block_handle& which I believe will remove the dangling reference warning.

[greg7mdp]

Also why does head() return a block_handle rather than a const block_handle&

fork_db_head() does construct a handle, so returning a value is really the only thing we can do I believe unless we add a new block_handle member in fork_db.

As for head(), I think we should return a reference, with the hope that users in tests will not store the returned reference in an auto& variable which would then change along with controller's chain_head.

[heifner]

I think we should be consistent in what head() and fork_db_head() returns. I'm tempted to suggest we add a const block_handle& chain_head() const to avoid the std::variant of std::shared_ptr copies. However, I don't think it is worth it. Maybe we should instead have block_handle::block() return a signed_block_ptr.

[greg7mdp]

Maybe we should instead have block_handle::block() return a signed_block_ptr.

+1.

I think also block_handle::id() and previous() should return values not references.

[spoonincode]

What do you all want to do about this? Go ahead as is or go through some more refactor?

[heifner]

At this point, probably just merge this PR. We can refactor later if we think it is needed.

[ericpassmore]

Note:start
group: STABILITY
category: INTERNALS
summary: Avoid dangling reference in producer plugin by providing explicit type for pointer to copy of chain_head.
Note:end