Re: shell32: for FO_COPY, if the destination is an empty string, SHFileOperation should use the current directory.

[Alexandre Julliard]

[e-mail] writes:
> @@ -1179,6 +1181,15 @@ static HRESULT copy_files(FILE_OPERATION *op, const FILE_LIST *flFrom, FILE_LIST
>      const FILE_ENTRY *entryToCopy;
>      const FILE_ENTRY *fileDest = &flTo->feFiles[0];
> - /* If the destination is empty, SHFileOperation should use the current directory */
> - if (!fileDest)
> - {
> -
>    WCHAR currd[MAX_PATH];
> -
>    GetCurrentDirectoryW(MAX_PATH, currd);
> -
>    parse_file_list(flTo, currd);
> -
>    fileDest = &flTo->feFiles[0];
> - }

This can't possibly work. parse_file_list requires a null-terminated
list, and you will be overwriting the previous data. This needs more
thought.

shell32: for FO_COPY, if the destination is an empty string, SHFileOperation should use the current directory.

[Lionel Debroux]

Alexandre Julliard wrote:
> [e-mail] writes:
> > @@ -1179,6 +1181,15 @@ static HRESULT copy_files(FILE_OPERATION *op, const FILE_LIST *flFrom, FILE_LIST
> >      const FILE_ENTRY *entryToCopy;
> >      const FILE_ENTRY *fileDest = &flTo->feFiles[0];
> > - /* If the destination is empty, SHFileOperation should use the current directory */
> > - if (!fileDest)
> > - {
> > -
> >    WCHAR currd[MAX_PATH];
> > -
> >    GetCurrentDirectoryW(MAX_PATH, currd);
> > -
> >    parse_file_list(flTo, currd);
> > -
> >    fileDest = &flTo->feFiles[0];
> > - }
>
> This can't possibly work. parse_file_list requires a null-terminated
> list, and you will be overwriting the previous data.

Well, AFAICS, there is no previous data in flTo, because of the early 
abort upon empty path in parse_file_list():
     if (!szFiles[0])
         return ERROR_ACCESS_DENIED;

right before

flList->feFiles = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
                                 flList->num_alloc*sizeof(FILE_ENTRY));

I checked it by adding a trace to the early exit of destroy_file_list():
     if (!flList || !flList->feFiles)
and calling destroy_file_list() before the call to parse_file_list() in 
copy_files() added by the patch.

I've updated the patch to add more comments and to check whether the 
destination file name is empty, instead of checking flTo->feFiles 
against NULL, though that should be equivalent due to
     ZeroMemory(&flTo, sizeof(FILE_LIST));
in SHFileOperationW.

Is this try2 closer to the correct way of doing things ?

Lionel.

shell32: for FO_COPY, if the destination is an empty string, SHFileOperation should use the current directory.

[Alexandre Julliard]

Lionel Debroux  writes:
> I've updated the patch to add more comments and to check whether the
> destination file name is empty, instead of checking flTo->feFiles
> against NULL, though that should be equivalent due to
>     ZeroMemory(&flTo, sizeof(FILE_LIST));
> in SHFileOperationW.
>
> Is this try2 closer to the correct way of doing things ?

No, you are still not passing a valid list, and doing lots of extra
unneeded work. The first step would be to test what an empty string
means for the other cases, so that you can know where to put the
code. Then probably build a simple file list containing "." or something
like that at the appropriate place.