Potential use-after-free in GltfConverters / AssetFetcher

[kring]

The new AssetFetcher introduced in #854 stores the AsyncSystem and the vector of request headers by reference. This is safe if at least one of these is true:

1. The AssetFetcher is only used during the course of the function it is passed to, never after the function returns. But this is not true. I3dmToGltfConverter captures it in a lambda to be used later, after the function returns. Even if we fix this converter, future converters will need to keep this in mind as well.
2. The caller extends the lifetime of those objects until the converter function is done with them. But there's currently no documentation of this requirement. And even if there were, this is a difficult thing to be sure of, and an easy thing to overlook.

The current use may very well be safe (even checking that is a fair bit of work), but it's precarious. It's easy to imagine future changes leading to subtle, intermittent bugs.

The easiest solution is store the AsyncSystem and request headers by value instead. If that's too expensive, the next best thing is to pass them to the function directly rather than wrapping them up in AssetFetcher. It makes for a lot of parameters, but it's much more natural / obvious to capture them by value that way.

CC @timoore

[kring]

One easy improvement idea is to delete the AssetFetcher copy/move constructors and assignment operators. That will allow the efficiency of storing these objects by reference in AssetFetcher, and the convenience of keeping them in an object rather than passing them as individual parameters, while also forcing implementors of converter functions to do the safe thing of capturing individual fields by value rather than capturing the entire object by value (which inadvertently captures the async system and headers by reference).

[timoore]

This has been addressed in #912 .

[kring]

I'm going to reopen this, because we generally don't close issues until the PR that addresses them is merged. If you add "Fixes #893" to the top post of the PR, then GitHub will automatically close the issue upon merge.

[timoore]

I'm going to reopen this, because we generally don't close issues until the PR that addresses them is merged. If you add "Fixes #893" to the top post of the PR, then GitHub will automatically close the issue upon merge.

Whoops! No problem.