Re: [Bug-wget] Wget 1.11.4 and FTP Server from Windows Server 2008 R2

[Tim Ruehsen]

Am Friday 05 October 2012 schrieb Ray Satiro:
> _______________________________
> 
> > From: Tim Ruehsen <address@hidden>
> >
> >To: address@hidden; Ray Satiro <address@hidden>
> >Sent: Thursday, October 4, 2012 9:53 AM
> >Subject: Re: [Bug-wget] Wget 1.11.4 and FTP Server from Windows Server
> >2008 R2
> >
> >> I couldn't build 6e4c3ab for MinGW without reverting the recent commit
> >> 67e6027 "Add support for file names longer than MAX_FILE" from Tim
> >> Ruehsen. I have no pathconf() here and it looks like gnulib doesn't
> >> offer it for mingw or msvc. I'm not suggesting it should be reverted in
> >> the main repo but there would have to be _PC_NAME_MAX and pathconf()
> >> substitutes for windows.
> >
> >Hi Ray,
> >
> >since I have no Win Env, could you check if something like this works ?
> >
> >#ifdef WINDOWS
> >ret = PATH_MAX;
> >#else
> >ret = pathconf (*p ? p : ".", name);
> >#endif
> >
> >It is in utils.c/get_max_length().
> >
> >I read the following link, but it is not quite clear, how to go on.
> >http://stackoverflow.com/questions/833291/is-there-an-equivalent-to-winapi
> >s- max-path-under-linux-unix
> 
> I think a lot of the confusion with that in windows programming is that
> there's _MAX_PATH, MAX_PATH, PATH_MAX, and you can go past all that anyway
> by using the extended path prefix with the unicode mapped api. And since
> the ansi api is in most cases just a wrapper around the unicode api you
> can in a lot of cases use the extended path prefix in the ansi api as
> well.
> 
> 
> The limitations in wget probably depend on gnulib because a lot is wrapped
> in that. gnulib calls mscrt but also direct api. MS says their CRT
> functions can handle paths > MAX_PATH but I haven't always found that to
> be true, although I can't recall a specific example so it's true enough I
> guess. I never use PATH_MAX but I do use MAX_PATH when I'm not using the
> extended path prefix. PATH_MAX varies in windows but MAX_PATH should
> always be 260 and that includes the terminating null. Look at this
> 
> http://msdn.microsoft.com/en-us/library/aa365247%28VS.85%29.aspx#maxpath
> 
> 
> pathconf() from what I remember was not always reliable/portable but that
> was many years ago so maybe it is these days. How big a path you can use
> depends on what your api accepts so really it's a question of what the
> gnulib functions can handle. Since there's no gnulib pathconf for ms I
> suggest you do what I do and stick with MAX_PATH unless the extended path
> prefix is in use (I don't see it...). Also there are some caveats with
> extended path for example you can't do / (you do that in your code) and
> expect it to be converted to a backslash
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).
> aspx#namespaces regarding your code I can't do unit testing because I don't
> have the time but hopefully someone will.

Thanks for the detailed information.
I must admit that I just fixed up the patch to fit into wget 1.14 source code.
If someone is interested in the history of this patch: 
https://savannah.gnu.org/bugs/?func=detailitem&item_id=21714

For my kids I still have a WinXP partition, but I personally don't use Windows 
since many years. So someone else has to do the patching and testing.


> I did notice this ------------
> size_t max_length;
> [...]
> /* Check that the length of the file name is acceptable. */
> max_length = get_max_length (fnres.base, fnres.tail, _PC_NAME_MAX) -
> CHOMP_BUFFER; if (max_length > 0 && strlen (temp_fnres.base) > max_length)
> {
> [...]
>   /* Shorten the file name. */
>   temp_fnres.base[max_length] = '\0';
> ------------
> 
> you are assigning to an unsigned type so what if you end up with something
> like max_length = 5 - 25;
> or slightly more likely
> max_length = 0 - 25;
> 
> temp_fnres.base[max_length] = '\0';
> 
> here you're modifying the string object's buffer directly but not updating
> its tail. if someone else later comes along and adds below that there
> could be some havoc. also when using that object it should be one or the
> other null terminated or not. so lets say you have
> 
> { 'a', 'b', 'c' } and your tail is 3. you call append_char( '\0', &dest )
> then you have { 'a', 'b', 'c', '\0' } and your tail is 4 but now what. you
> need to append another string and you do append_string( "def", &dest );
> well that's going in starting at &base[tail] so you'll have { 'a', 'b',
> 'c', '\0', 'd', 'e', 'f' } and your tail is 7. so i'd make it so you have
> base[tail] as null but really i think the usage in the comments to use
> append_char with a null character isn't good or if you must maybe it
> should be something like append_char( c, *dest ) { if( !c ) { append_null(
> dest ); return; } [...] } append_null( *dest ) { if( dest->size ==
> dest->tail ) { GROW( dest, 1 ); } dest->base[tail] = 0; } but with this
> lets say you go back and append_string then you're no longer null
> terminated. and further the string object may be intended to hold
> separated strings and i've totally misinterpreted this..

You are perfectly right. In my projects I use -Wsign-compare, letting the 
compiler catch these signed vs. unsigned cases.
But your are overwhelmed by warnings if you try that with historically grown 
projects. Most maintainers are not amused about the tons of patches to fix the 
warnings... (my experience).

But I will take a look at this special case.

Regards, Tim