mirror of
https://gitlab.uni-freiburg.de/opensourcevdi/spice
synced 2025-12-27 23:49:04 +00:00
d2e1f939fe
During my dynamic monitor support testing today, I hit the following assert
in red_worker.c:
"red_push_monitors_config: condition `monitors_config != NULL' failed"
This is caused by the following scenario:
1) Guest causes handle_dev_monitors_config_async() to be called
2) handle_dev_monitors_config_async() calls worker_update_monitors_config()
3) handle_dev_monitors_config_async() pushes worker->monitors_config, this
takes a ref on the current monitors_config
4) Guest causes handle_dev_monitors_config_async() to be called *again*
5) handle_dev_monitors_config_async() calls worker_update_monitors_config()
6) worker_update_monitors_config() does a decref on worker->monitors_config,
releasing the workers reference, this monitor_config from step 2 is
not yet free-ed though as the pipe-item still holds a ref
7) worker_update_monitors_config() creates a new monitors_config with an
initial ref-count of 1 and stores that in worker->monitors_config
8) The pipe-item of the *first* monitors_config is send, upon completion
a decref is done on the monitors_config, and monitors_config_decref not
only frees the monitor_config, but *also* sets worker->monitors_config
to NULL, even though worker->monitors_config no longer refers to the
monitor_config being freed, it refers to the 2nd monitor_config!
9) The client which was connected when this all happened disconnects
10) A new client connects, leading to the assert:
at red_worker.c:9519
num_common_caps=1, common_caps=0x5555569b6f60, migrate=0,
stream=<optimized out>, client=<optimized out>, worker=<optimized out>)
at red_worker.c:10423
at red_worker.c:11301
Note that red_worker.c:9519 is:
red_push_monitors_config(dcc);
gdb does not point to the actual line of the assert because the function gets
inlined.
The fix is easy and obvious, don't set worker->monitors_config to NULL in
monitors_config_decref. I'm a bit baffled as to why that code is there in
the first place, the whole point of ref-counting is to not have one single
unique place to store the reference...
This fix should not have any adverse side-effects as the 4 callers of
monitors_config_decref fall into 2 categories:
1) Code which immediately after the decref replaces worker->monitors_config
with a new monitors_config:
worker_update_monitors_config()
set_monitors_config_to_primary()
2) pipe-item freeing code, which should not touch the worker state at all
to being with
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
||
|---|---|---|
| .. | ||
| tests | ||
| .gitignore | ||
| agent-msg-filter.c | ||
| agent-msg-filter.h | ||
| char_device.c | ||
| char_device.h | ||
| demarshallers.h | ||
| dispatcher.c | ||
| dispatcher.h | ||
| glz_encode_match_tmpl.c | ||
| glz_encode_tmpl.c | ||
| glz_encoder_config.h | ||
| glz_encoder_dictionary_protected.h | ||
| glz_encoder_dictionary.c | ||
| glz_encoder_dictionary.h | ||
| glz_encoder.c | ||
| glz_encoder.h | ||
| inputs_channel.c | ||
| inputs_channel.h | ||
| jpeg_encoder.c | ||
| jpeg_encoder.h | ||
| main_channel.c | ||
| main_channel.h | ||
| main_dispatcher.c | ||
| main_dispatcher.h | ||
| Makefile.am | ||
| migration_protocol.h | ||
| mjpeg_encoder.c | ||
| mjpeg_encoder.h | ||
| red_bitmap_utils.h | ||
| red_channel.c | ||
| red_channel.h | ||
| red_client_cache.h | ||
| red_client_shared_cache.h | ||
| red_common.h | ||
| red_dispatcher.c | ||
| red_dispatcher.h | ||
| red_memslots.c | ||
| red_memslots.h | ||
| red_parse_qxl.c | ||
| red_parse_qxl.h | ||
| red_tunnel_worker.c | ||
| red_tunnel_worker.h | ||
| red_worker.c | ||
| red_worker.h | ||
| reds_gl_canvas.c | ||
| reds_gl_canvas.h | ||
| reds_sw_canvas.c | ||
| reds_sw_canvas.h | ||
| reds-private.h | ||
| reds.c | ||
| reds.h | ||
| smartcard.c | ||
| smartcard.h | ||
| snd_worker.c | ||
| snd_worker.h | ||
| spice-experimental.h | ||
| spice-server.syms | ||
| spice.h | ||
| spicevmc.c | ||
| stat.h | ||
| zlib_encoder.c | ||
| zlib_encoder.h | ||