Re: bug and "patch": blank spaces in filenames causes looping

[Micah Cowan]

-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

[Josh Williams]

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

[Rich Cook]

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

[Micah Cowan]

-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

[Micah Cowan]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Steven M. Schweda wrote:
>>From :
> 
>> [...]
>>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

[Steven M. Schweda]

>From :

> [...]
>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

[Rich Cook]

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

925-784-3077
--
✐

Re: bug and "patch": blank spaces in filenames causes looping

[Micah Cowan]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Bruso, John wrote:
> Please remove me from this list. thanks,

Nobody on this list has the ability to do this, unfortunately (Wget
maintainership is separate from the maintainers of this list). To
further confuse the issue, [EMAIL PROTECTED] is actually just an alias to
wget@sunsite.dk, which is the one you're actually subscribed to.

To unsubscribe, send an email to [EMAIL PROTECTED]; it will
send you a confirmation email that you'll need to reply to before you'll
actually be unsubscribed.

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

iD8DBQFGjXaB7M8hyUobTrERCPluAJ96Uig9uMkHSeA8G5iRDPT2HDtaEQCffeN/
s+U0CnIY5oHYXWSwa6HXVBg=
=0gDG
-END PGP SIGNATURE-

Re: bug and "patch": blank spaces in filenames causes looping

[Micah Cowan]

-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

[Rich Cook]

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

925-784-3077
--
✐

Re: bug and "patch": blank spaces in filenames causes looping

[Hrvoje Niksic]

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

[Bruso, John]

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]

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

925-784-3077
--
✐

Re: bug and "patch": blank spaces in filenames causes looping

[Hrvoje Niksic]

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

[Hrvoje Niksic]

"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

[Rich Cook]

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

[Virden, Larry W.]

 


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

-- 
http://wiki.tcl.tk/ >
Even if explicitly stated to the contrary, nothing in this posting
should be construed as representing my employer's opinions.
mailto:[EMAIL PROTECTED] > http://www.purl.org/NET/lvirden/
>
 

Re: bug and "patch": blank spaces in filenames causes looping

[Hrvoje Niksic]

"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]

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

[Tony Lewis]

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.