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

[Hans de Goede]

Hi,

Please see my comments on the original version.

Regards,

Hans


On 03/13/2012 02:51 PM, Daniel P. Berrange wrote:
> On Tue, Mar 13, 2012 at 01:40:12PM +0000, 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(-)
>
> Urgh, this patch was a bit messed up - a missing 'g_free' and an unrelated
> change. Consider this one replaced by the following two patches
>
>  From 1faa6949404a7aad64030fd29812749ca9ddabfe Mon Sep 17 00:00:00 2001
> From: "Daniel P. Berrange"<berrange at redhat.com>
> Date: Tue, 13 Mar 2012 13:24:07 +0000
> Subject: [PATCH] Remove some large stack allocations
>
> 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 |    8 ++++++--
>   gtk/spice-channel.c    |   13 ++++++++++---
>   2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/gtk/channel-playback.c b/gtk/channel-playback.c
> index 32f8b1a..2b28d07 100644
> --- a/gtk/channel-playback.c
> +++ b/gtk/channel-playback.c
> @@ -353,18 +353,22 @@ 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_free(pcm);
>               g_warning("celt_decode() error");
>               return;
>           }
>
>           emit_main_context(channel, SPICE_PLAYBACK_DATA,
> -                          (uint8_t *)pcm, sizeof(pcm));
> +                          (uint8_t *)pcm, pcmLen);
> +        g_free(pcm);
>           break;
>       }
>       default:
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index cdc86ba..d53210e 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));