[PATCH 2/2] ws2_32: Fix implementation of InetNtopW.

[Bruno Jesus]

On Fri, Jul 22, 2016 at 9:04 AM, Philipp Hoppermann <plata at mailbox.org> wrote:
> Replaced call to inet_ntop by more elaborate solution.

Looks much better, thanks. See the comments below.

> Signed-off-by: Philipp Hoppermann <plata at mailbox.org>
> ---
>  dlls/ws2_32/socket.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 68 insertions(+), 4 deletions(-)
>
> diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
> index 0622060..2045163 100644
> --- a/dlls/ws2_32/socket.c
> +++ b/dlls/ws2_32/socket.c
> @@ -7934,11 +7934,75 @@ INT WINAPI InetPtonW(INT family, PCWSTR addr, PVOID buffer)
>  }
>
>  /***********************************************************************
> -*              InetNtopW                      (WS2_32.@)
> -*/
> -PCWSTR WINAPI InetNtopW(INT  family, PVOID addr, PWSTR stringBuf, SIZE_T stringBufSize)
> + *              InetNtopW                      (WS2_32.@)
> + *
> + * Get string representation of IP address.
> + *
> + * PARAMS
> + *   family     [I] AF_INET or AF_INET6
> + *   addr       [I] IP address in byte
> + *   buffer     [O] IP address as string
> + *   buffer_len [O] length of the buffer in characters
> + *
> + * RETURNS
> + *   Success: IP address as string
> + *   Failure: NULL. Use GetLastError() to find the error cause.
> + *
> + *   Error codes:
> + *     WSAEAFNOSUPPORT
> + *          - family is not AF_INET or AF_INET6
> + *     ERROR_INVALID_PARAMETER
> + *          - buffer is NULL
> + *          - buffer_len is 0
> + *          - buffer_len is too small
> + *            (i.e. < 16 for AF_INET or < 46 for AF_INET6)
> + *
> + */

Usually only overly complex functions need comments, which is not the
case here so you can drop them. As you can see in the file most almost
no other function has comments like this.

> +PCWSTR WINAPI InetNtopW(INT  family, PVOID addr, PWSTR buffer, SIZE_T buffer_len)
>  {
> -    return inet_ntop(family, addr, stringBuf, stringBufSize);
> +    char *bufferA;
> +
> +    TRACE("family %d, addr %s, buffer (%p), buffer_len %lu\n", family, debugstr_w(addr), buffer, (unsigned long) buffer_len);

WS_inet_ntop traces buffer_len without the cast using %ld, I guess it
is better to follow its example and avoid the cast.

> +    // check family

// comments are not allowed, please use /* */. But most of them are
obvious like "check if buffer is NULL" so they can be dropped.

> +    if ((family != AF_INET) && (family != AF_INET6))
> +    {
> +        SetLastError(WSAEAFNOSUPPORT);
> +        return NULL;
> +    }
> +
> +    // check if buffer is NULL
> +    if (buffer == NULL)
> +    {
> +        SetLastError(ERROR_INVALID_PARAMETER);
> +        return NULL;
> +    }
> +
> +    // check buffer_len
> +    if ((family == AF_INET && buffer_len < 16) || (family == AF_INET6 && buffer_len < 46))
> +    {
> +        SetLastError(ERROR_INVALID_PARAMETER);
> +        return NULL;
> +    }

WS_inet_ntop already does some error checking, there is no need to
manually test again, also we trust the system inet_ntop call to deal
with buffer_len check.

> +    // allocate memory for the string representation in char
> +    if (!(bufferA = HeapAlloc(GetProcessHeap(), 0, buffer_len*sizeof(WCHAR))))
> +    {
> +        SetLastError(WSA_NOT_ENOUGH_MEMORY);
> +        return NULL;
> +    }

You are doubling the required amount of memory, buffer_len should be
already in characters, so no need to multiply.

> +    if (WS_inet_ntop(family, addr, bufferA, buffer_len*sizeof(WCHAR)) != NULL)

Again no need to multiply.

> +    {
> +        // convert to wchar
> +        MultiByteToWideChar(CP_ACP, 0, bufferA, buffer_len*sizeof(WCHAR), buffer, buffer_len);

Use: MultiByteToWideChar(CP_ACP, 0, bufferA, -1, buffer, buffer_len);

> +
> +        HeapFree(GetProcessHeap(), 0, bufferA);
> +        return buffer;
> +    } else
> +    {
> +        return NULL;

You are leaking bufferA in the else case, use a variable to store the
results of WS_inet_ntop and return it instead so you can always
HeapFree before the return and then there is no need for else.

Best wishes,
Bruno