Only call ws_client_free once all scheduled events have triggered

nicolasff / webdis

A Redis HTTP interface with JSON output

https://webd.is

BSD 2-Clause "Simplified" License

2.82k stars 307 forks source link

Only call ws_client_free once all scheduled events have triggered #210

Closed jessie-murray closed 2 years ago

jessie-murray commented 2 years ago

Fixes #209. A WS client socket closure could cause Webdis to schedule the send of a closing frame, leading to both EV_READ and EV_WRITE scheduled events. They would both fail and each lead to a call to ws_client_free, causing a double-free and a crash.

nicolasff commented 2 years ago

Thanks for the quick PR!

Looking at the other call to ws_client_free (from worker_can_read), shouldn't it also be changed to ws_close_if_able? Also, this feels like the sort of bug that could easily be covered by a new test in ws-tests.py. It seems like it's also important to check for leaks for any change dealing with memory allocation.

And lastly the CodeQL results show 4 new warnings. Although these seem unrelated to this particular change, they are worth investigating.

nicolasff commented 2 years ago

Looks good!

The new test does reproduce the issue reliably, and the fix solves it. I also confirmed that this is no longer reproducible with a browser. Valgrind reports no leaks.

Thanks again!