Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
On Thursday 23 October 2014 08:40:34 Giuseppe Scrivano wrote: Ángel González keis...@gmail.com writes: Sigh, I had locally added xstrndup to bootstrap.conf but failed to include it in the commit. I'm fine with the changes. Feel free to push. pushed! Thanks, Giuseppe Hi Giuseppe, I just made a git pull of the 1.16 version. And it seems that this patch is missing... thus CSS parsing is broken. (BTW, same as https://savannah.gnu.org/bugs/?42953) How could this happen ? Tim
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
On Monday 27 October 2014 15:16:08 Tim Ruehsen wrote: On Thursday 23 October 2014 08:40:34 Giuseppe Scrivano wrote: Ángel González keis...@gmail.com writes: Sigh, I had locally added xstrndup to bootstrap.conf but failed to include it in the commit. I'm fine with the changes. Feel free to push. pushed! Thanks, Giuseppe Hi Giuseppe, I just made a git pull of the 1.16 version. And it seems that this patch is missing... thus CSS parsing is broken. (BTW, same as https://savannah.gnu.org/bugs/?42953) How could this happen ? Tim Arg, sorry for the noise. It was in a not-up-to-date local branch. master looks fine !!! Tim
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
Tim Ruehsen tim.rueh...@gmx.de writes: On Thursday 23 October 2014 08:40:34 Giuseppe Scrivano wrote: Ángel González keis...@gmail.com writes: Sigh, I had locally added xstrndup to bootstrap.conf but failed to include it in the commit. I'm fine with the changes. Feel free to push. pushed! Thanks, Giuseppe Hi Giuseppe, I just made a git pull of the 1.16 version. And it seems that this patch is missing... thus CSS parsing is broken. (BTW, same as https://savannah.gnu.org/bugs/?42953) How could this happen ? was the patch pushed? We have only fast-forward push enabled for the repository so history is linear and we can't force any push. Anyway, we were in a hurry for this release to fix the reported security issue, we can make a new one soon. Regards, Giuseppe
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
Ángel González keis...@gmail.com writes: On 21/10/14 09:50, Giuseppe Scrivano wrote: Ángel, thanks for the debugging effort! Would you mind to send a separate patch that doesn't use strncpy (so to make make dist-check happy), probably we can just use memcpy here. Regards, Giuseppe The revert to the old behavior was only intended for checking it was the right place. We have xstrndup() to do just what we want. Please test. in any case this part is wrong as it changes how the function works, and I think we should fix it anyway. Good idea to use xstrndup. Regards, Giuseppe
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
Am Mittwoch, 22. Oktober 2014, 01:20:17 schrieb Ángel González: On 21/10/14 09:50, Giuseppe Scrivano wrote: Ángel, thanks for the debugging effort! Would you mind to send a separate patch that doesn't use strncpy (so to make make dist-check happy), probably we can just use memcpy here. Regards, Giuseppe The revert to the old behavior was only intended for checking it was the right place. We have xstrndup() to do just what we want. Please test. Regards Hi Ángel, + return xstrndup (uri, at + *pos, *length);; Shouldn't it be + return xstrndup (at + *pos, *length); ? signature.asc Description: This is a digitally signed message part.
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
Tim Rühsen tim.rueh...@gmx.de writes: Am Mittwoch, 22. Oktober 2014, 01:20:17 schrieb Ángel González: On 21/10/14 09:50, Giuseppe Scrivano wrote: Ángel, thanks for the debugging effort! Would you mind to send a separate patch that doesn't use strncpy (so to make make dist-check happy), probably we can just use memcpy here. Regards, Giuseppe The revert to the old behavior was only intended for checking it was the right place. We have xstrndup() to do just what we want. Please test. Regards Hi Ángel, + return xstrndup (uri, at + *pos, *length);; Shouldn't it be + return xstrndup (at + *pos, *length); yes... Angel, if you are ok with, I can push this version (I've done added the gnulib module and changed a bit the changelog entry): Regards, Giuseppe From 601b282cd8e7b2783f818469f55923e91cc4e1a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81ngel=20Gonz=C3=A1lez?= keis...@gmail.com Date: Wed, 22 Oct 2014 01:10:21 +0200 Subject: [PATCH] css-url.c (get_uri_string): Fix regression from 8e6de1fb5 Solves the issue discovered by Gabriel Somlo and reported in the ml thread Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2) --- ChangeLog | 4 bootstrap.conf | 1 + src/ChangeLog | 4 src/css-url.c | 3 ++- 4 files changed, 11 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index d1b755a..eca59da 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2013-10-22 Ángel González keis...@gmail.com + + * bootstrap.conf (gnulib_modules): Add module xstrndup. + 2014-09-25 Tim Ruehsen tim.rueh...@gmx.de * configure.ac: removed WgetTest.pm.in diff --git a/bootstrap.conf b/bootstrap.conf index bbfb38f..11f5f92 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -78,6 +78,7 @@ update-copyright vasprintf vsnprintf write +xstrndup gnulib_extra_files= diff --git a/src/ChangeLog b/src/ChangeLog index d5aeca0..bdc6844 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,7 @@ +2013-10-22 Ángel González keis...@gmail.com + + * css-url.c (get_uri_string): Honor the specified length argument. + 2014-10-16 Tim Ruehsen tim.rueh...@gmx.de * url.c (url_parse): little code cleanup diff --git a/src/css-url.c b/src/css-url.c index c605798..8ee4e8c 100644 --- a/src/css-url.c +++ b/src/css-url.c @@ -52,6 +52,7 @@ as that of the covered work. */ #include html-url.h #include css-tokens.h #include css-url.h +#include xstrndup.h /* from lex.yy.c */ extern char *yytext; @@ -97,7 +98,7 @@ get_uri_string (const char *at, int *pos, int *length) *length -= 2; } - return xstrdup (at + *pos); + return xstrndup (at + *pos, *length); } void -- 1.9.3
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
On 22/10/14 20:28, Giuseppe Scrivano wrote: Hi Ángel, + return xstrndup (uri, at + *pos, *length);; Shouldn't it be + return xstrndup (at + *pos, *length); yes... Angel, if you are ok with, I can push this version (I've done added the gnulib module and changed a bit the changelog entry): Regards, Giuseppe Sigh, I had locally added xstrndup to bootstrap.conf but failed to include it in the commit. I'm fine with the changes. Feel free to push.
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
On Wed, Oct 22, 2014 at 08:28:57PM +0200, Giuseppe Scrivano wrote: Tim R??hsen tim.rueh...@gmx.de writes: Am Mittwoch, 22. Oktober 2014, 01:20:17 schrieb ??ngel Gonz??lez: On 21/10/14 09:50, Giuseppe Scrivano wrote: ??ngel, thanks for the debugging effort! Would you mind to send a separate patch that doesn't use strncpy (so to make make dist-check happy), probably we can just use memcpy here. Regards, Giuseppe The revert to the old behavior was only intended for checking it was the right place. We have xstrndup() to do just what we want. Please test. Regards Hi ??ngel, + return xstrndup (uri, at + *pos, *length);; Shouldn't it be + return xstrndup (at + *pos, *length); yes... Angel, if you are ok with, I can push this version (I've done added the gnulib module and changed a bit the changelog entry): Just for the record, I tested this and it does fix the problem I was seeing. Thanks again, --Gabriel From 601b282cd8e7b2783f818469f55923e91cc4e1a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81ngel=20Gonz=C3=A1lez?= keis...@gmail.com Date: Wed, 22 Oct 2014 01:10:21 +0200 Subject: [PATCH] css-url.c (get_uri_string): Fix regression from 8e6de1fb5 Solves the issue discovered by Gabriel Somlo and reported in the ml thread Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2) --- ChangeLog | 4 bootstrap.conf | 1 + src/ChangeLog | 4 src/css-url.c | 3 ++- 4 files changed, 11 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index d1b755a..eca59da 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2013-10-22 ??ngel Gonz??lez keis...@gmail.com + + * bootstrap.conf (gnulib_modules): Add module xstrndup. + 2014-09-25 Tim Ruehsen tim.rueh...@gmx.de * configure.ac: removed WgetTest.pm.in diff --git a/bootstrap.conf b/bootstrap.conf index bbfb38f..11f5f92 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -78,6 +78,7 @@ update-copyright vasprintf vsnprintf write +xstrndup gnulib_extra_files= diff --git a/src/ChangeLog b/src/ChangeLog index d5aeca0..bdc6844 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,7 @@ +2013-10-22 ??ngel Gonz??lez keis...@gmail.com + + * css-url.c (get_uri_string): Honor the specified length argument. + 2014-10-16 Tim Ruehsen tim.rueh...@gmx.de * url.c (url_parse): little code cleanup diff --git a/src/css-url.c b/src/css-url.c index c605798..8ee4e8c 100644 --- a/src/css-url.c +++ b/src/css-url.c @@ -52,6 +52,7 @@ as that of the covered work. */ #include html-url.h #include css-tokens.h #include css-url.h +#include xstrndup.h /* from lex.yy.c */ extern char *yytext; @@ -97,7 +98,7 @@ get_uri_string (const char *at, int *pos, int *length) *length -= 2; } - return xstrdup (at + *pos); + return xstrndup (at + *pos, *length); } void -- 1.9.3
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
On 21/10/14 09:50, Giuseppe Scrivano wrote: Ángel, thanks for the debugging effort! Would you mind to send a separate patch that doesn't use strncpy (so to make make dist-check happy), probably we can just use memcpy here. Regards, Giuseppe The revert to the old behavior was only intended for checking it was the right place. We have xstrndup() to do just what we want. Please test. Regards From 66ae9d897a4a2b7458a5d61230dc316befddd731 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81ngel=20Gonz=C3=A1lez?= keis...@gmail.com Date: Wed, 22 Oct 2014 01:10:21 +0200 Subject: [PATCH] css-url.c (get_uri_string): Fix regression from 8e6de1fb5 Solves the issue discovered by Gabriel Somlo and reported in the ml thread Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2) --- src/ChangeLog | 4 src/css-url.c | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/ChangeLog b/src/ChangeLog index d5aeca0..6004c34 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,7 @@ +2013-10-22 Ãngel González keis...@gmail.com + + * css-url.c (get_uri_string): Fix regression from 8e6de1fb5. + 2014-10-16 Tim Ruehsen tim.rueh...@gmx.de * url.c (url_parse): little code cleanup diff --git a/src/css-url.c b/src/css-url.c index c605798..2ac1cef 100644 --- a/src/css-url.c +++ b/src/css-url.c @@ -45,6 +45,7 @@ as that of the covered work. */ #include stdlib.h #include ctype.h #include errno.h +#include xstrndup.h #include wget.h #include utils.h @@ -97,7 +98,7 @@ get_uri_string (const char *at, int *pos, int *length) *length -= 2; } - return xstrdup (at + *pos); + return xstrndup (uri, at + *pos, *length);; } void -- 2.1.2
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
On Wed, Oct 22, 2014 at 01:20:17AM +0200, ??ngel Gonz??lez wrote: On 21/10/14 09:50, Giuseppe Scrivano wrote: ??ngel, thanks for the debugging effort! Would you mind to send a separate patch that doesn't use strncpy (so to make make dist-check happy), probably we can just use memcpy here. Regards, Giuseppe The revert to the old behavior was only intended for checking it was the right place. We have xstrndup() to do just what we want. Please test. Build fails with: make[3]: Entering directory `/home/somlo/PROJECTS/CERT/WGETHACK/wget/src' gcc -DHAVE_CONFIG_H -DSYSTEM_WGETRC=\/usr/local/etc/wgetrc\ -DLOCALEDIR=\/usr/local/share/locale\ -I. -I../lib -I../lib -g -O2 -MT css-url.o -MD -MP -MF .deps/css-url.Tpo -c -o css-url.o css-url.c css-url.c:48:22: fatal error: xstrndup.h: No such file or directory #include xstrndup.h ^ compilation terminated. make[3]: *** [css-url.o] Error 1 There seems to be no -I../gnulib/lib on the gcc command line, which is probably why it can't find it. Thanks, --Gabriel From 66ae9d897a4a2b7458a5d61230dc316befddd731 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81ngel=20Gonz=C3=A1lez?= keis...@gmail.com Date: Wed, 22 Oct 2014 01:10:21 +0200 Subject: [PATCH] css-url.c (get_uri_string): Fix regression from 8e6de1fb5 Solves the issue discovered by Gabriel Somlo and reported in the ml thread Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2) --- src/ChangeLog | 4 src/css-url.c | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/ChangeLog b/src/ChangeLog index d5aeca0..6004c34 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,7 @@ +2013-10-22 ??ngel Gonz??lez keis...@gmail.com + + * css-url.c (get_uri_string): Fix regression from 8e6de1fb5. + 2014-10-16 Tim Ruehsen tim.rueh...@gmx.de * url.c (url_parse): little code cleanup diff --git a/src/css-url.c b/src/css-url.c index c605798..2ac1cef 100644 --- a/src/css-url.c +++ b/src/css-url.c @@ -45,6 +45,7 @@ as that of the covered work. */ #include stdlib.h #include ctype.h #include errno.h +#include xstrndup.h #include wget.h #include utils.h @@ -97,7 +98,7 @@ get_uri_string (const char *at, int *pos, int *length) *length -= 2; } - return xstrdup (at + *pos); + return xstrndup (uri, at + *pos, *length);; } void -- 2.1.2
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
On 21/10/14 01:20, Gabriel Somlo wrote:
Hi Giuseppe,
I think I found a regression in the development branch of wget, which
wasn't present in 1.15. Using git bisect, it appears the offending
commit was 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2 (Drop usage of
strncpy) from Jun. 9 2014.
To reproduce, grab a wide and shallow copy of wikipedia.org, like so:
wget -rpkEHN -e robots=off --random-wait -t 2 -U mozilla -l 1 \
-P ./vservers wikipedia.org
then (some 20 minutes or so later) open ./vservers/wikipedia.org/index.html
in your (firefox-32.0.2-1.fc20.x86_64) browser as a file.
Before commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2, the page looks
normal. After said commit, it looks (for me) like in the attached
screenshot.
I couldn't revert the commit, as subsequent changes have accumulated
since it was applied. I'm also relatively new to the code base, so
figuring out exactly what went wrong might be significantly faster
for you :)
I can (if necessary) open a ticket in the bug tracker (e.g. for a
better place to upload the screenshot, which may not make it through
on the mailing list), just let me know what you think.
Thanks much,
--Gabriel
Thanks for your report!
After a quick look, get_uri_string seems to not be taking into account
the end of the url() parameter (it was before 8e6de1fb5).
Can you check if this patch fixes the issue?
diff --git a/src/css-url.c b/src/css-url.c
index c605798..34a20af 100644
--- a/src/css-url.c
+++ b/src/css-url.c
@@ -72,6 +72,8 @@ extern int yylex (void);
static char *
get_uri_string (const char *at, int *pos, int *length)
{
+ char *uri;
+
if (0 != strncasecmp (at + *pos, url(, 4))
return NULL;
@@ -97,7 +99,14 @@ get_uri_string (const char *at, int *pos, int *length)
*length -= 2;
}
- return xstrdup (at + *pos);
+ uri = xmalloc (*length + 1);
+ if (uri)
+{
+ strncpy (uri, at + *pos, *length);
+ uri[*length] = '\0';
+}
+
+ return uri;
}
void
Re: [Bug-wget] Regression in git master branch (commit 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2)
On Tue, Oct 21, 2014 at 01:49:41AM +0200, ?ngel Gonz?lez wrote:
On 21/10/14 01:20, Gabriel Somlo wrote:
I think I found a regression in the development branch of wget, which
wasn't present in 1.15. Using git bisect, it appears the offending
commit was 8e6de1fb5ff0ca0c749da7db634a1b1e3a1215a2 (Drop usage of
strncpy) from Jun. 9 2014.
To reproduce, grab a wide and shallow copy of wikipedia.org, like so:
wget -rpkEHN -e robots=off --random-wait -t 2 -U mozilla -l 1 \
-P ./vservers wikipedia.org
then (some 20 minutes or so later) open ./vservers/wikipedia.org/index.html
in your (firefox-32.0.2-1.fc20.x86_64) browser as a file.
Thanks for your report!
After a quick look, get_uri_string seems to not be taking into account the
end of the url() parameter (it was before 8e6de1fb5).
Can you check if this patch fixes the issue?
Yeah, that took care of it for me !
Thanks again,
--Gabriel
diff --git a/src/css-url.c b/src/css-url.c
index c605798..34a20af 100644
--- a/src/css-url.c
+++ b/src/css-url.c
@@ -72,6 +72,8 @@ extern int yylex (void);
static char *
get_uri_string (const char *at, int *pos, int *length)
{
+ char *uri;
+
if (0 != strncasecmp (at + *pos, url(, 4))
return NULL;
@@ -97,7 +99,14 @@ get_uri_string (const char *at, int *pos, int *length)
*length -= 2;
}
- return xstrdup (at + *pos);
+ uri = xmalloc (*length + 1);
+ if (uri)
+{
+ strncpy (uri, at + *pos, *length);
+ uri[*length] = '\0';
+}
+
+ return uri;
}
void
