[Spice-devel] [PATCH spice-gtk 14/15] Remove some large stack allocations

[Hans de Goede]

Hi,

On 03/13/2012 02:40 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> A few functions have very large arrays declared on the stack.
> Replace these with heap allocations, to reduce risk of stack
> overflows in deep callpaths
> ---
>   gtk/channel-playback.c |    6 ++++--
>   gtk/spice-channel.c    |   16 ++++++++++++----
>   2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/gtk/channel-playback.c b/gtk/channel-playback.c
> index 32f8b1a..c353cd2 100644
> --- a/gtk/channel-playback.c
> +++ b/gtk/channel-playback.c
> @@ -353,10 +353,12 @@ static void playback_handle_data(SpiceChannel *channel, SpiceMsgIn *in)
>                             packet->data, packet->data_size);
>           break;
>       case SPICE_AUDIO_DATA_MODE_CELT_0_5_1: {
> -        celt_int16_t pcm[256 * 2];
> +        celt_int16_t *pcm;
> +        gsize pcmLen = 256 * 2;
>
>           g_return_if_fail(c->celt_decoder != NULL);
>
> +        pcm = g_new0(celt_int16_t, pcmLen);
>           if (celt051_decode(c->celt_decoder, packet->data,
>                              packet->data_size, pcm) != CELT_OK) {
>               g_warning("celt_decode() error");
> @@ -364,7 +366,7 @@ static void playback_handle_data(SpiceChannel *channel, SpiceMsgIn *in)
>           }
>
>           emit_main_context(channel, SPICE_PLAYBACK_DATA,
> -                          (uint8_t *)pcm, sizeof(pcm));
> +                          (uint8_t *)pcm, pcmLen);
>           break;
>       }
>       default:

You seem to be never freeing the data here. Also I disagree with switching
to malloc / free here in general. This path will get called many times a second,
malloc / free is not a cheap operation and 1024 bytes is not such a big amount to
put on the stack.

> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index cdc86ba..248f387 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -941,17 +941,24 @@ static int spice_channel_read_sasl(SpiceChannel *channel, void *data, size_t len
>       /*             c->sasl_decoded_length, c->sasl_decoded_offset); */
>
>       if (c->sasl_decoded == NULL || c->sasl_decoded_length == 0) {
> -        char encoded[8192]; /* should stay lower than maxbufsize */
> +        char *encoded;
> +        gsize encodedLen;
>           int err, ret;
>
> +        encodedLen = 8192;
> +        encoded = g_new0(char, encodedLen);
> +
>           g_warn_if_fail(c->sasl_decoded_offset == 0);
>
> -        ret = spice_channel_read_wire(channel, encoded, sizeof(encoded));
> -        if (ret<  0)
> +        ret = spice_channel_read_wire(channel, encoded, encodedLen);
> +        if (ret<  0) {
> +            g_free(encoded);
>               return ret;
> +        }
>
>           err = sasl_decode(c->sasl_conn, encoded, ret,
>                             &c->sasl_decoded,&c->sasl_decoded_length);
> +        g_free(encoded);
>           if (err != SASL_OK) {
>               g_warning("Failed to decode SASL data %s",
>                         sasl_errstring(err, NULL, NULL));

This one looks fine from the free pov. But again this is a semi hot
code path, and malloc / free is not a cheap operation, also there are
no guarantees wrt cache hotness when it comes to the heap, where as the stack
is likely hot in the cache.

> @@ -1629,6 +1636,7 @@ static void spice_channel_recv_link_msg(SpiceChannel *channel)
>   {
>       SpiceChannelPrivate *c;
>       int rc, num_caps, i;
> +    uint32_t *caps;
>
>       g_return_if_fail(channel != NULL);
>       g_return_if_fail(channel->priv != NULL);
> @@ -1666,7 +1674,7 @@ static void spice_channel_recv_link_msg(SpiceChannel *channel)
>       /* see original spice/client code: */
>       /* g_return_if_fail(c->peer_msg + c->peer_msg->caps_offset * sizeof(uint32_t)>  c->peer_msg + c->peer_hdr.size); */
>
> -    uint32_t *caps = (uint32_t *)((uint8_t *)c->peer_msg + c->peer_msg->caps_offset);
> +    caps = (uint32_t *)((uint8_t *)c->peer_msg + c->peer_msg->caps_offset);
>
>       g_array_set_size(c->remote_common_caps, c->peer_msg->num_common_caps);
>       for (i = 0; i<  c->peer_msg->num_common_caps; i++, caps++) {

This seems an unrelated fix.

Regards,

Hans