[PATCH 03 of 12] Win32: non-ASCII directory names support in ngx_getcwd()

[Sergey Kandaurov]

> On 19 Feb 2023, at 21:23, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> Hello!
> 
> On Fri, Feb 17, 2023 at 07:04:04PM +0400, Sergey Kandaurov wrote:
> 
>>> On 13 Jan 2023, at 01:35, Maxim Dounin <mdounin at mdounin.ru> wrote:
>>> 
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin at mdounin.ru>
>>> # Date 1673548916 -10800
>>> #      Thu Jan 12 21:41:56 2023 +0300
>>> # Node ID 7cf820c46860796cff91f53a5d2db669bb5b5a6c
>>> # Parent  d05c0adf5890aecc68ce8906ef19ca07502ed071
>>> Win32: non-ASCII directory names support in ngx_getcwd().
>>> 
>>> This makes it possible to start nginx without a prefix explicitly set
>>> in a directory with non-ASCII characters in it.
>>> 
>>> diff -r d05c0adf5890 -r 7cf820c46860 src/os/win32/ngx_files.c
>>> --- a/src/os/win32/ngx_files.c	Thu Jan 12 21:41:39 2023 +0300
>>> +++ b/src/os/win32/ngx_files.c	Thu Jan 12 21:41:56 2023 +0300
>>> @@ -428,6 +428,30 @@ ngx_realpath(u_char *path, u_char *resol
>>> }
>>> 
>>> 
>>> +size_t
>>> +ngx_getcwd(u_char *buf, size_t size)
>>> +{
>>> +    u_char   *p;
>>> +    size_t    n;
>>> +    u_short   utf16[NGX_MAX_PATH];
>>> +
>>> +    n = GetCurrentDirectoryW(NGX_MAX_PATH, utf16);
>>> +
>>> +    if (n == 0) {
>>> +        return 0;
>>> +    }
> 
> Re-reading the documentation, I tend to think this might worth an 
> additional test for "(n > NGX_MAX_PATH)"
> (https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcurrentdirectory):
> 
> : If the function succeeds, the return value specifies the number 
> : of characters that are written to the buffer, not including the 
> : terminating null character.
> 
> : If the function fails, the return value is zero. To get extended 
> : error information, call GetLastError.
> 
> : If the buffer that is pointed to by lpBuffer is not large 
> : enough, the return value specifies the required size of the 
> : buffer, in characters, including the null-terminating character.
> 
> That is, there will be no explicit error (n == 0) if the buffer 
> will happen to be insufficient.  On the other hand, this probably 
> can happen with long paths enabled (see 
> https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry), 
> so it might be a good idea to add an explicit handling while here.
> 

Yes, I looked through GetCurrentDirectory return values,
but didn't pay an attention on long paths support.
Considering them, it may have sense to add such checks.

>>> +
>>> +    p = ngx_utf16_to_utf8(buf, utf16, &size, NULL);
>>> +
>> 
>> No error check may result in double-free, first freed
>> after (re-)allocation on NGX_EILSEQ, then as below.
> 
> Free on NGX_EILSEQ after reallocation only frees the memory 
> allocated by the ngx_utf16_to_utf8() itself.  The incorrect free() 
> was before reallocation, and is now fixed.
> 
> Since on errors ngx_utf16_to_utf8() returns NULL, it is matched by 
> the "if (p != buf)" test, and processed identically to 
> reallocations.  The only questionable part is ngx_free(NULL), 
> though as per C standard (starting at least with C89; and also 
> win32 documentation) this is guaranteed to be a nop.
> 
>>> +    if (p != buf) {
>>> +        ngx_free(p);
>>> +        return 0;
>> 
>> Why return an error if (re-)allocation happened?
>> Sizes (calculated in 1-byte units) above NGX_MAX_PATH
>> seem perfectly valid with multibyte UTF-8.
> 
> The ngx_getcwd() interface does not provide a way to return a new 
> pointer.  Rather, it is strictly limited to the buffer provided by 
> the caller, and can only return an error if the buffer is not 
> large enough.

Sure, missed that point.

> 
> One possible improvement here is to explicitly set the errno to a 
> reasonable value if this happens.  This would require an explicit 
> handling of other errors though.
> 
> Combined with the above:
> 
> diff -r 6705dd675ace src/os/win32/ngx_files.c
> --- a/src/os/win32/ngx_files.c	Sat Feb 18 16:01:20 2023 +0300
> +++ b/src/os/win32/ngx_files.c	Sun Feb 19 03:22:48 2023 +0300
> @@ -442,10 +442,20 @@ ngx_getcwd(u_char *buf, size_t size)
>         return 0;
>     }
> 
> +    if (n > NGX_MAX_PATH) {
> +        ngx_set_errno(ERROR_INSUFFICIENT_BUFFER);
> +        return 0;
> +    }
> +
>     p = ngx_utf16_to_utf8(buf, utf16, &size, NULL);
> 
> +    if (p == NULL) {
> +        return 0;
> +    }
> +
>     if (p != buf) {
>         ngx_free(p);
> +        ngx_set_errno(ERROR_INSUFFICIENT_BUFFER);
>         return 0;
>     }
> 
> 
> [...]
> 

Thanks for the lengthy explanation.
The proposed addition looks good to me.

-- 
Sergey Kandaurov