cygwin_create_path causes null pointer crashes

cygwin_create_path causes null pointer crashes

[Thomas Wolff]

cygwin_create_path is supposed to call malloc itself,
however, it occasionally returns with null and errno == ENOSPC,
if it's provided with a network path, e.g.
cygwin_create_path(CCP_POSIX_TO_WIN_W, "/cygdrive/s/.config/mintty")
returns null in ~ 3 of 1000 cases, where /cygdrive/s exists but 
/cygdrive/s/.config does not
------
Thomas

Re: cygwin_create_path causes null pointer crashes

[Corinna Vinschen]

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

Hi Thomas,

On Dec 10 03:03, Thomas Wolff wrote:
> cygwin_create_path is supposed to call malloc itself,
> however, it occasionally returns with null and errno == ENOSPC,
> if it's provided with a network path, e.g.
> cygwin_create_path(CCP_POSIX_TO_WIN_W, "/cygdrive/s/.config/mintty")
> returns null in ~ 3 of 1000 cases, where /cygdrive/s exists but
> /cygdrive/s/.config does not

Did you try to debug and fix the issue?  This is the developer's mailing
list after all...


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: 819 bytes --]

Re: cygwin_create_path causes null pointer crashes

[Thomas Wolff]

Hi Corinna,

Am 11.12.2016 um 12:08 schrieb Corinna Vinschen:
> Hi Thomas,
>
> On Dec 10 03:03, Thomas Wolff wrote:
>> cygwin_create_path is supposed to call malloc itself,
>> however, it occasionally returns with null and errno == ENOSPC,
>> if it's provided with a network path, e.g.
>> cygwin_create_path(CCP_POSIX_TO_WIN_W, "/cygdrive/s/.config/mintty")
>> returns null in ~ 3 of 1000 cases, where /cygdrive/s exists but
>> /cygdrive/s/.config does not
> Did you try to debug and fix the issue?  This is the developer's mailing
> list after all...
I can contribute some initial analysis:
Cloning cygwin_create_path from cygwin/path.cc with these essentials:
   ssize_t size = cygwin_conv_path (what, from, NULL, 0);
   // checking size, calling malloc
   if (cygwin_conv_path (what, from, to, size) == -1)
   ...
there are actually two different errors happening, with example path 
"/cygdrive/s/.config/mintty":
1. the first call (for size determination) returns 44 but the second 
call returns the correct result "S:\.config\mintty".
2. the first call returns 36 but the second call (provided with a larger 
size value patched into the function) returns "S:\.config\mintty.lnk" 
(which needs 44 bytes); this is obviously the call that would return 
null if not patched with a faked buffer size

Another observation, by the slowness of the calls, is that apparently 
the drive is actually accessed during the path conversion, which I 
wouldn't have expected from a plain path conversion function.

The underlying function cygwin_conv_path looks a bit complicated for a 
straight-forward analysis to me...
------
Thomas

Re: cygwin_create_path causes null pointer crashes

[Corinna Vinschen]

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

On Dec 12 20:28, Thomas Wolff wrote:
> Hi Corinna,
> 
> Am 11.12.2016 um 12:08 schrieb Corinna Vinschen:
> > Hi Thomas,
> > 
> > On Dec 10 03:03, Thomas Wolff wrote:
> > > cygwin_create_path is supposed to call malloc itself,
> > > however, it occasionally returns with null and errno == ENOSPC,
> > > if it's provided with a network path, e.g.
> > > cygwin_create_path(CCP_POSIX_TO_WIN_W, "/cygdrive/s/.config/mintty")
> > > returns null in ~ 3 of 1000 cases, where /cygdrive/s exists but
> > > /cygdrive/s/.config does not
> > Did you try to debug and fix the issue?  This is the developer's mailing
> > list after all...
> I can contribute some initial analysis:
> Cloning cygwin_create_path from cygwin/path.cc with these essentials:
>   ssize_t size = cygwin_conv_path (what, from, NULL, 0);
>   // checking size, calling malloc
>   if (cygwin_conv_path (what, from, to, size) == -1)

Cloning?  Why didn't you just debug with GDB and/or strace?

> there are actually two different errors happening, with example path
> "/cygdrive/s/.config/mintty":
> 1. the first call (for size determination) returns 44 but the second call
> returns the correct result "S:\.config\mintty".
> 2. the first call returns 36 but the second call (provided with a larger
> size value patched into the function) returns "S:\.config\mintty.lnk" (which
> needs 44 bytes); this is obviously the call that would return null if not
> patched with a faked buffer size

A simple strace shows that there's a really ugly problem with network
shares:

Consider that the Windows functions have two(*) error codes for
"file not found".

- STATUS_OBJECT_NAME_NOT_FOUND indicates that the path to the file is
  valid, but the file doesn't exist.

- STATUS_OBJECT_PATH_NOT_FOUND indicates that the path to the file
  doesn't exist.

I.e., if the .config dir exists, but mintty doesn't, NtOpenFile returns
STATUS_OBJECT_NAME_NOT_FOUND.  If the config doe doesn't exist,
NtOpenFile returns STATUS_OBJECT_PATH_NOT_FOUND.

The problem is that in case of network drives, the return code changes.

The first call to NtOpenFile ("S:\.config\mintty") returns
STATUS_OBJECT_PATH_NOT_FOUND.  The second and all subsequent calls
return STATUS_OBJECT_NAME_NOT_FOUND.

Apparently this is a caching issue.  If you wait a bit and repeat the
call, it returns STATUS_OBJECT_PATH_NOT_FOUND again.  This does not
occur with local paths.

This is *so* wrong.  And the downside is that Cygwin relies on the
correct return code in the path conversion code.  Oh well.

Anyway, we can't fix Windows, but we can workaround its problems, which
is what Cygwin does, so I applied a patch.

> Another observation, by the slowness of the calls, is that apparently the
> drive is actually accessed during the path conversion, which I wouldn't have
> expected from a plain path conversion function.

It has to since the existence check on files has to check for
.lnk and .exe suffixes and it has to return an error code, e.g.
ENOENT, ENOTDIR, EACCES, ...

> The underlying function cygwin_conv_path looks a bit complicated for a
> straight-forward analysis to me...

It only consists of a simple switch statement for the four conversions
POSIX <-> Windows (char/wchar_t).  All the dirty work is done by
path_conv::check.

I just generated a new developer snapshot and I'm going to create
a 2.6.1-0.2 test release.  Please test.


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: 819 bytes --]

Re: cygwin_create_path causes null pointer crashes

[Thomas Wolff]

Am 13.12.2016 um 12:56 schrieb Corinna Vinschen:
> On Dec 12 20:28, Thomas Wolff wrote:
>> Hi Corinna,
>>
>> Am 11.12.2016 um 12:08 schrieb Corinna Vinschen:
>>> Hi Thomas,
>>>
>>> On Dec 10 03:03, Thomas Wolff wrote:
>>>> cygwin_create_path is supposed to call malloc itself,
>>>> however, it occasionally returns with null and errno == ENOSPC,
>>>> if it's provided with a network path, e.g.
>>>> cygwin_create_path(CCP_POSIX_TO_WIN_W, "/cygdrive/s/.config/mintty")
>>>> returns null in ~ 3 of 1000 cases, where /cygdrive/s exists but
>>>> /cygdrive/s/.config does not
>>> Did you try to debug and fix the issue?  This is the developer's mailing
>>> list after all...
>> I can contribute some initial analysis:
>> Cloning cygwin_create_path from cygwin/path.cc with these essentials:
>>    ssize_t size = cygwin_conv_path (what, from, NULL, 0);
>>    // checking size, calling malloc
>>    if (cygwin_conv_path (what, from, to, size) == -1)
> Cloning?  Why didn't you just debug with GDB and/or strace?
Well, one reason is that I had to manipulate the value of size between 
the two calls ...

>> ...
> A simple strace shows that there's a really ugly problem with network shares:
>
> ...
> Anyway, we can't fix Windows, but we can workaround its problems, which
> is what Cygwin does, so I applied a patch.
Great, this works, thanks.

Thomas