[Spice-devel] [PATCH spice 1/1] spicec: do not call connect_secure when connect_unsecure fails due to protocol version mismatch

[Hans de Goede]

Good catch!

But nack for the current implementation of the fix:

If options.allow_unsecure() == True and options.allow_secure() == False,
and the unsecure connection fails with error_code != SPICEC_ERROR_CODE_VERSION_MISMATCH
we will exit RedChannelBase::connect without throwing an error while
we've not established a connection.

Regards,

Hans


On 12/07/2010 10:47 AM, Arnon Gilboa wrote:
> If connect_unsecure failed due to protocol version mismatch, don't try to
> connect_secure with the same version, but retry (connect_secure or
> connect_unsecure) with older version.
>
> This solves the following bug: when "new" Spice client (protocol version 2)
> with given secure_port connects to "old" server which is not using the same
> secure_port (or not using a secure_port at all), the client exits immediately.
>
> In this scenario, the client first tries to use Spice protocol version 2 to
> connect the unsecure port, and altough this fails due to version mismatch, it
> tries to connect to the secure port with the same protocol version 2, which is
> a wrong behavior, fails due to socket error 10061 (WSAECONNREFUSED -
> Connection refused) and handled mistakenly by immediate exit, instead of
> retrying with protocol version 1.
> ---
>   client/red_channel.cpp |   22 ++++++++++++----------
>   1 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/client/red_channel.cpp b/client/red_channel.cpp
> index 273b28d..2886505 100644
> --- a/client/red_channel.cpp
> +++ b/client/red_channel.cpp
> @@ -217,20 +217,22 @@ void RedChannelBase::connect(const ConnectionOptions&  options, uint32_t connecti
>                   RedPeer::connect_unsecure(host, options.unsecure_port);
>                   link(connection_id, password, protocol);
>                   return;
> -            } catch (...) {
> -                if (!options.allow_secure()) {
> +            } catch (Exception&  e) {
> +                // On protocol version mismatch, don't connect_secure with the same version
> +                if (e.get_error_code() == SPICEC_ERROR_CODE_VERSION_MISMATCH) {
>                       throw;
> -                }
> -                RedPeer::close();
> +                }
>               }
>           }
> -        ASSERT(options.allow_secure());
> -        RedPeer::connect_secure(options, host);
> -        link(connection_id, password, protocol);
> +        if (options.allow_secure()) {
> +            RedPeer::close();
> +            RedPeer::connect_secure(options, host);
> +            link(connection_id, password, protocol);
> +        }
>       } catch (Exception&  e) {
> -        if (protocol == 2&&
> -            options.protocol == 0&&
> -            e.get_error_code() == SPICEC_ERROR_CODE_VERSION_MISMATCH) {
> +        // On protocol version mismatch, retry with older version
> +        if (e.get_error_code() == SPICEC_ERROR_CODE_VERSION_MISMATCH&&
> +                                 protocol == 2&&  options.protocol == 0) {
>               RedPeer::cleanup();
>               protocol = 1;
>               goto retry;