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.
> > +
> > + 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.
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;
}
[...]
--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
