Re: [Bug-wget] [Patch] fix bug #39175 Header value length limited with 256
On Friday 17 April 2015 12:57:49 Miquel Llobet wrote: > On Sun, Mar 15, 2015 at 10:42 PM, Giuseppe Scrivano > > wrote: > > since there is a reasonable upperbound value, can't we just have > > something like: "char *hdrval = xmalloc(8192);"? > > Done, I also changed all the resp_header_copy to resp_header_strdup to copy > the header dinamically. As a result resp_header_copy is now unused, should > it be deleted as headers have no fixed size? > > I applied my changes on top of Hubert's gethttp single exit point patch, so > that should go first. Hi Miquel, there seem to be memory leaks in your patch. You overwrite the previously malloc'ed 'hdrval ' whenever you call resp_header_strdup(). Just stay with Giuseppe's proposal >> - char hdrval[512]; >> + char hdrval[8190]; -> char *hdrval = xmalloc(8192); Regards, Tim signature.asc Description: This is a digitally signed message part.
Re: [Bug-wget] [Patch] fix bug #39175 Header value length limited with 256
On Sun, Mar 15, 2015 at 10:42 PM, Giuseppe Scrivano wrote: > since there is a reasonable upperbound value, can't we just have > something like: "char *hdrval = xmalloc(8192);"? > Done, I also changed all the resp_header_copy to resp_header_strdup to copy the header dinamically. As a result resp_header_copy is now unused, should it be deleted as headers have no fixed size? I applied my changes on top of Hubert's gethttp single exit point patch, so that should go first. Best, Miquel 0001-Patch-Fixed-39175-Header-value-length-limit.patch Description: Binary data
Re: [Bug-wget] [Patch] fix bug #39175 Header value length limited with 256
Miquel Llobet writes: > Could this be dinamically allocated? > > Indeed it can! Thanks Giuseppe and Darshit for pointing out. Ideally > we should change all the resp_header_copy calls to resp_header_strdup > (dynamic resp header copy). There is a bit to code to change, as the > function returns are different etc. expect a big patch. since there is a reasonable upperbound value, can't we just have something like: "char *hdrval = xmalloc(8192);"? What is left to do is ensure we don't leak this buffer when the function exits. Regards, Giuseppe
Re: [Bug-wget] [Patch] fix bug #39175 Header value length limited with 256
> > Could this be dinamically allocated? Indeed it can! Thanks Giuseppe and Darshit for pointing out. Ideally we should change all the resp_header_copy calls to resp_header_strdup (dynamic resp header copy). There is a bit to code to change, as the function returns are different etc. expect a big patch. I will also make a test for long header sizes as Tim suggested. Cheers, Miquel Llobet On Sat, Mar 14, 2015 at 8:34 PM, Darshit Shah wrote: > Hi Miquel, > > Thanks a lot for making this contribution. > > I not a supporter of making such large (8K is gigantic!) memory > assignments on the stack. Wget runs not only on the latest x86_64 > processors, but often also on ancient and obscure architectures. And > assignment of a variable using 8K of memory on the stack may not be > possible on some architectures. > Instead, I think this should be done via dynamic memory allocation. > > > On 03/12, Miquel Llobet wrote: > >> Increased the header buffer to 8Kb, as there are no limits to the size of >> field name, values or headers themselves. While the current value is big >> enough, other projects such as Apache [1] or nginx have limits of 4-8Kb. >> >> If we want to allow for arbitrary size headers we should use >> resp_header_strdup instead of resp_header_copy, but this new value should >> be enough. >> >> --- src/http.c.orig 2015-03-12 21:50:03.0 +0100 >> +++ src/http.c 2015-03-12 21:04:08.0 +0100 >> @@ -1695,7 +1695,7 @@ >> >> char *head; >> struct response *resp; >> - char hdrval[512]; >> + char hdrval[8190]; >> char *message; >> >> /* Declare WARC variables. */ >> >> [1]: https://httpd.apache.org/docs/2.2/mod/core.html# >> limitrequestfieldsize >> >> Miquel Llobet >> > --- end quoted text --- > > -- > Thanking You, > Darshit Shah >
Re: [Bug-wget] [Patch] fix bug #39175 Header value length limited with 256
Hi Miquel, Thanks a lot for making this contribution. I not a supporter of making such large (8K is gigantic!) memory assignments on the stack. Wget runs not only on the latest x86_64 processors, but often also on ancient and obscure architectures. And assignment of a variable using 8K of memory on the stack may not be possible on some architectures. Instead, I think this should be done via dynamic memory allocation. On 03/12, Miquel Llobet wrote: Increased the header buffer to 8Kb, as there are no limits to the size of field name, values or headers themselves. While the current value is big enough, other projects such as Apache [1] or nginx have limits of 4-8Kb. If we want to allow for arbitrary size headers we should use resp_header_strdup instead of resp_header_copy, but this new value should be enough. --- src/http.c.orig 2015-03-12 21:50:03.0 +0100 +++ src/http.c 2015-03-12 21:04:08.0 +0100 @@ -1695,7 +1695,7 @@ char *head; struct response *resp; - char hdrval[512]; + char hdrval[8190]; char *message; /* Declare WARC variables. */ [1]: https://httpd.apache.org/docs/2.2/mod/core.html#limitrequestfieldsize Miquel Llobet --- end quoted text --- -- Thanking You, Darshit Shah pgpegkd6UbX0V.pgp Description: PGP signature
Re: [Bug-wget] [Patch] fix bug #39175 Header value length limited with 256
Miquel Llobet writes: > Increased the header buffer to 8Kb, as there are no limits to the size of > field name, values or headers themselves. While the current value is big > enough, other projects such as Apache [1] or nginx have limits of 4-8Kb. > > If we want to allow for arbitrary size headers we should use > resp_header_strdup instead of resp_header_copy, but this new value should > be enough. > > --- src/http.c.orig 2015-03-12 21:50:03.0 +0100 > +++ src/http.c 2015-03-12 21:04:08.0 +0100 > @@ -1695,7 +1695,7 @@ > >char *head; >struct response *resp; > - char hdrval[512]; > + char hdrval[8190]; Thanks for your contribution! Using a 8Kb buffer on the stack should be fine but I am not sure how many other buffers we are already allocating this way and how deep the stack will get. Could this be dinamically allocated? Also, please use a ChangeLog style for the commit message, take a look at the latest commit on the git repository to have an idea of how it should look. Giuseppe
Re: [Bug-wget] [Patch] fix bug #39175 Header value length limited with 256
On Thursday 12 March 2015 22:02:46 Miquel Llobet wrote: > Increased the header buffer to 8Kb, as there are no limits to the size of > field name, values or headers themselves. While the current value is big > enough, other projects such as Apache [1] or nginx have limits of 4-8Kb. > > If we want to allow for arbitrary size headers we should use > resp_header_strdup instead of resp_header_copy, but this new value should > be enough. > > --- src/http.c.orig 2015-03-12 21:50:03.0 +0100 > +++ src/http.c 2015-03-12 21:04:08.0 +0100 > @@ -1695,7 +1695,7 @@ > >char *head; >struct response *resp; > - char hdrval[512]; > + char hdrval[8190]; >char *message; > >/* Declare WARC variables. */ > > [1]: https://httpd.apache.org/docs/2.2/mod/core.html#limitrequestfieldsize > > Miquel Llobet Hi Miquel, good idea to accept larger header sizes. Could you please add a test case with a "long" header line sent by the server ? Or maybe just extending testenv/Test-cookie-... ? Set-Cookie header lines may be large, also the Location header (for redirection) with a long URL ? Tim signature.asc Description: This is a digitally signed message part.
