Hi,
On Tue, Jan 19, 2016 at 09:52:24AM +0000, Frediano Ziglio wrote:
> This fixes a crash if red_channel_client disconnect is called
> handling a message.
> This can happen for instance handling SPICE_MSGC_ACK which calls
> red_channel_client_push which try to write items to socket detecting
> writing errors (for instance socket disconnection).
> Messages are read in a loop and the red_channel_client_disconnect
> would cause rcc->stream to be NULL which will turn in a use-after-free
> problem (stream in red_peer_handle_incoming will use cached stream value).
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/red-channel.c | 38 ++++++++++++++++++++++----------------
> 1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 306c87d..51e8958 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -1231,22 +1231,28 @@ void red_channel_client_ref(RedChannelClient *rcc)
>
> void red_channel_client_unref(RedChannelClient *rcc)
> {
> - if (!--rcc->refs) {
> - spice_debug("destroy rcc=%p", rcc);
> - if (rcc->send_data.main.marshaller) {
> - spice_marshaller_destroy(rcc->send_data.main.marshaller);
> - }
> + if (--rcc->refs != 0) {
My opnion is that doing --(var) on if() does not make the code easy
readable. rcc->refs will always be decreased so it should be outside
the conditional.
I see this in multiple places in spice-server but my vote is to change
that at least for new code :)
best,
toso
> + return;
> + }
>
> - if (rcc->send_data.urgent.marshaller) {
> - spice_marshaller_destroy(rcc->send_data.urgent.marshaller);
> - }
> + spice_debug("destroy rcc=%p", rcc);
>
> - red_channel_client_destroy_remote_caps(rcc);
> - if (rcc->channel) {
> - red_channel_unref(rcc->channel);
> - }
> - free(rcc);
> + reds_stream_free(rcc->stream);
> + rcc->stream = NULL;
> +
> + if (rcc->send_data.main.marshaller) {
> + spice_marshaller_destroy(rcc->send_data.main.marshaller);
> }
> +
> + if (rcc->send_data.urgent.marshaller) {
> + spice_marshaller_destroy(rcc->send_data.urgent.marshaller);
> + }
> +
> + red_channel_client_destroy_remote_caps(rcc);
> + if (rcc->channel) {
> + red_channel_unref(rcc->channel);
> + }
> + free(rcc);
> }
>
> void red_channel_client_destroy(RedChannelClient *rcc)
> @@ -1749,7 +1755,7 @@ void red_channel_pipes_add_empty_msg(RedChannel *channel, int msg_type)
> int red_channel_client_is_connected(RedChannelClient *rcc)
> {
> if (!rcc->dummy) {
> - return rcc->stream != NULL;
> + return ring_item_is_linked(&rcc->channel_link);
> } else {
> return rcc->dummy_connected;
> }
> @@ -1804,6 +1810,8 @@ static void red_channel_remove_client(RedChannelClient *rcc)
> rcc->channel->type, rcc->channel->id,
> rcc->channel->thread_id, pthread_self());
> }
> + spice_return_if_fail(ring_item_is_linked(&rcc->channel_link));
> +
> ring_remove(&rcc->channel_link);
> spice_assert(rcc->channel->clients_num > 0);
> rcc->channel->clients_num--;
> @@ -1845,8 +1853,6 @@ void red_channel_client_disconnect(RedChannelClient *rcc)
> rcc->channel->core->watch_remove(rcc->stream->watch);
> rcc->stream->watch = NULL;
> }
> - reds_stream_free(rcc->stream);
> - rcc->stream = NULL;
> if (rcc->latency_monitor.timer) {
> rcc->channel->core->timer_remove(rcc->latency_monitor.timer);
> rcc->latency_monitor.timer = NULL;
> --
> 2.4.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel