Re: bug and patch: blank spaces in filenames causes looping
On Jul 13, 2007, at 12:29 PM, Micah Cowan wrote: sprintf(filecopy, \%.2047s\, file); This fix breaks the FTP protocol, making wget instantly stop working with many conforming servers, but apparently start working with yours; the RFCs are very clear that the file name argument starts right after the string RETR ; the very next character is part of the file name, including if the next character is a space (or a quote). The file name is terminated by the CR LF sequence (which implies that the sequence CR LF may not occcur in the filename). Therefore, if you ask for a file file.txt, a conforming server will attempt to find and deliver a file whose name begins and ends with double-quotes. Therefore, this seems like a server problem. I think you may well be correct. I am now unable to reproduce the problem where the server does not recognize a filename unless I give it quotes. In fact, as you say, the server ONLY recognizes filenames WITHOUT quotes and quoting breaks it. I had to revert to the non- quoted code to get proper behavior. I am very confused now. I apologize profusely for wasting your time. How embarrassing! I'll save this email, and if I see the behavior again, I will provide you with the details you requested below. Could you please provide the following: 1. The version of wget you are running (wget --version) 2. The exact command line you are using to invoke wget 3. The output of that same command line, run with --debug -- Rich wealthychef Cook 925-784-3077 -- it takes many small steps to climb a mountain, but the view gets better all the time.
Re: bug and patch: blank spaces in filenames causes looping
On 7/15/07, Rich Cook [EMAIL PROTECTED] wrote: I think you may well be correct. I am now unable to reproduce the problem where the server does not recognize a filename unless I give it quotes. In fact, as you say, the server ONLY recognizes filenames WITHOUT quotes and quoting breaks it. I had to revert to the non- quoted code to get proper behavior. I am very confused now. I apologize profusely for wasting your time. How embarrassing! I'll save this email, and if I see the behavior again, I will provide you with the details you requested below. I wouldn't say it was a waste of time. Actually, I think it's good for us to know that this problem exists on some servers. We're considering writing a patch to recognise servers that do not support spaces. If the standard method fails, then it will retry as an escaped character. Nothing has been written for this yet, but it has been discussed, and may be implemented in the future.
Re: bug and patch: blank spaces in filenames causes looping
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Rich Cook wrote: On Jul 13, 2007, at 12:29 PM, Micah Cowan wrote: sprintf(filecopy, \%.2047s\, file); This fix breaks the FTP protocol, making wget instantly stop working with many conforming servers, but apparently start working with yours; the RFCs are very clear that the file name argument starts right after the string RETR ; the very next character is part of the file name, including if the next character is a space (or a quote). The file name is terminated by the CR LF sequence (which implies that the sequence CR LF may not occcur in the filename). Therefore, if you ask for a file file.txt, a conforming server will attempt to find and deliver a file whose name begins and ends with double-quotes. Therefore, this seems like a server problem. I think you may well be correct. I am now unable to reproduce the problem where the server does not recognize a filename unless I give it quotes. In fact, as you say, the server ONLY recognizes filenames WITHOUT quotes and quoting breaks it. I had to revert to the non-quoted code to get proper behavior. I am very confused now. I apologize profusely for wasting your time. How embarrassing! No worries, it happens! Sometimes the tests we run go other than we think they did. :) I'll save this email, and if I see the behavior again, I will provide you with the details you requested below. That would be terrific, thanks. - -- Micah J. Cowan Programmer, musician, typesetting enthusiast, gamer... http://micah.cowan.name/ -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGmpOD7M8hyUobTrERCA7FAJ4oygvX7rpQy1k5FL7j3R12LUdWUACfVHrc sk1tpS12pDYBvVbD4Nv7/I4= =KCxk -END PGP SIGNATURE-
Re: bug and patch: blank spaces in filenames causes looping
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Rich Cook wrote: On OS X, if a filename on the FTP server contains spaces, and the remote copy of the file is newer than the local, then wget gets thrown into a loop of No such file or directory endlessly. I have changed the following in ftp-simple.c, and this fixes the error. Sorry, I don't know how to use the proper patch formatting, but it should be clear. I and another developer could not reproduce this problem, either in the current trunk or in wget 1.10.2. sprintf(filecopy, \%.2047s\, file); This fix breaks the FTP protocol, making wget instantly stop working with many conforming servers, but apparently start working with yours; the RFCs are very clear that the file name argument starts right after the string RETR ; the very next character is part of the file name, including if the next character is a space (or a quote). The file name is terminated by the CR LF sequence (which implies that the sequence CR LF may not occcur in the filename). Therefore, if you ask for a file file.txt, a conforming server will attempt to find and deliver a file whose name begins and ends with double-quotes. Therefore, this seems like a server problem. Could you please provide the following: 1. The version of wget you are running (wget --version) 2. The exact command line you are using to invoke wget 3. The output of that same command line, run with --debug Thank you very much. - -- Micah J. Cowan Programmer, musician, typesetting enthusiast, gamer... http://micah.cowan.name/ -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGl9KT7M8hyUobTrERCJfoAJ91z9c2GniuoaX0mj9oqzHrrpNCtQCePQnm lvbVe0i5/jVy9V10uQpYgmk= =iQq1 -END PGP SIGNATURE-
Re: bug and patch: blank spaces in filenames causes looping
From various:
[...]
char filecopy[2048];
if (file[0] != '') {
sprintf(filecopy, \%.2047s\, file);
} else {
strncpy(filecopy, file, 2047);
}
[...]
It should be:
sprintf(filecopy, \%.2045s\, file);
[...]
I'll admit to being old and grumpy, but am I the only one who
shudders when one small code segment contains 2048, 2047, and 2045
as separate, independent literal constants, instead of using a macro, or
sizeof, or something which would let the next fellow change one buffer
size in one place, instead of hunting all over the code looking for
every 20xx which might be related?
Just a thought.
Steven M. Schweda [EMAIL PROTECTED]
382 South Warwick Street(+1) 651-699-9818
Saint Paul MN 55105-2547
Re: bug and patch: blank spaces in filenames causes looping
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256
Steven M. Schweda wrote:
From various:
[...]
char filecopy[2048];
if (file[0] != '') {
sprintf(filecopy, \%.2047s\, file);
} else {
strncpy(filecopy, file, 2047);
}
[...]
It should be:
sprintf(filecopy, \%.2045s\, file);
[...]
I'll admit to being old and grumpy, but am I the only one who
shudders when one small code segment contains 2048, 2047, and 2045
as separate, independent literal constants, instead of using a macro, or
sizeof, or something which would let the next fellow change one buffer
size in one place, instead of hunting all over the code looking for
every 20xx which might be related?
Well, as already mentioned, aprintf() would be much more appropriate, as
it elminates the need for constants like these.
And yes, magic numbers drive me crazy, too. Of course, when used with
printf's 's' specifier, it needs special handling (crafting a STR()
macro or somesuch).
- --
Micah J. Cowan
Programmer, musician, typesetting enthusiast, gamer...
http://micah.cowan.name/
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFGjxcX7M8hyUobTrERCHSAAJ9VkQdfhK4/LwByseYH2ZYVzoPqPwCePU1k
2Llybpq/oceXWMyZpBO4bPY=
=Vj/R
-END PGP SIGNATURE-
RE: bug and patch: blank spaces in filenames causes looping
There is a buffer overflow in the following line of the proposed code:
sprintf(filecopy, \%.2047s\, file);
It should be:
sprintf(filecopy, \%.2045s\, file);
in order to leave room for the two quotes.
Tony
-Original Message-
From: Rich Cook [mailto:[EMAIL PROTECTED]
Sent: Wednesday, July 04, 2007 10:18 AM
To: [EMAIL PROTECTED]
Subject: bug and patch: blank spaces in filenames causes looping
On OS X, if a filename on the FTP server contains spaces, and the
remote copy of the file is newer than the local, then wget gets
thrown into a loop of No such file or directory endlessly. I have
changed the following in ftp-simple.c, and this fixes the error.
Sorry, I don't know how to use the proper patch formatting, but it
should be clear.
==
the beginning of ftp_retr:
=
/* Sends RETR command to the FTP server. */
uerr_t
ftp_retr (int csock, const char *file)
{
char *request, *respline;
int nwritten;
uerr_t err;
/* Send RETR request. */
request = ftp_request (RETR, file);
==
becomes:
==
/* Sends RETR command to the FTP server. */
uerr_t
ftp_retr (int csock, const char *file)
{
char *request, *respline;
int nwritten;
uerr_t err;
char filecopy[2048];
if (file[0] != '') {
sprintf(filecopy, \%.2047s\, file);
} else {
strncpy(filecopy, file, 2047);
}
/* Send RETR request. */
request = ftp_request (RETR, filecopy);
--
Rich wealthychef Cook
925-784-3077
--
it takes many small steps to climb a mountain, but the view gets
better all the time.
Re: bug and patch: blank spaces in filenames causes looping
Good point, although it's only a POTENTIAL buffer overflow, and it's
limited to 2 bytes, so at least it's not exploitable. :-)
On Jul 5, 2007, at 9:05 AM, Tony Lewis wrote:
There is a buffer overflow in the following line of the proposed code:
sprintf(filecopy, \%.2047s\, file);
It should be:
sprintf(filecopy, \%.2045s\, file);
in order to leave room for the two quotes.
Tony
-Original Message-
From: Rich Cook [mailto:[EMAIL PROTECTED]
Sent: Wednesday, July 04, 2007 10:18 AM
To: [EMAIL PROTECTED]
Subject: bug and patch: blank spaces in filenames causes looping
On OS X, if a filename on the FTP server contains spaces, and the
remote copy of the file is newer than the local, then wget gets
thrown into a loop of No such file or directory endlessly. I have
changed the following in ftp-simple.c, and this fixes the error.
Sorry, I don't know how to use the proper patch formatting, but it
should be clear.
==
the beginning of ftp_retr:
=
/* Sends RETR command to the FTP server. */
uerr_t
ftp_retr (int csock, const char *file)
{
char *request, *respline;
int nwritten;
uerr_t err;
/* Send RETR request. */
request = ftp_request (RETR, file);
==
becomes:
==
/* Sends RETR command to the FTP server. */
uerr_t
ftp_retr (int csock, const char *file)
{
char *request, *respline;
int nwritten;
uerr_t err;
char filecopy[2048];
if (file[0] != '') {
sprintf(filecopy, \%.2047s\, file);
} else {
strncpy(filecopy, file, 2047);
}
/* Send RETR request. */
request = ftp_request (RETR, filecopy);
--
Rich wealthychef Cook
925-784-3077
--
it takes many small steps to climb a mountain, but the view gets
better all the time.
--
Rich wealthychef Cook
925-784-3077
--
it takes many small steps to climb a mountain, but the view gets
better all the time.
RE: bug and patch: blank spaces in filenames causes looping
-Original Message- From: Hrvoje Niksic [mailto:[EMAIL PROTECTED] Tony Lewis [EMAIL PROTECTED] writes: Wget has an `aprintf' utility function that allocates the result on the heap. Avoids both buffer overruns and arbitrary limits on file name length. If it uses the heap, then doesn't that open a hole where a particularly long file name would overflow the heap? -- URL: http://wiki.tcl.tk/ Even if explicitly stated to the contrary, nothing in this posting should be construed as representing my employer's opinions. URL: mailto:[EMAIL PROTECTED] URL: http://www.purl.org/NET/lvirden/
Re: bug and patch: blank spaces in filenames causes looping
Tony Lewis [EMAIL PROTECTED] writes: There is a buffer overflow in the following line of the proposed code: sprintf(filecopy, \%.2047s\, file); Wget has an `aprintf' utility function that allocates the result on the heap. Avoids both buffer overruns and arbitrary limits on file name length.
Re: bug and patch: blank spaces in filenames causes looping
Rich Cook [EMAIL PROTECTED] writes: Trouble is, it's undocumented as to how to free the resulting string. Do I call free on it? Yes. Freshly allocated with malloc in the function documentation was supposed to indicate how to free the string.
Re: bug and patch: blank spaces in filenames causes looping
Virden, Larry W. [EMAIL PROTECTED] writes: Tony Lewis [EMAIL PROTECTED] writes: Wget has an `aprintf' utility function that allocates the result on the heap. Avoids both buffer overruns and arbitrary limits on file name length. If it uses the heap, then doesn't that open a hole where a particularly long file name would overflow the heap? No, aprintf tries to allocate as much memory as necessary. If the memory is unavailable, malloc returns NULL and Wget exits.
Re: bug and patch: blank spaces in filenames causes looping
Trouble is, it's undocumented as to how to free the resulting string. Do I call free on it? I'd use asprintf, but I'm afraid to suggest that here as it may not be portable. On Jul 5, 2007, at 10:45 AM, Hrvoje Niksic wrote: Tony Lewis [EMAIL PROTECTED] writes: There is a buffer overflow in the following line of the proposed code: sprintf(filecopy, \%.2047s\, file); Wget has an `aprintf' utility function that allocates the result on the heap. Avoids both buffer overruns and arbitrary limits on file name length. -- Rich wealthychef Cook 925-784-3077 -- it takes many small steps to climb a mountain, but the view gets better all the time.
Re: bug and patch: blank spaces in filenames causes looping
On Jul 5, 2007, at 11:08 AM, Hrvoje Niksic wrote: Rich Cook [EMAIL PROTECTED] writes: Trouble is, it's undocumented as to how to free the resulting string. Do I call free on it? Yes. Freshly allocated with malloc in the function documentation was supposed to indicate how to free the string. Oh, I looked in the source and there was this xmalloc thing that didn't show up in my man pages, so I punted. Sorry. -- ✐There's no time to stop for gas, we're already late-- Karin Donker -- Rich wealthychef Cook http://5pmharmony.com 925-784-3077 -- ✐
RE: bug and patch: blank spaces in filenames causes looping
Please remove me from this list. thanks, John Bruso From: Rich Cook [mailto:[EMAIL PROTECTED] Sent: Thu 7/5/2007 12:30 PM To: Hrvoje Niksic Cc: Tony Lewis; [EMAIL PROTECTED] Subject: Re: bug and patch: blank spaces in filenames causes looping On Jul 5, 2007, at 11:08 AM, Hrvoje Niksic wrote: Rich Cook [EMAIL PROTECTED] writes: Trouble is, it's undocumented as to how to free the resulting string. Do I call free on it? Yes. Freshly allocated with malloc in the function documentation was supposed to indicate how to free the string. Oh, I looked in the source and there was this xmalloc thing that didn't show up in my man pages, so I punted. Sorry. -- ?There's no time to stop for gas, we're already late-- Karin Donker -- Rich wealthychef Cook http://5pmharmony.com http://5pmharmony.com/ 925-784-3077 -- ?
Re: bug and patch: blank spaces in filenames causes looping
Rich Cook [EMAIL PROTECTED] writes: On Jul 5, 2007, at 11:08 AM, Hrvoje Niksic wrote: Rich Cook [EMAIL PROTECTED] writes: Trouble is, it's undocumented as to how to free the resulting string. Do I call free on it? Yes. Freshly allocated with malloc in the function documentation was supposed to indicate how to free the string. Oh, I looked in the source and there was this xmalloc thing that didn't show up in my man pages, so I punted. Sorry. No problem. Note that xmalloc isn't entirely specific to Wget, it's a fairly standard GNU name for a malloc-or-die function. Now I remembered that Wget also has xfree, so the above advice is not entirely correct -- you should call xfree instead. However, in the normal case xfree is a simple wrapper around free, so even if you used free, it would have worked just as well. (The point of xfree is that if you compile with DEBUG_MALLOC, you get a version that check for leaks, although it should be removed now that there is valgrind, which does the same job much better. There is also the business of barfing on NULL pointers, which should also be removed.) I'd have implemented a portable asprintf, but I liked the aprintf interface better (I first saw it in libcurl).
Re: bug and patch: blank spaces in filenames causes looping
So forgive me for a newbie-never-even-lurked kind of question: will this fix make it into wget for other users (and for me in the future)? Or do I need to do more to make that happen, or...? Thanks! On Jul 5, 2007, at 12:52 PM, Hrvoje Niksic wrote: Rich Cook [EMAIL PROTECTED] writes: On Jul 5, 2007, at 11:08 AM, Hrvoje Niksic wrote: Rich Cook [EMAIL PROTECTED] writes: Trouble is, it's undocumented as to how to free the resulting string. Do I call free on it? Yes. Freshly allocated with malloc in the function documentation was supposed to indicate how to free the string. Oh, I looked in the source and there was this xmalloc thing that didn't show up in my man pages, so I punted. Sorry. No problem. Note that xmalloc isn't entirely specific to Wget, it's a fairly standard GNU name for a malloc-or-die function. Now I remembered that Wget also has xfree, so the above advice is not entirely correct -- you should call xfree instead. However, in the normal case xfree is a simple wrapper around free, so even if you used free, it would have worked just as well. (The point of xfree is that if you compile with DEBUG_MALLOC, you get a version that check for leaks, although it should be removed now that there is valgrind, which does the same job much better. There is also the business of barfing on NULL pointers, which should also be removed.) I'd have implemented a portable asprintf, but I liked the aprintf interface better (I first saw it in libcurl). -- ✐There's no time to stop for gas, we're already late-- Karin Donker -- Rich wealthychef Cook http://5pmharmony.com 925-784-3077 -- ✐
Re: bug and patch: blank spaces in filenames causes looping
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Rich Cook wrote: So forgive me for a newbie-never-even-lurked kind of question: will this fix make it into wget for other users (and for me in the future)? Or do I need to do more to make that happen, or...? Thanks! Well, I need a chance to look over the patch, run some tests, etc, to see if it really covers everything it should (what about other, non-space characters?). The fix (or one like it) will probably make it into Wget at some point, but I wouldn't expect it to come out in the next release (which, itself, will not be arriving for a couple months); it will probably go into wget 1.12. - -- Micah J. Cowan Programmer, musician, typesetting enthusiast, gamer... http://micah.cowan.name/ -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGjXYj7M8hyUobTrERCI5JAJ0UIDGzQsC8xCI3lK26pzzQ+BkS6ACgj16o oWDlelFyfvvTlhtlDpLYLXM= =DZ8v -END PGP SIGNATURE-
Re: bug and patch: blank spaces in filenames causes looping
Thanks for the follow up. :-) On Jul 5, 2007, at 3:52 PM, Micah Cowan wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Rich Cook wrote: So forgive me for a newbie-never-even-lurked kind of question: will this fix make it into wget for other users (and for me in the future)? Or do I need to do more to make that happen, or...? Thanks! Well, I need a chance to look over the patch, run some tests, etc, to see if it really covers everything it should (what about other, non-space characters?). The fix (or one like it) will probably make it into Wget at some point, but I wouldn't expect it to come out in the next release (which, itself, will not be arriving for a couple months); it will probably go into wget 1.12. - -- Micah J. Cowan Programmer, musician, typesetting enthusiast, gamer... http://micah.cowan.name/ -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.6 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGjXYj7M8hyUobTrERCI5JAJ0UIDGzQsC8xCI3lK26pzzQ+BkS6ACgj16o oWDlelFyfvvTlhtlDpLYLXM= =DZ8v -END PGP SIGNATURE- -- ✐There's no time to stop for gas, we're already late-- Karin Donker -- Rich wealthychef Cook http://5pmharmony.com 925-784-3077 -- ✐
