Protect against drawing a sprite when the underlying texture has been moved from

[ChrisThrasher]

Description

Inspired by #3062. If you construct a texture and a sprite then move the sprite, the sprite is left pointing at an address where there is no longer a usable texture. The texture's lifetime has not yet ended, however the texture is in a moved-from state and thus it's not suitable to draw the sprite. Doing so will result in unintended and undesirable effects.

[ChrisThrasher]

I wonder if it's better to put this check in sf::RenderTarget::draw so this bug can be caught in more places.

[vittorioromeo]

I appreciate your attempt at catching this bug, because it's very subtle and I agree that it's something that we should try to prevent.

However, what was happening in #3062 is not what you described, and the assertion you added doesn't help at all because m_texture->getNativeHandle() is undefined behavior if m_texture is pointing to a non-existent texture.

The issue with #3062 was along these lines:

struct Obj
{
    sf::Texture m_t;
    sf::Sprite m_s;
    
    Obj(sf::Texture&& t) : m_t(std::move(t)), m_s(m_t) { }
};

std::optional<Obj> makeObj()
{
    auto t = sf::Texture::loadFromFile(/* ... */);
    if (!t.has_value()) { return std::nullopt; } 
    
    Obj result{std::move(t)};
    return result; // NRVO not possible here!
}

int main()
{
    // ...
    auto o = makeObj();
    if (o.has_value()) 
        renderWindow.draw(o.m_s); // BOOM
}

What's happening above is that when we create result in the frame of makeObj, result.m_s points to the address of result.m_t, which is somewhere is the frame of makeObj.

As we return from makeObj without NRVO, the address of the texture changes, and the address stored in result.m_s is now pointing to a destroyed object -- it is not just "moved from", it's completely gone and it's undefined behavior to access it.

The only way I can think of catching issues like this one is with debug-only runtime reference counting like in this experimental branch I once created: 868e9a0

But I do recall you (and/or maybe someone else) were against that approach.

Now, this is a very annoying and subtle bug, and I think it should be prevented. I think the right way of preventing this bug is to completely stop sf::Sprite from storing a texture at all, and instead just pass the texture on draw.

So, instead of:

sf::Texture t(...);
sf::Sprite s(t);

renderWindow.draw(s);

Users should instead do:

sf::Texture t(...);
sf::Sprite s;

renderWindow.draw(s, t); 

Or something along those lines, maybe using RenderStates or some other way of injecting the texture.

It makes no sense that the sprite stores a pointer to the texture forever, as it's only needed during the draw call.