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 -- Thanking You, Darshit Shah