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

[Darshit Shah]

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