ssl memory leak, PR 63687

[Stefan Eissing <st...@greenbytes.de>]

In regard to https://bz.apache.org/bugzilla/show_bug.cgi?id=63687
there are now 2 patches, varying in style but fixing the very same missing free. 

My attempt uses gotos to have a single point of exit where to free things, while Giovanni Bechis provided a changed version that frees on all possible early returns. Otherwise they achieve the same.

I would like to ask Ruediger, who is more "into" mod_ssl, to decide which to apply and what fits the module best. I am fine with both versions. Obviously, I nowadays prefer the goto/single exit style myself, but the rest of the module is different. That might  interfere with readability for people not accustomed to the goto variant.

Thanks for the help (and sorry to have screwed this up in the fist place),

Stefan


 

Re: ssl memory leak, PR 63687

[Ruediger Pluem <rp...@apache.org>]

Me too. +1.

Regards

Rüdiger

On 4/15/20 1:38 PM, Stefan Eissing wrote:
> +1. I like it.
> 
>> Am 15.04.2020 um 13:36 schrieb Yann Ylavic <yl...@gmail.com>:
>>
>> <PR63687.diff>
> 
> 

Re: ssl memory leak, PR 63687

[Stefan Eissing <st...@greenbytes.de>]

+1. I like it.

> Am 15.04.2020 um 13:36 schrieb Yann Ylavic <yl...@gmail.com>:
> 
> <PR63687.diff>

Re: ssl memory leak, PR 63687

[Yann Ylavic <yl...@gmail.com>]

On Wed, Apr 15, 2020 at 2:06 PM Giovanni Bechis <gi...@paclan.it> wrote:
>
> Il 15 aprile 2020 13:36:05 CEST, Yann Ylavic <yl...@gmail.com> ha scritto:
> >On Wed, Apr 15, 2020 at 1:18 PM Yann Ylavic <yl...@gmail.com>
> >wrote:
> >>
> >> In this particular patch, setting "rv" and using a single label may
> >be
> >> an option too. Something like this for each goto:
> >>    rv = 0|1;
> >>    goto cleanup;
> >> ?
> >
> >Would look like the attached, FWIW..
>
> +1, best of both words.

Thanks folks, r1876548.

Re: ssl memory leak, PR 63687

[Giovanni Bechis <gi...@paclan.it>]

Il 15 aprile 2020 13:36:05 CEST, Yann Ylavic <yl...@gmail.com> ha scritto:
>On Wed, Apr 15, 2020 at 1:18 PM Yann Ylavic <yl...@gmail.com>
>wrote:
>>
>> In this particular patch, setting "rv" and using a single label may
>be
>> an option too. Something like this for each goto:
>>    rv = 0|1;
>>    goto cleanup;
>> ?
>
>Would look like the attached, FWIW..

+1, best of both words.
 
    Giovanni 

Re: ssl memory leak, PR 63687

[Yann Ylavic <yl...@gmail.com>]

On Wed, Apr 15, 2020 at 1:18 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> In this particular patch, setting "rv" and using a single label may be
> an option too. Something like this for each goto:
>    rv = 0|1;
>    goto cleanup;
> ?

Would look like the attached, FWIW..

Re: ssl memory leak, PR 63687

[Yann Ylavic <yl...@gmail.com>]

On Wed, Apr 15, 2020 at 1:08 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Wed, Apr 15, 2020 at 10:11 AM Stefan Eissing
> <st...@greenbytes.de> wrote:
> >
> > Obviously, I nowadays prefer the goto/single exit style myself, but the rest of the module is different.
>
> +1, cleanup in each early return is easier to get wrong IMHO.
> In your patch I'd rename the "leave" label to "cleanup" though, to
> make it clear why the goto is needed (but also because adding a naming
> discussion to a style one is fun :) ).

In this particular patch, setting "rv" and using a single label may be
an option too. Something like this for each goto:
   rv = 0|1;
   goto cleanup;
?

Re: ssl memory leak, PR 63687

[Yann Ylavic <yl...@gmail.com>]

On Wed, Apr 15, 2020 at 10:11 AM Stefan Eissing
<st...@greenbytes.de> wrote:
>
> Obviously, I nowadays prefer the goto/single exit style myself, but the rest of the module is different.

+1, cleanup in each early return is easier to get wrong IMHO.
In your patch I'd rename the "leave" label to "cleanup" though, to
make it clear why the goto is needed (but also because adding a naming
discussion to a style one is fun :) ).

Cheers,
Yann.