Re: [Bug-wget] [Bug-Wget] Misc. patches
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 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 wrote:
>> Darshit Shah 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
Re: [Bug-wget] Script providing SOCKS proxy support
Maybe we can maintain a contrib/ directory like other projects? This directory could ship extensions and wrapper scripts that make of Wget in common use cases. On Sat, Jul 19, 2014 at 3:28 AM, Ángel González wrote: > I have written a wrapping script for wget that -using tsocks- makes it > connect through a socks proxy if the environment variable socks_proxy > is set. > > I'm sharing it here as it may be of interest for a wider audience (or should > it > be included on git?) > > Regards > -- Thanking You, Darshit Shah
Re: [Bug-wget] Dangling info->lfilename
Hi Gisle,
Could you please confirm if the above patch fixes the problem?
On Tue, Jun 17, 2014 at 7:29 PM, Giuseppe Scrivano wrote:
> Hello,
>
> "Gisle Vanem" writes:
>
>> Hello Giuseppe.
>>
>> You patch to mswindows.c at
>>
>> http://git.savannah.gnu.org/cgit/wget.git/commit/src/mswindows.c?id=8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2
>>
>> @@ -123,7 +123,7 @@ struct fake_fork_info
>> {
>> HANDLE event;
>> bool logfile_changed;
>> - char lfilename[MAX_PATH + 1];
>> + char *lfilename;
>> };
>>
>> Causes a dangling pointer after use. I.e.
>> info->lfilename = strdup (opt.lfilename);
>>
>> seems never to be freed. We should call free() *before*
>> 'info' gets unmapped or revert that change.
>
> I am not very familiar with that part of the code, so from my
> understanding that is a one-time operation and I've decided to leave the
> memory leak (I thought the same happened with "info" but now I am
> reading about MapViewOfFile and UnmapViewOfFile) instead of introducing
> an untested free that could cause a segfault.
>
> As I have no opportunity to test it now, does this fix solve the
> problem?
>
> ---
>
> diff --git a/src/mswindows.c b/src/mswindows.c
> index 3bdf217..1c80421 100644
> --- a/src/mswindows.c
> +++ b/src/mswindows.c
> @@ -293,6 +293,7 @@ fake_fork (void)
>if (info->logfile_changed)
> printf (_("Output will be written to %s.\n"), quote (info->lfilename));
>
> + free(info->lfilename);
>UnmapViewOfFile (info);
>
> cleanup:
>
> ---
>
>
> I wonder if --background justifies all this mess in the code...maybe it
> is time to obsolete it and remove in future? We shouldn't really
> reinvent the shell. Is anyone using it?
>
> Regards,
> Giuseppe
>
--
Thanking You,
Darshit Shah
Re: [Bug-wget] [PATCH] Allow to redefine ciphers list for OpenSSL
I'm not sure, but looking at the patch, it /does/ seem like it tries to override the user settings, which IMO should not happen. If that is indeed the case, I do not support this patch either. @Giuseppe: About the failing test, that particular test seems to have some weird timing problems. I'm assuming it is probably a fault in the perl server. It randomly fails for me too. However, if you run make check again, chances are the test will pass. :) On Sat, Jul 19, 2014 at 3:12 AM, Ángel González wrote: > On 17/07/14 13:49, Tomas Hozza wrote: >> >> I agree. The patch didn't take any configuration possibility from the >> user. >> The users would be able to configure whatever in the same way they were >> before. >> >> Please really see some of those patches I sent. The discussion was little >> bit confusing at some points ~ like the intentions were interpreted >> differently. >> >> Regards, > > > I still strongly oppose to the patch. If the user configures wget to only > use Perfect > Forward Security, and your patch makes wget connect to a server not using it > you > are overriding user configuration (in the weakening direction). > See my last email for details. > > Patch v3 also seem to coalesce the different options of --secure-protocol if > using > GnuTLS, which IMHO doesn't make sense either. > > PS: s/cipers/ciphers/ in v3 > > -- Thanking You, Darshit Shah
[Bug-wget] (no subject)
hello sir.. not execute comment wget because some download file than this occurs error .
Re: [Bug-wget] Dangling info->lfilename
"Darshit Shah" wrote:
Could you please confirm if the above patch fixes the problem?
Guiseppe have reverted that relevant change. I.e. it's now a
buffer like it used to:
@@ -123,7 +123,7 @@ struct fake_fork_info
{
HANDLE event;
bool logfile_changed;
- char lfilename[MAX_PATH + 1];
+ char *lfilename;
};
2014-06-19 Giuseppe Scrivano
* mswindows.c (fake_fork_child): Revert dinamic allocation of
info->lfilename.
Reported by: Gisle Vanem
Re: [Bug-wget] Dangling info->lfilename
Could you please confirm if the above patch fixes the problem? Guiseppe have reverted that relevant change. I.e. it's now a buffer like it used to: So yes, that patch fixes the problem of the dangling pointer. --gv
Re: [Bug-wget] Script providing SOCKS proxy support
On 19/07/14 18:39, Darshit Shah wrote: Maybe we can maintain a contrib/ directory like other projects? This directory could ship extensions and wrapper scripts that make of Wget in common use cases. That was my first thought, although I then noticed that util folder is very much like that.
Re: [Bug-wget] [Bug-Wget] Misc. patches
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 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
wrote:
> >> Darshit Shah 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
Re: [Bug-wget] Script providing SOCKS proxy support
Am Freitag, 18. Juli 2014, 23:58:58 schrieb Ángel González: > I have written a wrapping script for wget that -using tsocks- makes it > connect through a socks proxy if the environment variable socks_proxy > is set. > > I'm sharing it here as it may be of interest for a wider audience (or > should it > be included on git?) Thanks for sharing (I guess I have to polish my bash knowledge ;-). Could you explicitely name a license for your code (and maybe put it in the top comment) ? That would clarify under what circumstances the code can be used. Regards, Tim
