Re: [2/2] kernel32: Fix GetVolumeNameForVolumeMountPointW to match Mountmgr

[Vitaliy Margolen]

Few nitpicks about your patch.

Guy Albertelli wrote:
> - if (!(input = HeapAlloc( GetProcessHeap(), 0, i_size )))
> - {
> -
>    SetLastError( ERROR_NOT_ENOUGH_MEMORY );
> -
>    goto err_ret;
> - }
> - memset( input, 0, i_size );

To zero out allocated memory you should use HEAP_ZERO_MEMORY allocation flag:
input = HeapAlloc( GetProcessHeap(), HEAP_ZERO_MEMORY, i_size );

However there is no need to do any of that - do not just clear memory
without a good reason. It takes time to do it, especially that you doing it
several times.

What you do need to do in your code is explicitly zero-terminate all strings
you receive from mount manager after you copy them.
> - p = (WCHAR *) input;

This is wrong - types of "p" and "input" has nothing to do with each other.
> - p = (WCHAR*)((char *)input + sizeof(MOUNTMGR_MOUNT_POINT));
> - input->DeviceNameOffset = (char *)p - (char *)input;
> - memcpy( p, nonpersist_name, lstrlenW( nonpersist_name )*sizeof(WCHAR) );
> - input->DeviceNameLength = lstrlenW( nonpersist_name )*sizeof(WCHAR);

This is ugly. You can write it like this:

input->DeviceNameOffset = sizeof(*input);
input->DeviceNameLength = lstrlenW( nonpersist_name ) * sizeof(WCHAR)
memcpy( input + 1, nonpersist_name, input->DeviceNameLength );
> -
>        /* is there space in the return variable ?? */
> -
>        if ((o1->SymbolicLinkNameLength/sizeof(WCHAR))+2 > size)
> -
>        {
> -
>            SetLastError( ERROR_INVALID_PARAMETER );
> -
>            goto err_ret;
> -
>        }

This doesn't look right. There are several other error codes that more
appropriate. You should expand your tests to check what native returns here.
Also are you sure that "size" is in chars not bytes?
> -
>        /* make string null terminated  */
> -
>        p = (WCHAR*)((char*)volume + o1->SymbolicLinkNameLength);
> -
>        memset( p, 0, sizeof(WCHAR) );

Again ugly. You can write it like:
volume[o1->SymbolicLinkNameLength / sizeof(WCHAR)] = 0;
> - if (input) HeapFree( GetProcessHeap(), 0, input );
> - if (output) HeapFree( GetProcessHeap(), 0, output );

You do not need to check for pointer != NULL before freeing it. All free()
functions already doing it.

Vitaliy.

[2/2] kernel32: Fix GetVolumeNameForVolumeMountPointW to match Mountmgr

[Guy Albertelli]

On Sat, 2009-05-09 at 09:52 -0600, Vitaliy Margolen wrote:
> Few nitpicks about your patch.
>
> Guy Albertelli wrote:
> > -
> >        /* is there space in the return variable ?? */
> > -
> >        if ((o1->SymbolicLinkNameLength/sizeof(WCHAR))+2 > size)
> > -
> >        {
> > -
> >            SetLastError( ERROR_INVALID_PARAMETER );
> > -
> >            goto err_ret;
> > -
> >        }
>
> This doesn't look right. There are several other error codes that more
> appropriate. You should expand your tests to check what native returns here.
> Also are you sure that "size" is in chars not bytes?

You are right, should be ERROR_FILENAME_EXCED_RANGE - at least on XP.

Will cleanup rest and resubmit with additional tests

Thanks Guy