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

[Tim Rühsen]

ACK from here.

And please also amend 
        return true ? (is_acceptable == 1) : false;
to
        return is_acceptable == 1;

Regards, Tim

Am Samstag, 19. Juli 2014, 22:05:26 schrieb 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