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
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
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
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
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 -- ✐
