winsup\cygwin\path.cc issues

winsup\cygwin\path.cc issues

[Ken Harris]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 3550 bytes --]

Hi:
                While originally investigating a sporadic failure in MSYS2, I believe I found that its origin may actually be within Cygwin.

                Given the following command sequence on cygwin64 in a CMD.EXE command prompt (on Windows 10 x64 if it matters).


  cd C:\Cygwin64\bin
                echo.exe running \”test\” logging to ../../../my.log

                two problems appear:
                #1 –  line 2802 in https://cygwin.com/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/path.cc;h=a132a0a7e0418ba921eefab72fade9930baadff3;hb=HEAD
                pointer ‘p’ will underflow if a backslash is not found in ‘path’.
                It may be that it finds an (unrelated) backslash in someone else’s buffer and fails benignly…but in MSYS2, I’ve see it bump up against unmapped address space a fail very cryptically (as an access violation early in dcrt0.cc initialization before the signal handler is installed).

                #2 – The reason it fails to find its expected backslash appears within path.cc again beginning on line 1381, when a drive-letter is inserted into destination path. Later in normalize_win32_path(), when “../” expressions are applied (i.e. lines 1421 and 1441), these calculations don’t take into account the size of the inserted drive letter (plus colon) and results in overwriting the top-most slash with pathname characters – which then exercises issue #1.

                If echo doesn’t crash, there doesn’t appear to be any obvious evidence of this error. In fact, from debugging a recent version of Cygwin’s “echo.exe”, it looked like the environment block occupying memory at a lower address than ‘path’ contained plenty of backslashes to break line 2802’s loop. In MSYS2, it seems occasionally  (though not always), memory below ‘path’ pointer will _not_ contain a backslash. The result seems to be just C0000005 returned by the echo.exe process.

                I’m not familiar with how to build Cygwin so I haven’t verified this with putting an assert in code. I did debug echo.exe with WinDBG and following how the caller of NtCreateFile handles C0000034 return value (which least to the ‘p’ underflow), watched the disassembly for line 2802:

cygwin1!setpassent+0x22a3:
00000001`800de563 8078ff5c        cmp     byte ptr [rax-1],5Ch ds:00000006`0000ffee=00

                march down lower than the path string:

00000006`0000fff0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ..................
00000006`00010002 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ..................
00000006`00010014 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ..................
00000006`00010026 00 00 13 80 00 00 00 00 00 00 43 3a 43 6d 79 2e 6c 6f  ..........C:Cmy.lo
00000006`00010038 67 00 20 46 69 6c 65 73 5c 4d 41 54 4c 41 42 00 52 32  g. Files\MATLAB.R2
00000006`0001004a 30 31 38 62 00 6c 6e 6b 00 00 00 00 00 00 00 00 00 00  018b.lnk..........
00000006`0001005c 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ..................
00000006`0001006e 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ..................

                Hope you find this of use.

                Thanks,
                -ken

\0ТÒÐÐ¥\a&ö&ÆVÒ\a&W\x06÷'G3¢\x02\x02\x02\x02\x02\x02\x06‡GG\x03¢òö7–wv–âæ6öÒ÷\a&ö&ÆV×2æ‡FÖÀФd\x15\x13¢\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x02\x06‡GG\x03¢òö7–wv–âæ6öÒöf\x17\x12ðФFö7VÖVçF\x17F–öã¢\x02\x02\x02\x02\x02\x02\x02\x02\x06‡GG\x03¢òö7–wv–âæ6öÒöFö72æ‡FÖÀÐ¥Vç7V'67&–&R\x06–æfó¢\x02\x02\x02\x02\x02\x06‡GG\x03¢òö7–wv–âæ6öÒöÖÂò7Vç7V'67&–&R×6–×\x06ÆPРÐ

Re: winsup\cygwin\path.cc issues

[Marco Atzeri]

On 5/2/2018 3:49 PM, Ken Harris wrote:
> Hi:
>                  While originally investigating a sporadic failure in MSYS2, I believe I found that its origin may actually be within Cygwin.
> 
>                  Given the following command sequence on cygwin64 in a CMD.EXE command prompt (on Windows 10 x64 if it matters).
> 
> 
>    cd C:\Cygwin64\bin
>                  echo.exe running \”test\” logging to ../../../my.log
> 


Not clear to me what is the exact command line to replicate

In addition "C:\Cygwin64\bin" is "/bin" so where do you expect
  /bin/../../../my.log to be ?

Regards
Marco

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

RE: winsup\cygwin\path.cc issues

[Ken Harris]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 3290 bytes --]

Thank you so much, Corinna: So far - it looks great. I'll roll your patch into our MSYS2 build which will exercise it about as widely as we possibly can. If I see anything amiss, I'll track it down, and if I can repro in Cygwin alone, I'll report back. 
Again, Thanks!
-Ken

-----Original Message-----
From: Corinna Vinschen [mailto:corinna-cygwin@cygwin.com] 
Sent: Tuesday, May 29, 2018 12:32 PM
To: cygwin@cygwin.com
Cc: Ken Harris <Ken.Harris@mathworks.com>
Subject: Re: winsup\cygwin\path.cc issues

Hi Ken,

On May  4 01:23, Ken Harris wrote:
> Hi Marco: Sorry for not replying to the original exchange we had. I 
> wasn't subscribed to the list but now I am so it won't happen again 
> (so I'm quoting our exchange below).
>                 
>                 I installed and built cygwin1.dll with an added assert
>                 in path.cc to identify when the buffer underrun
>                 condition I originally described occurs:
> 
> $ diff -b
> ./cygwin-2.10.0-1.src/newlib-cygwin/winsup/cygwin/path.cc.ORIG
> ./cygwin-2.10.0-1.src/newlib-cygwin/winsup/cygwin/path.cc 2803c2803 < 
> ; ---
> >                   assert(p >= path);
> 
>                 Thus, a simple:
> 
>                 cat '\A../../../B'
> 
>                 will result in the assert firing:
> 
> kharris@ah-kharris /usr/src $ cat '\A../../../B' assertion "p >= path"
> failed: file "../../.././winsup/cygwin/path.cc", line 2803, function:
> int symlink_info::check(char*, const suffix_info*, fs_info&,
> path_conv_handle&) Aborted (core dumped)
> 
> Attached is a patch (in addition to the added assert) with what I
> *think* might really fix the problem. This was where the expected 
> backslash got squashed which allowed symlink_info::check() to go 
> "negative" with its 'p' pointer and look for a backslash in someone 
> else's memory.

Thanks for your preliminary work, but as far as I can see this isn't the entire solution.  The same problem occurs if your CWD is the root of a drive, e.g., C:\, and you call cat A../../../B.  Even simpler, try `cat 'C:\A../../../B''

The reason is that the code in normalize_win32_path never actually ignores drive prefixes.  There's an implicit (and oh so wrong) assumption that any path starts with a slash or backslash one way or the other.
It's pretty weird that it took so long to find this blatant problem.

I applied a patch which hopefully fixes this problem in all code paths:
https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=35998fc2fa6c

I also left your assertion in the code for now as an additional patch https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=7d00a5e320db
just to be sure, but I will take this out again before a release.

I uploaded new developer snapshots to https://cygwin.com/snapshots/
containing the above patches.  Please give them a try.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat
\x03B‹KCB”\x1c›Ø›\x19[H\x1c™\^[ܝ\x1cΈ\b\b\b\b\b\b\x1a\x1d\x1d\x1c\x0e‹ËØÞYÝÚ[‹˜ÛÛKÜ\x1c›Ø›\x19[\Ëš\x1d^[[\x03B‘TNˆ\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\x1a\x1d\x1d\x1c\x0e‹ËØÞYÝÚ[‹˜ÛÛKÙ˜\KÃB‘^[ØÝ[Y[\x18]\x1a[ÛŽˆ\b\b\b\b\b\b\b\b\x1a\x1d\x1d\x1c\x0e‹ËØÞYÝÚ[‹˜ÛÛKÙ^[ØÜËš\x1d^[[\x03B•[œÝXœØÜšX™H\x1a[™›Îˆ\b\b\b\b\b\x1a\x1d\x1d\x1c\x0e‹ËØÞYÝÚ[‹˜ÛÛKÛ[\vÈÝ[œÝXœØÜšX™K\Ú[\^[\x19CBƒB

Re: winsup\cygwin\path.cc issues

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2546 bytes --]

Hi Ken,

On May  4 01:23, Ken Harris wrote:
> Hi Marco: Sorry for not replying to the original exchange we had. I
> wasn't subscribed to the list but now I am so it won't happen again
> (so I'm quoting our exchange below).
>                 
>                 I installed and built cygwin1.dll with an added assert
>                 in path.cc to identify when the buffer underrun
>                 condition I originally described occurs:
> 
> $ diff -b
> ./cygwin-2.10.0-1.src/newlib-cygwin/winsup/cygwin/path.cc.ORIG
> ./cygwin-2.10.0-1.src/newlib-cygwin/winsup/cygwin/path.cc 2803c2803 <
> ; ---
> >                   assert(p >= path);
> 
>                 Thus, a simple:
> 
>                 cat '\A../../../B'
> 
>                 will result in the assert firing:
> 
> kharris@ah-kharris /usr/src $ cat '\A../../../B' assertion "p >= path"
> failed: file "../../.././winsup/cygwin/path.cc", line 2803, function:
> int symlink_info::check(char*, const suffix_info*, fs_info&,
> path_conv_handle&) Aborted (core dumped)
> 
> Attached is a patch (in addition to the added assert) with what I
> *think* might really fix the problem. This was where the expected
> backslash got squashed which allowed symlink_info::check() to go
> "negative" with its 'p' pointer and look for a backslash in someone
> else's memory.

Thanks for your preliminary work, but as far as I can see this isn't the
entire solution.  The same problem occurs if your CWD is the root of a
drive, e.g., C:\, and you call cat A../../../B.  Even simpler, try `cat
'C:\A../../../B''

The reason is that the code in normalize_win32_path never actually
ignores drive prefixes.  There's an implicit (and oh so wrong) assumption
that any path starts with a slash or backslash one way or the other.
It's pretty weird that it took so long to find this blatant problem.

I applied a patch which hopefully fixes this problem in all code paths:
https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=35998fc2fa6c

I also left your assertion in the code for now as an additional patch
https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=7d00a5e320db
just to be sure, but I will take this out again before a release.

I uploaded new developer snapshots to https://cygwin.com/snapshots/
containing the above patches.  Please give them a try.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: winsup\cygwin\path.cc issues

[Ken Harris]

[-- Attachment #1: Type: text/plain, Size: 2393 bytes --]

Hi Marco:
                Sorry for not replying to the original exchange we had. I wasn't subscribed to the list but now I am so it won't happen again (so I'm quoting our exchange below).
                
                I installed and built cygwin1.dll with an added assert in path.cc to identify when the buffer underrun condition I originally described occurs:

$ diff -b ./cygwin-2.10.0-1.src/newlib-cygwin/winsup/cygwin/path.cc.ORIG ./cygwin-2.10.0-1.src/newlib-cygwin/winsup/cygwin/path.cc
2803c2803
<                   ;
---
>                   assert(p >= path);

                Thus, a simple:

                cat '\A../../../B'

                will result in the assert firing:

kharris@ah-kharris /usr/src
$ cat '\A../../../B'
assertion "p >= path" failed: file "../../.././winsup/cygwin/path.cc", line 2803, function: int symlink_info::check(char*, const suffix_info*, fs_info&, path_conv_handle&)
Aborted (core dumped)

Attached is a patch (in addition to the added assert) with what I *think* might really fix the problem. This was where the expected backslash got squashed which allowed symlink_info::check() to go "negative" with its 'p' pointer and look for a backslash in someone else's memory.

                I've applied this "correction" in our MSYS2 code.  I hope to get some flight-time with it soon (long duration, automated processing)  and if it causes unexpected problems, I'll report back on that. Otherwise, I just hope it might be helpful to anyone who might run into similar puzzling circumstances (the puzzle is when the errant 'p' pointer _doesn't_ find a stray backslash in someone else's memory. It segv-s and _that_ was the nasty part of the puzzle).

                Thanks,
                -Ken


On 5/2/2018 3:49 PM, Ken Harris wrote:

    Hi:
                     While originally investigating a sporadic failure in MSYS2, I believe I found that its origin may actually be within Cygwin.

                     Given the following command sequence on cygwin64 in a CMD.EXE command prompt (on Windows 10 x64 if it matters).


       cd C:\Cygwin64\bin
                     echo.exe running \"test\" logging to ../../../my.log



Not clear to me what is the exact command line to replicate

In addition "C:\Cygwin64\bin" is "/bin" so where do you expect
/bin/../../../my.log to be ?

Regards
Marco

[-- Attachment #2: path.cc.patch --]
[-- Type: application/octet-stream, Size: 1745 bytes --]

--- ./cygwin-2.10.0-1.src/newlib-cygwin/winsup/cygwin/path.cc.ORIG	2018-05-03 19:43:00.482472100 -0400
+++ ./cygwin-2.10.0-1.src/newlib-cygwin/winsup/cygwin/path.cc	2018-05-03 20:09:44.035630700 -0400
@@ -1342,6 +1342,7 @@
 int
 normalize_win32_path (const char *src, char *dst, char *&tail)
 {
+  int drvprefixlen = 0;
   const char *src_start = src;
   bool beg_src_slash = isdirsep (src[0]);
 
@@ -1385,9 +1386,10 @@
 	*tail++ = cyg_toupper (*src++);
       else if (*src != '/')
 	{
-	  if (beg_src_slash)
-	    tail += cygheap->cwd.get_drive (dst);
-	  else if (!cygheap->cwd.get (dst, 0))
+	  if (beg_src_slash) {
+	    drvprefixlen = cygheap->cwd.get_drive (dst);
+	    tail += drvprefixlen;
+	  } else if (!cygheap->cwd.get (dst, 0))
 	    return get_errno ();
 	  else
 	    {
@@ -1423,10 +1425,10 @@
 	  else
 	    {
 	      /* Back up over /, but not if it's the first one.  */
-	      if (tail > dst + 1)
+	      if (tail > dst + 1 + drvprefixlen)
 		tail--;
 	      /* Now back up to the next /.  */
-	      while (tail > dst + 1 && tail[-1] != '\\' && tail[-2] != ':')
+	      while (tail > dst + 1 + drvprefixlen && tail[-1] != '\\' && tail[-2] != ':')
 		tail--;
 	      src += 2;
 	      /* Skip /'s to the next path component. */
@@ -1446,7 +1448,7 @@
       if ((tail - dst) >= NT_MAX_PATH)
 	return ENAMETOOLONG;
     }
-  if (tail > dst + 1 && tail[-1] == '.' && tail[-2] == '\\')
+  if (tail > dst + 1 + drvprefixlen && tail[-1] == '.' && tail[-2] == '\\')
     tail--;
   *tail = '\0';
   debug_printf ("%s = normalize_win32_path (%s)", dst, src_start);
@@ -2800,7 +2802,7 @@
 	      if (*p != '.' && *p != ' ')
 		{
 		  while (*--p != '\\')
-		    ;
+		    assert(p >= path);
 		  if (*++p != ' ')
 		    p = NULL;
 		}

[-- Attachment #3: Type: text/plain, Size: 219 bytes --]


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple