Sergey Kandaurov
February 22, 2023 11:02AM
> 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
Subject Author Views Posted

[PATCH 00 of 12] win32 non-ASCII names support fixes

Maxim Dounin 984 January 12, 2023 04:40PM

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

Maxim Dounin 235 January 12, 2023 04:40PM

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

Sergey Kandaurov 170 February 17, 2023 10:04AM

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

Maxim Dounin 223 February 19, 2023 12:24PM

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

Sergey Kandaurov 159 February 22, 2023 11:02AM

[PATCH 01 of 12] Win32: non-ASCII names support in autoindex (ticket #458)

Maxim Dounin 197 January 12, 2023 04:40PM

Re: [PATCH 01 of 12] Win32: non-ASCII names support in autoindex (ticket #458)

Sergey Kandaurov 167 February 17, 2023 09:40AM

Re: [PATCH 01 of 12] Win32: non-ASCII names support in autoindex (ticket #458)

Maxim Dounin 159 February 19, 2023 12:18PM

Re: [PATCH 01 of 12] Win32: non-ASCII names support in autoindex (ticket #458)

Sergey Kandaurov 162 February 22, 2023 10:40AM

[PATCH 02 of 12] Win32: non-ASCII names support in "include" with wildcards

Maxim Dounin 214 January 12, 2023 04:40PM

Re: [PATCH 02 of 12] Win32: non-ASCII names support in "include" with wildcards

Sergey Kandaurov 186 February 17, 2023 09:54AM

Re: [PATCH 02 of 12] Win32: non-ASCII names support in "include" with wildcards

Maxim Dounin 196 February 19, 2023 12:20PM

Re: [PATCH 02 of 12] Win32: non-ASCII names support in "include" with wildcards

Sergey Kandaurov 171 February 22, 2023 10:50AM

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

Maxim Dounin 193 January 12, 2023 04:40PM

Re: [PATCH 04 of 12] Win32: non-ASCII directory names support in ngx_create_dir()

Sergey Kandaurov 189 February 17, 2023 10:14AM

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

Maxim Dounin 172 January 12, 2023 04:40PM

Re: [PATCH 05 of 12] Win32: non-ASCII directory names support in ngx_delete_dir()

Sergey Kandaurov 173 February 17, 2023 10:14AM

[PATCH 06 of 12] Win32: reworked ngx_win32_rename_file() to check errors

Maxim Dounin 203 January 12, 2023 04:40PM

[PATCH 07 of 12] Win32: reworked ngx_win32_rename_file() to use nginx wrappers

Maxim Dounin 177 January 12, 2023 04:40PM

[PATCH 09 of 12] Win32: non-ASCII names support in ngx_rename_file()

Maxim Dounin 229 January 12, 2023 04:40PM

[PATCH 10 of 12] Win32: non-ASCII names support in ngx_open_tempfile()

Maxim Dounin 188 January 12, 2023 04:40PM

[PATCH 08 of 12] Win32: non-ASCII names support in ngx_delete_file()

Maxim Dounin 175 January 12, 2023 04:40PM

[PATCH 12 of 12] Win32: non-ASCII names in ngx_fs_bsize(), ngx_fs_available()

Maxim Dounin 209 January 12, 2023 04:40PM

[PATCH 11 of 12] Win32: fixed ngx_fs_bsize() for symlinks

Maxim Dounin 225 January 12, 2023 04:40PM

Re: [PATCH 11 of 12] Win32: fixed ngx_fs_bsize() for symlinks

Sergey Kandaurov 169 February 17, 2023 10:18AM

Re: [PATCH 11 of 12] Win32: fixed ngx_fs_bsize() for symlinks

Maxim Dounin 174 February 19, 2023 12:24PM

Re: [PATCH 11 of 12] Win32: fixed ngx_fs_bsize() for symlinks

Sergey Kandaurov 169 February 22, 2023 11:02AM

Re: [PATCH 11 of 12] Win32: fixed ngx_fs_bsize() for symlinks

Maxim Dounin 174 February 23, 2023 01:48PM

Re: [PATCH 11 of 12] Win32: fixed ngx_fs_bsize() for symlinks

Sergey Kandaurov 172 February 24, 2023 05:42AM

Re: [PATCH 11 of 12] Win32: fixed ngx_fs_bsize() for symlinks

Sergey Kandaurov 175 March 21, 2023 07:26AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 131
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 500 on July 15, 2024
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready