Handle director loop #3899
Comments
nigoroll
added a commit
to nigoroll/varnish-cache
that referenced
this issue
Mar 3, 2023
Before this patch, layered directors needed to be destroyed top to bottom, and whenever that order was missed, we would panic, because a to-be-destroyed director still had references to it. One special case where this issue would always trigger are looped directors. Those should not be used and will cause havoc, which is a separate issue varnishcache#3899. But we should still be able to unconfigure such a configuration. We solve the destruction order issue by making it a two step process: When a director is destroyed through VRT_DelDirector, a new release function is called, which has to disassociate any backends. The director then loses a reference, and when all references are gone, the destroy function is called. The new callback would not be necessary for the cases in varnish-cache today, directors could simply disassociate any backends before calling VRT_DelDirector. But this would complicate or even make impossible transfer of director ownership, where the code responsible for creating a director is not the same as the one calling VRT_DelDirector(). As a side effect, it also helps clarity. Fixes varnishcache#3895
nigoroll
added a commit
to nigoroll/varnish-cache
that referenced
this issue
Mar 6, 2023
Before this patch, layered directors needed to be destroyed top to bottom, and whenever that order was missed, we would panic, because a to-be-destroyed director still had references to it. One special case where this issue would always trigger are looped directors. Those should not be used and will cause havoc, which is a separate issue varnishcache#3899. But we should still be able to unconfigure such a configuration. We solve the destruction order issue by making it a two step process: When a director is destroyed through VRT_DelDirector, a new release function is called, which has to disassociate any backends. The director then loses a reference, and when all references are gone, the destroy function is called. The new callback would not be necessary for the cases in varnish-cache today, directors could simply disassociate any backends before calling VRT_DelDirector. But this would complicate or even make impossible transfer of director ownership, where the code responsible for creating a director is not the same as the one calling VRT_DelDirector(). As a side effect, it also helps clarity. Fixes varnishcache#3895
nigoroll
added a commit
to nigoroll/varnish-cache
that referenced
this issue
Mar 6, 2023
Before this patch, layered directors needed to be destroyed top to bottom, and whenever that order was missed, we would panic, because a to-be-destroyed director still had references to it. One special case where this issue would always trigger are looped directors. Those should not be used and will cause havoc, which is a separate issue varnishcache#3899. But we should still be able to unconfigure such a configuration. We solve the destruction order issue by making it a two step process: When a director is destroyed through VRT_DelDirector, a new release function is called, which has to disassociate any backends. The director then loses a reference, and when all references are gone, the destroy function is called. The new callback would not be necessary for the cases in varnish-cache today, directors could simply disassociate any backends before calling VRT_DelDirector. But this would complicate or even make impossible transfer of director ownership, where the code responsible for creating a director is not the same as the one calling VRT_DelDirector(). As a side effect, it also helps clarity. Fixes varnishcache#3895
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This ticket is to take a related, but different issue out of #3895
We do not currently handle director loops, which can cause an infinite recursion on resolution, inevitably leading to a stack overflow.
Two options are on the table right now
when adding a
srcbackend to adst, asksrcvia a new callback ifdstis reachable. The callback would iterate over all backends. If any matchesdst, that would fail the add operation. Otherwise the callback would be called recursively.Limit the number of layers which director resolution can pass through.
The text was updated successfully, but these errors were encountered: