Re: [Bug-wget] [Bug-Wget] Misc. patches

[Darshit Shah]

Does anyone ack this patch? It's a memory leak that I would like to fix.

I'll work on Tim's suggestions next.

On Sat, Jul 5, 2014 at 4:38 PM, Darshit Shah <address@hidden> wrote:
> I just pushed a slightly amended patch. However, here is what I propose:
>
> diff --git a/src/cookies.c b/src/cookies.c
> index 76301ac..3139671 100644
> --- a/src/cookies.c
> +++ b/src/cookies.c
> @@ -549,6 +549,9 @@ check_domain_match (const char *cookie_domain,
> const char *host)
>    return true ? (is_acceptable == 1) : false;
>
>  no_psl:
> +  /* Cleanup the PSL pointers first */
> +  xfree (cookie_domain_lower);
> +  xfree (host_lower);
>  #endif
>
>    /* For efficiency make some elementary checks first */
>
>
> The idea is that we add two new xfree calls instead of pushing the
> originals to afer the no_psl label since we return form the function
> *before* the label is encounterd when psl checks are successful.
>
> There will not be any double frees of these pointers either since that
> region of the code is only executed when psl fails and hence the xfree
> statements weren't called.
>
>
> On Sat, Jul 5, 2014 at 4:19 PM, Giuseppe Scrivano <address@hidden> wrote:
>> Darshit Shah <address@hidden> writes:
>>
>>>>>  static bool
>>>>>  check_domain_match (const char *cookie_domain, const char *host)
>>>>> @@ -509,6 +519,7 @@ check_domain_match (const char *cookie_domain, const 
>>>>> char *host)
>>>>>
>>>>>  #ifdef HAVE_LIBPSL
>>>>>    DEBUGP (("cdm: 1"));
>>>>> +  char * cookie_domain_lower, * host_lower;
>>>>
>>>> please initialize them to NULL and format like char
>>>> *cookie_domain_lower, *host_lower (no space between * and the variable
>>>> name), otherwise...
>>>>
>>>>>    const psl_ctx_t *psl;
>>>>>    int is_acceptable;
>>>>>
>>>>> @@ -519,7 +530,18 @@ check_domain_match (const char *cookie_domain, const 
>>>>> char *host)
>>>>>        goto no_psl;
>>>>>      }
>>>>>
>>>>> -  is_acceptable = psl_is_cookie_domain_acceptable (psl, host, 
>>>>> cookie_domain);
>>>>> +  if (psl_str_to_utf8lower (cookie_domain, NULL, NULL, 
>>>>> &cookie_domain_lower) != PSL_SUCCESS ||
>>>>> +      psl_str_to_utf8lower (host, NULL, NULL, &host_lower) != 
>>>>> PSL_SUCCESS)
>>>>
>>>> ...if the first "psl_str_to_utf8lower" fails then "host_lower" keeps
>>>> some bogus value...
>>>>
>>>>> +    {
>>>>> +        DEBUGP (("libpsl unable to parse domain name. "
>>>>> +                 "Falling back to simple heuristics.\n"));
>>>>> +        goto no_psl;
>>>>> +    }
>>>>> +
>>>>> +  is_acceptable = psl_is_cookie_domain_acceptable (psl, host_lower, 
>>>>> cookie_domain_lower);
>>>>> +  xfree (cookie_domain_lower);
>>>>> +  xfree (host_lower);
>>>>
>>>> ...and *boom* here.
>>>>
>>> Aah! I somehow managed not to get any "boom"s despite having a test
>>> that saw psl_str_to_utf8lower() fail. However, your comment is correct
>>> and I'll fix that. The general idea was that if the function fails, it
>>> will fail on both the calls
>>
>> I somehow misread the patch and the position of the no_psl label.  We
>> should move the two xfree in the cleanup block, after "no_psl", to avoid
>> a potential memory leak.
>>
>> Regards,
>> Giuseppe
>
>
>
> --
> Thanking You,
> Darshit Shah



-- 
Thanking You,
Darshit Shah