Am I right that if the base is not in the hash its memory has been freed, which will cause use-after-free at`next = base->link_next`?

In D10909#278078, @Sergey Sharybin (sergey) wrote:

Am I right that if the base is not in the hash its memory has been freed, which will cause use-after-free at`next = base->link_next`?

Yes, that is true. It never happens though, because seq_cache_recycle_linked() is _only_ used for final images and therefore base->link_next is always NULL. There is one usage where it's run if SeqCacheItem->ibuf == NULL which should never happen.

I think that code could be removed.

Honestly I would rather remove this whole linking system. It's only purpose was to aid with invalidation. Now for some time I have not seen any reports that invalidation is not working correctly. But I don't want to remove it at the end of BCON2 really as it may uncover bugs that will take time to discover again.

Afraid I'm a bit more confused now :( There is also SeqCacheKey *prev = base->link_prev;. So, if the base is not in the cache->hash, can its content be trusted? Or should it be changed to something like:

while (base) {
  if (base is not in cache->hash) {
    break;
  }
  SeqCacheKey *prev = base->link_prev;
  BLI_ghash_remove ...
  base = prev;
}

?

In D10909#278097, @Sergey Sharybin (sergey) wrote:

Afraid I'm a bit more confused now :( There is also SeqCacheKey *prev = base->link_prev;.

Sorry it turns out that I was wrong previously. Because if base is freed by seq_cache_keyfree, it remains in memory until it is replaced by different key or by seq_cache_destruct().

So, if the base is not in the cache->hash, can its content be trusted?

Since key can be replaced, approach in this patch will start to remove random keys, which I have confirmed when I enable cache visualization.

Not sure what would be the best solution. I could add usercount to SeqCacheKey, but that would be relatively risky change on par with removing this linking system altogether. I will think about some other solutions.

Is probably fine, but this system is rather cryptic to me. Mix of hash and linked list.. Just make sure you aren't fighting race condition here, because that'd require different solution.

I'd also use BLI_ghash_haskey as it semantically a better fit.

In D10909#278180, @Sergey Sharybin (sergey) wrote:

Is probably fine, but this system is rather cryptic to me. Mix of hash and linked list.. Just make sure you aren't fighting race condition here, because that'd require different solution.

It is a bit cryptic... But when speed effect is used, raw images can be rendered for random frame, this seemed like good solution to follow keys when removing final one. Probably it would be more sensible to explicitly store frame in which these keys were created, but that will require some modifications.
It's definitely not a race condition.