> On 19 Feb 2023, at 21:23, Maxim Dounin <mdounin@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@mdounin.ru> wrote:
>>>
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin@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
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel