Re: [PATCH RFC] fork: reduce chances for "address space is already occupied" errors
Hi Corinna,
On 3/26/19 7:28 PM, Corinna Vinschen wrote:
> On Mar 26 19:25, Corinna Vinschen wrote:
>> Hi Michael,
>>
>>
>> Redirected to cygwin-patches...
>>
>>
>> On Mar 26 18:10, Michael Haubenwallner wrote:
>>> Hi Corinna,
>>>
>>> as I do still encounter fork errors (address space needed by is
>>> already occupied) with dynamically loaded dlls (but unrelated to
>>> replaced dlls), one of them repeating even upon multiple retries,
>>
>> Why didn't rebase fix that?
As far as I understand, rebasing is about touching already installed
dlls as well, which would require to restart all Cygwin processes.
As the problem is about some dll built during a larger build job,
this is not something that feels useful to me.
>
> Btw., is that 32 or 64 bit? Both?
I'm on 64bit only, can't say for 32bit. And while in theory possible,
I'm not after supporting 32bit Cygwin in Gento Prefix at all...
>>> I'm
>>> coming up with attached patch.
>>>
>>> What do you think about it?
>>
>> I'm not opposed to this patch but I don't quite follow the description.
>> threadinterface->Init only creates three event objects. From what I can
>> tell, Events are stored in Paged and Nonpaged Pools, so they don't
>> affect the processes VM. What am I missing?
Honestly, I'm not completely sure whether this patch really does help:
Beyond the Events, there also is CreateNamedPipe and CreateFile used
in fhandler_pipe::create via sigproc_init, and these causing the address
conflicts with some dll actually is nothing more than a wild guess:
While their returned handles are below the conflicting dll address,
who can tell what these API calls do allocate internally?
Thanks!
/haubi/
>>
>>> >From dfc28bcbb7ed55fe33ddb8d15e761b4d5b4815f8 Mon Sep 17 00:00:00 2001
>>> From: Michael Haubenwallner
>>> Date: Tue, 26 Mar 2019 17:38:36 +0100
>>> Subject: [PATCH] Cygwin: fork: reserve dynloaded dll areas earlier
>>>
>>> In dll_crt0_0, both threadinterface->Init and sigproc_init allocate
>>> windows object handles using unpredictable memory regions, which may
>>> collide with dynamically loaded dlls when they were relocated.
>>> ---
>>> winsup/cygwin/dcrt0.cc | 6 ++
>>> winsup/cygwin/fork.cc | 6 --
>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
>>> index 11edcdf0d..fb726a739 100644
>>> --- a/winsup/cygwin/dcrt0.cc
>>> +++ b/winsup/cygwin/dcrt0.cc
>>> @@ -632,6 +632,12 @@ child_info_fork::handle_fork ()
>>>
>>>if (fixup_mmaps_after_fork (parent))
>>> api_fatal ("recreate_mmaps_after_fork_failed");
>>> +
>>> + /* We need to occupy the address space for dynamically loaded dlls
>>> + before we allocate any dynamic object, or we may end up with
>>> + error "address space needed by is already occupied"
>>> + for no good reason (seen with some relocated dll). */
>>> + dlls.reserve_space ();
>>> }
>>>
>>> bool
>>> diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
>>> index 74ee9acf4..7e1c08990 100644
>>> --- a/winsup/cygwin/fork.cc
>>> +++ b/winsup/cygwin/fork.cc
>>> @@ -136,12 +136,6 @@ frok::child (volatile char * volatile here)
>>> {
>>>HANDLE& hParent = ch.parent;
>>>
>>> - /* NOTE: Logically this belongs in dll_list::load_after_fork, but by
>>> - doing it here, before the first sync_with_parent, we can exploit
>>> - the existing retry mechanism in hopes of getting a more favorable
>>> - address space layout next time. */
>>> - dlls.reserve_space ();
>>> -
>>>sync_with_parent ("after longjmp", true);
>>>debug_printf ("child is running. pid %d, ppid %d, stack here %p",
>>> myself->pid, myself->ppid, __builtin_frame_address (0));
>>> --
>>> 2.17.0
>>>
>>>
>>
>>>
>>> --
>>> 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
>>
>>
>> --
>> Corinna Vinschen
>> Cygwin Maintainer
>
>
>
Re: [PATCH RFC] fork: reduce chances for "address space is already occupied" errors
Hi Michael, On Mar 27 09:26, Michael Haubenwallner wrote: > Hi Corinna, > > On 3/26/19 7:28 PM, Corinna Vinschen wrote: > > On Mar 26 19:25, Corinna Vinschen wrote: > >> Hi Michael, > >> > >> > >> Redirected to cygwin-patches... > >> > >> > >> On Mar 26 18:10, Michael Haubenwallner wrote: > >>> Hi Corinna, > >>> > >>> as I do still encounter fork errors (address space needed by is > >>> already occupied) with dynamically loaded dlls (but unrelated to > >>> replaced dlls), one of them repeating even upon multiple retries, > >> > >> Why didn't rebase fix that? > > As far as I understand, rebasing is about touching already installed > dlls as well, which would require to restart all Cygwin processes. > As the problem is about some dll built during a larger build job, > this is not something that feels useful to me. Wait, let me understand what's going on. IIUC you're building DLLs which are then used during the build job itself, right? > > Btw., is that 32 or 64 bit? Both? > > I'm on 64bit only, can't say for 32bit. And while in theory possible, > I'm not after supporting 32bit Cygwin in Gento Prefix at all... If so, then I'm really curious how many DLLs are affected and why this occurs on 64 bit. As you know, 64 bit has a defined memory layout. Binutils ld is supposed to base the DLLs to a pseudo-random address in the area between 0x4: and 0x6:. This area is occupied by un-rebased DLLs only. 8 Gigs is a *lot* of space for DLLs. That also means that the DLLs should not at all collide with windows objects (typically reserved in the lesser 2 Gigs area), unless they collide with themselves. At least that's the idea. Can you check what addresses the freshly built DLLs are based on by LD? Is there a chance that the algorithm used in LD is too dumb? Or, hmm. Is there a chance that newer Windows loads dynamically loaded DLLs whereever it likes, ignoring the base address, ASLR-like, even if the DLL is marked as non-ASLR-aware? But then again, we should have a lot more complaints on the list... > >>> I'm > >>> coming up with attached patch. > >>> > >>> What do you think about it? > >> > >> I'm not opposed to this patch but I don't quite follow the description. > >> threadinterface->Init only creates three event objects. From what I can > >> tell, Events are stored in Paged and Nonpaged Pools, so they don't > >> affect the processes VM. What am I missing? > > Honestly, I'm not completely sure whether this patch really does help: > Beyond the Events, there also is CreateNamedPipe and CreateFile used > in fhandler_pipe::create via sigproc_init, and these causing the address > conflicts with some dll actually is nothing more than a wild guess: > While their returned handles are below the conflicting dll address, > who can tell what these API calls do allocate internally? The handles are not addresses. If the sigproc_init stuff collides, I only see two chances for that, the process-local read/write buffers of the signal pipe, and the stack of the read_sig thread. If this patch helps your situation, we can pull it in and test it, but I think your situation asks for more debugging along the lines of the DLL rebasing above. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH fifo 0/2] Add support for duplex FIFOs
On Mar 26 20:01, Corinna Vinschen wrote: > On Mar 26 17:24, Ken Brown wrote: > > Hi Corinna, > > > > On 3/26/2019 4:36 AM, Corinna Vinschen wrote: > > > Hi Ken, > > > > > > On Mar 25 23:06, Ken Brown wrote: > > >> The second patch in this series enables opening a FIFO with O_RDWR > > >> access. The underlying Windows named pipe is creted with duplex > > >> access, and its handle is made the I/O handle of the first client. > > >> > > >> While testing this, I had some mysterious crashes, which are fixed by > > >> the first patch. > > > > > > I rebased the topic/fifo branch on top of master and force-pushed with > > > your patches. Make sure to reset your working tree to origin/topic/fifo > > > and add any further patches on top. > > > > I'm comfortable now with merging topic/fifo into master. I've tested the > > new > > select and fork code [*], and they seem to work as expected. That was the > > last > > thing holding me up. > > > > As soon as the merge is done, ... > > Will do tomorrow. > > > ..., I'll send a patch with release notes. Done. I also pushed out new dev snapshots. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
[PATCH 1/1] Cygwin: fix: seteuid32() must return EPERM if privileges are not held.
Starting w/ the intro of S4U, seteuid32() calls lsaprivkeyauth(), then
s4uauth(). s4uauth calls LsaRegisterLogonProcess().
LsaRegisterLogonProcess fails w/ STATUS_PORT_CONNECTION_REFUSED, if the
proper privileges are not held.
Because of RtlNtStatusToDosError(), this status would be mapped to
ERROR_ACCESS_DENIED, which in turn would map to EACCES. Therefore it is
useless to add this status to errmap[] (errno.cc), as s4auauth() should
return EPERM as errno here (i.e. if process is not privileged).
Hence the kludge.
Before the intro of S4U, seteuid32() called lsaprivkeyauth(), then
lsaauth(), then create_token(). Before the intro of Vista, the latter
would have called NtCreateToken().
NtCreateToken() would have failed w/ STATUS_PRIVILEGE_NOT_HELD for a
process w/o the proper privileges. In that case, calling seteuid32()
would have returned EPERM (as required).
Since the intro of Vista, and if the process had been started from an
UNelevated shell, create_token() does NOT reach NtCreateToken()!
As create_token() failed to properly set errno in that case, calling
seteuid32() would return errno as set by lsaauth(), i.e. EACCES, not
in agreement w/ Posix (a bug which was present for years).
(lsaauth() called LsaRegisterLogonProcess() which would fail)
---
winsup/cygwin/sec_auth.cc | 5 +
1 file changed, 5 insertions(+)
diff --git a/winsup/cygwin/sec_auth.cc b/winsup/cygwin/sec_auth.cc
index a76f453..7a1aa8e 100644
--- a/winsup/cygwin/sec_auth.cc
+++ b/winsup/cygwin/sec_auth.cc
@@ -1539,6 +1539,11 @@ s4uauth (bool logon, PCWSTR domain, PCWSTR user,
NTSTATUS &ret_status)
{
debug_printf ("%s: %y", logon ? "LsaRegisterLogonProcess"
: "LsaConnectUntrusted", status);
+ // kludge! (if the privilege is not held, return the proper errno)
+ if (status == STATUS_PORT_CONNECTION_REFUSED) // 0xC041 => EACCES
+{
+ status = STATUS_PRIVILEGE_NOT_HELD; // 0xC061 => EPERM
+}
__seterrno_from_nt_status (status);
goto out;
}
--
2.7.5
Re: [PATCH 1/1] Cygwin: fix: seteuid32() must return EPERM if privileges are not held.
On Mar 27 17:01, J.H. van de Water wrote: > Starting w/ the intro of S4U, seteuid32() calls lsaprivkeyauth(), then > s4uauth(). s4uauth calls LsaRegisterLogonProcess(). > LsaRegisterLogonProcess fails w/ STATUS_PORT_CONNECTION_REFUSED, if the > proper privileges are not held. > Because of RtlNtStatusToDosError(), this status would be mapped to > ERROR_ACCESS_DENIED, which in turn would map to EACCES. Therefore it is > useless to add this status to errmap[] (errno.cc), as s4auauth() should > return EPERM as errno here (i.e. if process is not privileged). > > Hence the kludge. > > Before the intro of S4U, seteuid32() called lsaprivkeyauth(), then > lsaauth(), then create_token(). Before the intro of Vista, the latter > would have called NtCreateToken(). > NtCreateToken() would have failed w/ STATUS_PRIVILEGE_NOT_HELD for a > process w/o the proper privileges. In that case, calling seteuid32() > would have returned EPERM (as required). > > Since the intro of Vista, and if the process had been started from an > UNelevated shell, create_token() does NOT reach NtCreateToken()! > As create_token() failed to properly set errno in that case, calling > seteuid32() would return errno as set by lsaauth(), i.e. EACCES, not > in agreement w/ Posix (a bug which was present for years). > (lsaauth() called LsaRegisterLogonProcess() which would fail) > --- Pushed with a minor style tweak. Thanks a lot, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
[PATCH] Cygwin: document the recent FIFO changes
--- winsup/cygwin/release/3.1.0 | 14 ++ winsup/doc/new-features.xml | 12 2 files changed, 26 insertions(+) create mode 100644 winsup/cygwin/release/3.1.0 diff --git a/winsup/cygwin/release/3.1.0 b/winsup/cygwin/release/3.1.0 new file mode 100644 index 0..1f017bfd1 --- /dev/null +++ b/winsup/cygwin/release/3.1.0 @@ -0,0 +1,14 @@ +What's new: +--- + + +What changed: +- + +- FIFOs can now be opened multiple times for writing. + Addresses: https://cygwin.com/ml/cygwin/2015-03/msg00047.html + https://cygwin.com/ml/cygwin/2015-12/msg00311.html + + +Bug Fixes +- diff --git a/winsup/doc/new-features.xml b/winsup/doc/new-features.xml index e14fbb1e8..c87601e9d 100644 --- a/winsup/doc/new-features.xml +++ b/winsup/doc/new-features.xml @@ -4,6 +4,18 @@ What's new and what changed in Cygwin +What's new and what changed in 3.1 + + + + +FIFOs can now be opened multiple times for writing. + + + + + + What's new and what changed in 3.0 -- 2.17.0
Re: [PATCH] Cygwin: document the recent FIFO changes
On Mar 27 18:10, Ken Brown wrote: > --- > winsup/cygwin/release/3.1.0 | 14 ++ > winsup/doc/new-features.xml | 12 > 2 files changed, 26 insertions(+) > create mode 100644 winsup/cygwin/release/3.1.0 > > diff --git a/winsup/cygwin/release/3.1.0 b/winsup/cygwin/release/3.1.0 > new file mode 100644 > index 0..1f017bfd1 > --- /dev/null > +++ b/winsup/cygwin/release/3.1.0 > @@ -0,0 +1,14 @@ > +What's new: > +--- > + > + > +What changed: > +- > + > +- FIFOs can now be opened multiple times for writing. > + Addresses: https://cygwin.com/ml/cygwin/2015-03/msg00047.html > + https://cygwin.com/ml/cygwin/2015-12/msg00311.html > + > + > +Bug Fixes > +- > diff --git a/winsup/doc/new-features.xml b/winsup/doc/new-features.xml > index e14fbb1e8..c87601e9d 100644 > --- a/winsup/doc/new-features.xml > +++ b/winsup/doc/new-features.xml > @@ -4,6 +4,18 @@ > > What's new and what changed in Cygwin > > +What's new and what changed in 3.1 > + > + > + > + > +FIFOs can now be opened multiple times for writing. > + > + > + > + > + > + > What's new and what changed in 3.0 > > > -- > 2.17.0 Pushed. Thanks, Corinna -- Corinna Vinschen Cygwin Maintainer signature.asc Description: PGP signature
Re: [PATCH RFC] fork: reduce chances for "address space is already occupied" errors
Michael Haubenwallner writes: > As far as I understand, rebasing is about touching already installed > dlls as well, which would require to restart all Cygwin processes. > As the problem is about some dll built during a larger build job, > this is not something that feels useful to me. That's exactly why I introduced the "--oblivious" option several years ago. It'll let you rebase a set of DLL while benefitting from the rebase database, but not recording them there, so if you later install them properly there will be no collision. I needed this for testing newly compiled Perl XS modules, but you seem to have a similar use case. Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ Wavetables for the Waldorf Blofeld: http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables
[PATCH] Cygwin: FIFO: implement clear_readahead
Make fhandler_base::clear_readahead virtual, and implement
fhandler_fifo::clear_readahead. This is called by
dtable::fixup_after_exec; it clears the readahead in each client.
---
winsup/cygwin/fhandler.h | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 3398cc625..21fec9e38 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -444,7 +444,7 @@ public:
return dev ().native ();
}
virtual bg_check_types bg_check (int, bool = false) {return bg_ok;}
- void clear_readahead ()
+ virtual void clear_readahead ()
{
raixput = raixget = ralen = rabuflen = 0;
rabuf = NULL;
@@ -1302,6 +1302,12 @@ public:
bool arm (HANDLE h);
void fixup_after_fork (HANDLE);
int __reg2 fstatvfs (struct statvfs *buf);
+ void clear_readahead ()
+ {
+fhandler_base::clear_readahead ();
+for (int i = 0; i < nclients; i++)
+ client[i].fh->clear_readahead ();
+ }
select_record *select_read (select_stuff *);
select_record *select_write (select_stuff *);
select_record *select_except (select_stuff *);
--
2.17.0
