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 <dar...@gmail.com> 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 <gscriv...@gnu.org>
wrote:
> >> Darshit Shah <dar...@gmail.com> 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
