[Spice-devel] [spice-devel] spice-xpi rhbz#805602

[Christophe Fergeau]

On Tue, Apr 03, 2012 at 02:28:07PM +0200, Peter Hatina wrote:
> Hi,
> 
> can you do a review for me?
> 
> From 18f6fcabe35be03fe88f1a08b79e22ef505bbacd Mon Sep 17 00:00:00 2001
> From: Peter Hatina <phatina at redhat.com>
> Date: Tue, 3 Apr 2012 13:35:55 +0200
> Subject: [PATCH] Validate port values
> 
> ---
>  SpiceXPI/src/plugin/plugin.cpp |   29 +++++++++++++++++++++++++++--
>  1 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/SpiceXPI/src/plugin/plugin.cpp b/SpiceXPI/src/plugin/plugin.cpp
> index de7afd0..fcfb151 100644
> --- a/SpiceXPI/src/plugin/plugin.cpp
> +++ b/SpiceXPI/src/plugin/plugin.cpp
> @@ -104,6 +104,18 @@ namespace {
> 
>          return dest;
>      }
> +
> +    // helper function for tcp/udp range conversion and validation
> +    static int portToInt(const std::string &port)
> +    {
> +        errno = 0;
> +        char *end;
> +        const long int min = 0;
> +        const long int max = 65535;

I don't think the constants are really needed here

> +        long int conv = strtol(port.c_str(), &end, 10);
> +        return (errno || *end != '\0' || end == port.c_str() || conv <
> min || conv > max)
> +            ? -1 : static_cast<int>(conv);
Do we really care about errno being set?
It seems you are doing that to protect against overflows, but they will be
caught by the conv < min/conv > max checks since LONG_MAX/LONG_MIN will
have been returned in this case.
Why are you testing for end == port.c_str() ? empty string? If so, we could
just check it at the beginning of the function.
I'd split the test for more readability, which would give something like:
if (*end != \0)
  return -1;
if (conv < min || conv > max)
  return -1;

return static_cast<int>(conv);



> +    }
>  }
> 
>  #ifdef NPAPI_USE_CONSTCHARS
> @@ -608,6 +620,17 @@ void nsPluginInstance::SendWStr(uint32_t id, const
> wchar_t *str)
> 
>  void nsPluginInstance::Connect()
>  {
> +    const int port = portToInt(m_port);
> +    const int sport = portToInt(m_secure_port);
> +    if (port < 0 && sport < 0)
> +    {
> +        if (port < 0)
> +            LOG_ERROR("invalid port: " << m_port);
> +        if (sport < 0)
> +            LOG_ERROR("invalid secure port: " << m_secure_port);
> +        return;
> +    }

Not 100% sure we want to put this kind of intelligence in spice-xpi, I'd
rely on the client to complain if it requires one of these 2 port numbers
to be set.

The rest of the patch looks good,

Christophe

> +
>      std::string socket_file(m_tmp_dir);
>      socket_file += "/spice-xpi";
>      if (setenv("SPICE_XPI_SOCKET", socket_file.c_str(), 1))
> @@ -707,8 +730,10 @@ void nsPluginInstance::Connect()
>          LOG_INFO("Initiating connection with controller");
>          SendInit();
>          SendStr(CONTROLLER_HOST, m_host_ip.c_str());
> -        SendValue(CONTROLLER_PORT, atoi(m_port.c_str()));
> -        SendValue(CONTROLLER_SPORT, atoi(m_secure_port.c_str()));
> +        if (port >= 0)
> +            SendValue(CONTROLLER_PORT, port);
> +        if (sport >= 0)
> +            SendValue(CONTROLLER_SPORT, sport);

>          SendValue(CONTROLLER_FULL_SCREEN,
>                     (m_fullscreen == PR_TRUE ?
> CONTROLLER_SET_FULL_SCREEN : 0) |
>                     (m_admin_console == PR_FALSE ?
> CONTROLLER_AUTO_DISPLAY_RES : 0));
> -- 
> 1.7.1
> 
> 
> -- 
> Peter Hatina
> EMEA ENG-Desktop Development
> Red Hat Czech, Brno
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20120403/1d30cc8c/attachment.pgp>