varnishcache / varnish-cache

Varnish Cache source code repository
https://www.varnish-cache.org
Other
3.65k stars 374 forks source link

VDP closing is still incomplete (and wrong since the last change) #4067

Closed nigoroll closed 7 months ago

nigoroll commented 7 months ago

This issue will be followed up by a PR.

Expected Behavior

VDPs should be closed for all possible code paths and closing VDPs should be a function of the transport alone.

I shall correct my own mistakes.

Current Behavior

VDPs get initialized in cnt_transmit() and ever since VDP_close() got introduced, it was up to the transport layer to close them. Some time ago I noticed that we would miss to close VDPs in some cases and added 6d423aa5c60940ebde76c91f0a482767346df405 thinking that this might be a simple way to fix the missing close calls from a central place, avoiding repetition. But that was incomplete and wrong, I overlooked more cases and also created a race condition for pESI. cnt_transmit() calls the transport's delivery function whenever setting up the VDPs was successful, so all delivery functions should close the VDPs under all circumstances, error or not.

Places where we miss a VDP_Close() are:

Once this is done, 6d423aa5c60940ebde76c91f0a482767346df405 should be reverted.

nigoroll commented 7 months ago

fixed as of bb3d027bf6647185be548050243e1cb9d36741a7