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