Re: [PATCH] Add-on to gethostbyname2
-Original Message- From: Pierre A Humblet Sent: Friday, January 23, 2015 9:30 AM > From: Corinna Vinschen > Sent: Friday, January 23, 2015 5:48 AM > > On Jan 22 21:05, Pierre A. Humblet wrote: > > Add-on to gethostbyname2, as discussed previously on main list. > > The diff is also attached. > > > > Do you have some wording for the release info in the docs, please? > Make gethostbyname2 handle numerical host addresses as well as the reserved domain names "localhost" and "invalid". Actually it should be "numeric host addresses" Pierre
[PATCH] Add-on to gethostbyname2
Add-on to gethostbyname2, as discussed previously on main list.
The diff is also attached.
Pierre
2015-01-22 Pierre A. Humblet
* net.cc (cygwin_inet_pton): Declare.
(gethostby_specials): New function.
(gethostby_helper): Change returned addrtype in 4-to-6 case.
(gethostbyname2): Call gethostby_specials.
cvs diff -up net.cc
Index: net.cc
===
RCS file: /cvs/src/src/winsup/cygwin/net.cc,v
retrieving revision 1.322
diff -u -p -r1.322 net.cc
--- net.cc 20 Jan 2015 18:23:19 - 1.322
+++ net.cc 23 Jan 2015 00:02:22 -
@@ -72,6 +72,7 @@ extern "C"
int __stdcall rcmd (char **ahost, unsigned short inport, char *locuser,
char *remuser, char *cmd, SOCKET * fd2p);
int sscanf (const char *, const char *, ...);
+ int cygwin_inet_pton(int, const char *, void *);
int cygwin_inet_aton(const char *, struct in_addr *);
const char *cygwin_inet_ntop (int, const void *, char *, socklen_t);
int dn_length1(const unsigned char *, const unsigned char *,
@@ -1168,6 +1169,97 @@ memcpy4to6 (char *dst, const u_char *src
memcpy (dst + 12, src, NS_INADDRSZ);
}
+/* gethostby_specials: RFC 6761
+ Handles numerical addresses and special names for gethostbyname2 */
+static hostent *
+gethostby_specials (const char *name, const int af,
+ const int addrsize_in, const int addrsize_out)
+{
+ int namelen = strlen (name);
+ /* Ignore a final '.' */
+ if ((namelen == 0) || ((namelen -= (name[namelen - 1] == '.')) == 0)) {
+set_errno (EINVAL);
+h_errno = NETDB_INTERNAL;
+return NULL;
+ }
+
+ int res;
+ u_char address[NS_IN6ADDRSZ];
+ /* Test for numerical addresses */
+ res = cygwin_inet_pton(af, name, address);
+ /* Test for special domain names */
+ if (res != 1) {
+{
+ char const match[] = "invalid";
+ int const matchlen = sizeof(match) - 1;
+ int start = namelen - matchlen;
+ if ((start >= 0) && ((start == 0) || (name[start-1] == '.'))
+ && (strncasecmp (&name[start], match , matchlen) == 0)) {
+ h_errno = HOST_NOT_FOUND;
+ return NULL;
+ }
+}
+{
+ char const match[] = "localhost";
+ int const matchlen = sizeof(match) - 1;
+ int start = namelen - matchlen;
+ if ((start >= 0) && ((start == 0) || (name[start-1] == '.'))
+ && (strncasecmp (&name[start], match , matchlen) == 0)) {
+ res = 1;
+ if (af == AF_INET) {
+ address[0] = 127;
+ address[1] = address[2] = 0;
+ address[3] = 1;
+ }
+ else {
+ memset (address, 0, NS_IN6ADDRSZ);
+ address[NS_IN6ADDRSZ-1] = 1;
+ }
+ }
+}
+ }
+ if (res != 1)
+return NULL;
+
+ int const alias_count = 0, address_count = 1;
+ char * string_ptr;
+ int sz = DWORD_round (sizeof(hostent))
++ sizeof (char *) * (alias_count + address_count + 2)
++ namelen + 1
++ address_count * addrsize_out;
+ hostent *ret = realloc_ent (sz, (hostent *) NULL);
+ if (!ret)
+{
+ /* errno is already set */
+ h_errno = NETDB_INTERNAL;
+ return NULL;
+}
+
+ ret->h_addrtype = af;
+ ret->h_length = addrsize_out;
+ ret->h_aliases = (char **) (((char *) ret) + DWORD_round (sizeof(hostent)));
+ ret->h_addr_list = ret->h_aliases + alias_count + 1;
+ string_ptr = (char *) (ret->h_addr_list + address_count + 1);
+ ret->h_name = string_ptr;
+
+ memcpy (string_ptr, name, namelen);
+ string_ptr[namelen] = 0;
+ string_ptr += namelen + 1;
+
+ ret->h_addr_list[0] = string_ptr;
+ if (addrsize_in != addrsize_out) {
+memcpy4to6 (string_ptr, address);
+ret->h_addrtype = AF_INET6;
+ }
+ else
+memcpy (string_ptr, address, addrsize_out);
+
+ ret->h_aliases[alias_count] = NULL;
+ ret->h_addr_list[address_count] = NULL;
+
+ return ret;
+}
+
static hostent *
gethostby_helper (const char *name, const int af, const int type,
const int addrsize_in, const int addrsize_out)
@@ -1352,8 +1444,10 @@ gethostby_helper (const char *name, cons
string_ptr += curptr->namelen1;
}
ret->h_addr_list[address_count++] = string_ptr;
- if (addrsize_in != addrsize_out)
+ if (addrsize_in != addrsize_out) {
memcpy4to6 (string_ptr, curptr->data);
+ ret->h_addrtype = AF_INET6;
+ }
else
memcpy (string_ptr, curptr->data, addrsize_in);
string_ptr += addrsize_out;
@@ -1405,7 +1499,10 @@ gethostbyname2 (const char *name, int af
__leave;
}
- res = gethostby_helper (name, af, type, addrsize_in, addrsize_out);
+ h_errno = NETDB_SUCCESS;
+ res = gethostby_specials (name, af, addrsize_in, addrsize_out);
+ if ((res == NULL) &
[Patch] gethostby_helper
This patch has already been already applied.
Diff in attachment and also below.
Pierre
2011-08-16 Pierre Humblet
* net.cc (gethostby_helper): Remove DEBUGGING code from and
streamline the second pass.
Index: net.cc
===
RCS file: /cvs/src/src/winsup/cygwin/net.cc,v
retrieving revision 1.289
diff -u -p -r1.289 net.cc
--- net.cc 4 Aug 2011 08:22:11 - 1.289
+++ net.cc 16 Aug 2011 23:31:44 -
@@ -1198,57 +1198,34 @@ gethostby_helper (const char *name, cons
string_ptr = (char *) (ret->h_addr_list + address_count + 1);
/* Rescan the answers */
- ancount = alias_count + address_count; /* Valid records */
alias_count = address_count = 0;
+ prevptr->set_next (prevptr + 1);
- for (i = 0, curptr = anptr; i < ancount; i++, curptr = curptr->next ())
+ for (curptr = anptr; curptr <= prevptr; curptr = curptr->next ())
{
antype = curptr->type;
if (antype == ns_t_cname)
{
- complen = dn_expand (msg, eomsg, curptr->name (),
string_ptr, string_size);
-#ifdef DEBUGGING
- if (complen != curptr->complen)
- goto debugging;
-#endif
+ dn_expand (msg, eomsg, curptr->name (), string_ptr,
curptr->namelen1);
ret->h_aliases[alias_count++] = string_ptr;
- namelen1 = curptr->namelen1;
- string_ptr += namelen1;
- string_size -= namelen1;
- continue;
+ string_ptr += curptr->namelen1;
}
- if (antype == type)
+ else
+ {
+ if (address_count == 0)
{
- if (address_count == 0)
- {
- complen = dn_expand (msg, eomsg, curptr->name(),
string_ptr, string_size);
-#ifdef DEBUGGING
- if (complen != curptr->complen)
- goto debugging;
-#endif
- ret->h_name = string_ptr;
- namelen1 = curptr->namelen1;
- string_ptr += namelen1;
- string_size -= namelen1;
- }
- ret->h_addr_list[address_count++] = string_ptr;
- if (addrsize_in != addrsize_out)
- memcpy4to6 (string_ptr, curptr->data);
- else
- memcpy (string_ptr, curptr->data, addrsize_in);
- string_ptr += addrsize_out;
- string_size -= addrsize_out;
- continue;
- }
-#ifdef DEBUGGING
- /* Should not get here */
- goto debugging;
-#endif
+ dn_expand (msg, eomsg, curptr->name (), string_ptr,
curptr->namelen1);
+ ret->h_name = string_ptr;
+ string_ptr += curptr->namelen1;
+ }
+ ret->h_addr_list[address_count++] = string_ptr;
+ if (addrsize_in != addrsize_out)
+ memcpy4to6 (string_ptr, curptr->data);
+ else
+ memcpy (string_ptr, curptr->data, addrsize_in);
+ string_ptr += addrsize_out;
+ }
}
-#ifdef DEBUGGING
- if (string_size < 0)
-goto debugging;
-#endif
free (msg);
@@ -1263,16 +1240,6 @@ gethostby_helper (const char *name, cons
Should it be NO_RECOVERY ? */
h_errno = TRY_AGAIN;
return NULL;
-
-
-#ifdef DEBUGGING
- debugging:
- system_printf ("Please debug.");
- free (msg);
- free (ret);
- h_errno = NO_RECOVERY;
- return NULL;
-#endif
}
/* gethostbyname2: standards? */
net.cc.diff
Description: Binary data
Re: Mounting /tmp at TMP or TEMP as a last resort
- Original Message - From: "Dave Korn" To: Sent: Thursday, September 09, 2010 16:03 | On 08/09/2010 23:41, Christopher Faylor wrote: | > Corinna may disagree, | | Needless to say, I'm not Corinna! | | > but I think we | > should keep the parsing of /etc/fstab as lean as possible; | | I don't understand why. How many times per second does /etc/fstab get parsed? My problem with the original patch is that the mountinfo is kept per user, if my memory is correct, and /etc/fstab is parsed by the first process of a user. So, for example, if the user logs in interactively while a cron job (or another service) is running, /tmp may be mapped differently than if no cron job is running, because TMP may be defined differently in the service environment. That is not desirable. Corinna's suggestions are more appealing. If there are objections to using a /etc/profile.d/tmp-mnt.sh or an installer script, why not have /etc/profile (or such) create /etc/fstab.d/$USER on the fly if needed ? Pierre
Re: res_send() doesn't work with osquery enabled
- Original Message -
From: "Corinna Vinschen"
To:
Sent: Thursday, August 26, 2010 12:38
| Pierre, would you mind to take a look?
|
| On Aug 26 19:07, pse...@egg6.net wrote:
| > Currently res_init() checks for availability of the native windows
| > function DnsQuery_A. If the function is found, it's preferred over the
| > cygwin implementation and res_query is set up to use it.
| > As DnsQuery_A finds the configured name servers itself, the current code
| > assumes we can avoid loading the dns server list with GetNetworkParams().
| >
| > However, the assumption that everybody would use res_query is wrong. Some
| > programs may use res_mkquery() and res_send() or may only read the list of
| > servers from _res.nsaddr_list and send/receive the queries/replies
| > themselves. res_send() also relies on nsaddr_list.
It's true that the behavior described above is legitimate, even if nobody had
ever
requested it. If people want to access nsaddr_list after calling res_ninit,
loading
iphlpapi.dll every time (as the patch does) is unavoidable.
The other change has res_nsend return an error if no server can be found.
Alternatively the error could be reported by res_ninit, by removing the second
condition in
if (statp->nscount == 0 && !statp->os_query) {
errno = ENONET;
statp->res_h_errno = NETDB_INTERNAL;
Hypothetically this could affect some installations where iphlpapi doesn't
report any
servers although the Windows resolver can find a server (but I don't see how
this
could happen), so it's safer to proceed as in the patch.
However the patch should send errno to ENONET and set res_h_errno to
NETDB_INTERNAL
Except for the previous comment, I am fine with the patch.
Pierre
[PATCH] check_access()
This fixes problem # 3 in http://cygwin.com/ml/cygwin/2010-02/msg00330.html
Pierre
Index: ChangeLog
===
RCS file: /cvs/src/src/winsup/cygwin/ChangeLog,v
retrieving revision 1.4845
diff -u -r1.4845 ChangeLog
--- ChangeLog 25 Feb 2010 16:55:01 - 1.4845
+++ ChangeLog 26 Feb 2010 01:29:30 -
@@ -1,3 +1,8 @@
+2010-02-26 Pierre Humblet
+
+ * security.cc (check_access): Use user.imp_token if appropriate.
+Set errno and return if DuplicateTokenEx fails .
+
2010-02-25 Corinna Vinschen
* lc_era.h (lc_era_t): Fix apparent glibc bug in ja_JP era definition.
Index: security.cc
===
RCS file: /cvs/src/src/winsup/cygwin/security.cc,v
retrieving revision 1.239
diff -u -p -r1.239 security.cc
--- security.cc 3 Nov 2009 09:31:45 - 1.239
+++ security.cc 26 Feb 2010 01:24:13 -
@@ -751,16 +751,17 @@ check_access (security_descriptor &sd, G
? cygheap->user.imp_token ()
: hProcImpToken);
- if (!tok && !DuplicateTokenEx (hProcToken, MAXIMUM_ALLOWED, NULL,
-SecurityImpersonation, TokenImpersonation,
-&hProcImpToken))
-#ifdef DEBUGGING
- system_printf ("DuplicateTokenEx failed, %E");
-#else
- syscall_printf ("DuplicateTokenEx failed, %E");
-#endif
- else
-tok = hProcImpToken;
+ if (!tok)
+{
+ if (!DuplicateTokenEx (hProcToken, MAXIMUM_ALLOWED, NULL,
+ SecurityImpersonation, TokenImpersonation,
+ &hProcImpToken))
+ {
+__seterrno ();
+return ret;
+ }
+ tok = hProcImpToken;
+}
if (!AccessCheck (sd, tok, desired, &mapping, pset, &plen,
&granted, &status))
__seterrno ();
Re: dup3/O_CLOEXEC/F_DUPFD_CLOEXEC
At 05:52 PM 1/15/2010, Pierre A. Humblet wrote: The scenario you describe (one packet only, with a long delay between accept and WSAEventSelect) could easily be tested to settle the matter. Put a sleep before fdsock ! To close the matter, I have done just that, putting a 60 s sleep in :accept4 between the call to Windows accept and fdsock. Packet doesn't get lost :) Server: 2010_1_17.22:31:41 Listening 2010_1_17.22:32:43 Accepted 2010_1_17.22:32:43 Read 6 hello Client: 2010_1_17.22:31:43 Connecting to localhost 2010_1_17.22:31:43 Connected to localhost 2010_1_17.22:31:43 Written 6 hello 2010_1_17.22:32:58 Exiting Pierre
Re: dup3/O_CLOEXEC/F_DUPFD_CLOEXEC
- Original Message - From: "Corinna Vinschen" To: Sent: Friday, January 15, 2010 17:03 | On Jan 15 16:33, Pierre A. Humblet wrote: | > From: "Corinna Vinschen" | > | Oh, hang on. I guess we should better stick to the BSD behaviour. | > | Any call to WSAAsyncSelect or WSAEventSelect clears Winsock's internal | > | network event queue. This could lead to connection errors. Given | > | that, the switch to a specific mode should stay the responsibility | > | of the application, shouldn't it? | > | > I know next to nothing about this but notice that :accept4 calls fdsock | > which calls init_events which calls WSAEventSelect . | > Isn't that what you want to avoid? | | Oh, right. I'm wondering how this is supposed to work at all in | WinSock, if an application is using, say, blocking listening sockets and | only wants to use event driven IO on the accepted sockets. It looks | like this is impossible without the danger of losing information. In | theory, if the peer sends exactly one packet, the accepting socket could | wait forever for the packet if it arrived before WSAEventSelect has been | called. The FD_READ will never show up. The alternative is I'm just | exaggerating the potential problem. I don't know. | | > On the other hand I don't see how you can avoid it given this: | > | > Any WSAEventSelect association and network events selection set for the listening socket apply | > to the accepted socket. For example, if a listening socket has WSAEventSelect association of | > hEventObject with FD_ACCEPT, FD_READ, and FD_WRITE, then any socket accepted on that listening | > socket will also have FD_ACCEPT, FD_READ, and FD_WRITE network events associated with the same | > hEventObject. If a different hEventObject or network events are desired, the application should | > call WSAEventSelect, passing the accepted socket and the desired new information. | | The event mask is not the problem since the mask given to WSAEventSelect | is always the same in Cygwin, the whole set, regardless of how the | socket is used. The problem is that every socket needs its own | event object so WSAEventSelect has to be called anyway. I agree. It would be nice if the new event could be initialized to the value of the old. Then we could have too many events for the new socket, but that's OK. I am also wondering if we are misreading the doc. It says: For FD_READ, FD_OOB, and FD_ACCEPT network events, network event recording and event object signaling are level-triggered. and it goes on to provide an example where the event is reenabled if there is still data. The example given about the "clears the internal network event record" is of a completely different nature. The scenario you describe (one packet only, with a long delay between accept and WSAEventSelect) could easily be tested to settle the matter. Put a sleep before fdsock ! | What this means is, the accepted socket isn't in async mode anymore | since the WSAEventSelect call in init_events has ended it. So the async | flag is erroneously preserved and we will have to apply this patch AFAICS. At least we follow Linux :) Pierre
Re: dup3/O_CLOEXEC/F_DUPFD_CLOEXEC
- Original Message - From: "Corinna Vinschen" To: Sent: Friday, January 15, 2010 15:34 | On Jan 15 21:22, Corinna Vinschen wrote: | > On Jan 15 15:04, Pierre A. Humblet wrote: | > > I see an issue with accept/accept4 and was going to ask you how to | > > handle it. | > > | > > Before your changes in Cygwin the socket returned by accept had the | > > same blocking (and async) property as the listening socket. | > > Apparently this conforms to BSD but not to Linux (even old versions | > > without accept4), | > > http://www.kernel.org/doc/man-pages/online/pages/man2/accept.2.html | > > POSIX is silent on the topic. | > > | > > After your changes the new socket is non-blocking if either the | > > listening socket was non-blocking or SOCK_NONBLOCK is specified. This | > > does not conform to Linux. | > > | > > Why not have accept4 conform to Linux but keep the old behavior of accept by | > > changing accept in net.cc to | > > res = fh->accept4 (peer, len, fh->is_nonblocking () ? SOCK_NONBLOCK : 0); | > > | > > There is a similar Linux discrepancy with async_io. | > | > I have no problem to change the SOCK_NONBLOCK stuff as you proposed. | > | > I don't like the idea to introduce such a new flag for ASYNC which | > doesn't exist on Linux, though. How important is the async mode anyway? | > Will we really get any problems with existing apps if we switch to the | > Linux behaviour for async? | | Oh, hang on. I guess we should better stick to the BSD behaviour. | Any call to WSAAsyncSelect or WSAEventSelect clears Winsock's internal | network event queue. This could lead to connection errors. Given | that, the switch to a specific mode should stay the responsibility | of the application, shouldn't it? I know next to nothing about this but notice that :accept4 calls fdsock which calls init_events which calls WSAEventSelect . Isn't that what you want to avoid? On the other hand I don't see how you can avoid it given this: Any WSAEventSelect association and network events selection set for the listening socket apply to the accepted socket. For example, if a listening socket has WSAEventSelect association of hEventObject with FD_ACCEPT, FD_READ, and FD_WRITE, then any socket accepted on that listening socket will also have FD_ACCEPT, FD_READ, and FD_WRITE network events associated with the same hEventObject. If a different hEventObject or network events are desired, the application should call WSAEventSelect, passing the accepted socket and the desired new information. Pierre
Re: dup3/O_CLOEXEC/F_DUPFD_CLOEXEC
- Original Message - From: "Corinna Vinschen" To: Sent: Friday, January 15, 2010 10:42 | On Jan 14 17:09, Corinna Vinschen wrote: | > On Jan 14 08:39, Pierre A. Humblet wrote: | > > | > > For the same reason we should also have SOCK_CLOEXEC, and | > > SOCK_NONBLOCK while we are at it. I would use them in minires. | > | > Sure, but probably not yet, as far as my hack time is concerned. But | > of course SHTDI, PTC, and all that. I'd be glad for it, actually. | | It was simpler than I anticipated. I just applied a patch to implement | accept4, and SOCK_NONBLOCK as well as SOCK_CLOEXEC for socket, | socketpair and accept4. Thanks, I was just looking into that. I see an issue with accept/accept4 and was going to ask you how to handle it. Before your changes in Cygwin the socket returned by accept had the same blocking (and async) property as the listening socket. Apparently this conforms to BSD but not to Linux (even old versions without accept4), http://www.kernel.org/doc/man-pages/online/pages/man2/accept.2.html POSIX is silent on the topic. After your changes the new socket is non-blocking if either the listening socket was non-blocking or SOCK_NONBLOCK is specified. This does not conform to Linux. Why not have accept4 conform to Linux but keep the old behavior of accept by changing accept in net.cc to res = fh->accept4 (peer, len, fh->is_nonblocking () ? SOCK_NONBLOCK : 0); There is a similar Linux discrepancy with async_io. Pierre
Re: dup3/O_CLOEXEC/F_DUPFD_CLOEXEC
At 08:17 AM 1/14/2010, Corinna Vinschen wrote: On Jan 14 06:02, Eric Blake wrote: > > In a multi-threaded app, any fd that is opened only temporarily, such as > the one in mq_open, should be opened with O_CLOEXEC, so that no other > thread can win a race and do a fork/exec inside the window when the > temporary fd was open. So even though mq_open does not leak an fd to the > current process, it should pass O_CLOEXEC as part of its internal open() > call in order to avoid leaking the fd to unrelated child processes. Uh, ok, that makes sense. I'll send a revised patch later today. It will also include the pipe2 implementation. For the same reason we should also have SOCK_CLOEXEC, and SOCK_NONBLOCK while we are at it. I would use them in minires. Pierre
Re: [Patch] gethostbyname2 again
- Original Message - From: "Christopher Faylor" | On Fri, Mar 06, 2009 at 09:41:00AM -0500, Pierre A. Humblet wrote: | > || | This is ok with one very minor formatting nit. Please check in with an | appropriate changelog. | | >+static inline hostent * | >+realloc_ent (int sz, hostent * ) |^ | extra space OK. I can't do that before Mon eve. It would be easier if Corinna could merge this patch and the previous one (she has the latest version) and apply the whole thing at once, with one changelog block. Pierre
Re: [Patch] gethostbyname2 again
- Original Message -
From: "Christopher Faylor"
To:
Sent: Friday, March 06, 2009 12:44 AM
Subject: Re: [Patch] gethostbyname2 again
| On Tue, Mar 03, 2009 at 12:50:21PM -0500, Pierre A. Humblet wrote:
| >
| >To avoid real-time checks, I could do as what dup_ent does, and have 4
versions
| >of the realloc_ent function, one main one with dst and sz arguments (that one
| >would be called by dup_ent without any run-time checks) and 3 (actually only
| >1 is needed for now) that invoke the main one with the correct dst based on
the
| >type of the src argument . The src argument would be null but would have the
| >right type! That seems to meet your wishes. OK?
|
| Yes.
OK, here it the patch for realloc_ent. See also attachement.
The third chunk (the change to dup_ent) is not essential.
In addition in the patch that Corinna sent on March 3, the line
+ ret = (hostent *) realloc_ent (sz, unionent::t_hostent);
should be changed to
ret = realloc_ent (sz, (hostent *) NULL);
In the Changelog the line
(dup_ent): Remove dst argument and call realloc_ent.
should either be deleted or "Remove dst argument and c" should
be replaced by "C".
Pierre
Index: net.cc
===
RCS file: /cvs/src/src/winsup/cygwin/net.cc,v
retrieving revision 1.249
diff -u -p -r1.249 net.cc
--- net.cc 3 Mar 2009 11:44:17 - 1.249
+++ net.cc 6 Mar 2009 14:28:46 -
@@ -264,6 +264,25 @@ struct pservent
static const char *entnames[] = {"host", "proto", "serv"};
+static unionent *
+realloc_ent (unionent *&dst, int sz)
+{
+ /* Allocate the storage needed. Allocate a rounded size to attempt to force
+ reuse of this buffer so that a poorly-written caller will not be using
+ a freed buffer. */
+ unsigned rsz = 256 * ((sz + 255) / 256);
+ unionent * ptr;
+ if ((ptr = (unionent *) realloc (dst, rsz)))
+ dst = ptr;
+ return ptr;
+}
+
+static inline hostent *
+realloc_ent (int sz, hostent * )
+{
+ return (hostent *) realloc_ent (_my_tls.locals.hostent_buf, sz);
+}
+
/* Generic "dup a {host,proto,serv}ent structure" function.
This is complicated because we need to be able to free the
structure at any point and we can't rely on the pointer contents
@@ -355,13 +374,8 @@ dup_ent (unionent *&dst, unionent *src,
}
}
- /* Allocate the storage needed. Allocate a rounded size to attempt to force
- reuse of this buffer so that a poorly-written caller will not be using
- a freed buffer. */
- unsigned rsz = 256 * ((sz + 255) / 256);
- dst = (unionent *) realloc (dst, rsz);
-
- if (dst)
+ /* Allocate the storage needed. */
+ if (realloc_ent (dst, sz))
{
memset (dst, 0, sz);
/* This field is common to all *ent structures but named differently
realloc_ent.diff
Description: Binary data
Re: [Patch] gethostbyname2 again
- Original Message - From: "Christopher Faylor" <> To: Sent: Tuesday, March 03, 2009 10:38 AM Subject: Re: [Patch] gethostbyname2 again | On Mon, Mar 02, 2009 at 08:36:55PM -0500, Pierre A. Humblet wrote: | >realloc_ent function, and call it from both dup_ent and the helper. That | > caused minor | >changes in the 4 versions of dup_ent, and I don't know exactly what | > format to use in the ChangeLog | | I would rather that you keep dup_ent as is so that there is no need to | do run-time checks on the type. If you need to do something similar to | what is currently in dupent, then couldn't you still create a | realloc_ent but just pass in the destination pointer? Or even just make | realloc_ent mimic realloc but do the rounding that seems to be the | impetus for your breaking this out into a separate function. Chris, The impetus is that the new helper function is capable of formatting a fine hostent in a single block of memory. So it doesn't need to have dup_ent copy it in yet another memory block. However it still needs to store a pointer to the block of memory somewhere, so that it can be freed later, and reusing the tls.locals seems logical. If it does that, then it must also free or realloc whatever is stored there. That's what realloc_ent does. The rounding is not essential, it's just nice to do it consistently in one place. To avoid real-time checks, I could do as what dup_ent does, and have 4 versions of the realloc_ent function, one main one with dst and sz arguments (that one would be called by dup_ent without any run-time checks) and 3 (actually only 1 is needed for now) that invoke the main one with the correct dst based on the type of the src argument . The src argument would be null but would have the right type! That seems to meet your wishes. OK? Pierre
Re: [Patch] gethostbyname2 again
- Original Message - From: "Corinna Vinschen" To: Sent: Tuesday, March 03, 2009 10:13 AM Subject: Re: [Patch] gethostbyname2 again | On Mar 3 10:05, Pierre A. Humblet wrote: | > From: "Corinna Vinschen" | > | The ChangeLog entry is the same as in the OP, except for the additional | > | reference to include/cygwin/version.h. Please have a look. | > | > Hello Corinna, | > | > Thanks for fixing up the nits. The new patch looks good. I didn't find the new | > Changelog entry but I trust you. | | Right between my signature and the patch, the last line :) Indeed, there it is :) It's fine except that dn_length1 is now a new function in minires.c instead of net.cc Pierre
Re: [Patch] gethostbyname2 again
- Original Message - From: "Corinna Vinschen" To: Sent: Tuesday, March 03, 2009 7:01 AM Subject: Re: [Patch] gethostbyname2 again | [| | I attached the entire patch again with my changes. I had to change the | gethostby_helper function to define some of the variables at the start | of the function, othewise gcc complained about jumps to a label crossing | variable initializations. The bump of the API minor number in | include/cygwin/version.h was missing. I also tweaked the formatting a bit. | | The ChangeLog entry is the same as in the OP, except for the additional | reference to include/cygwin/version.h. Please have a look. | Hello Corinna, Thanks for fixing up the nits. The new patch looks good. I didn't find the new Changelog entry but I trust you. Pierre
[Patch] gethostbyname2 again
Corinna, OK, here we go again. This version calls res_query and all the work is done in net.cc. As discussed before that means a lot of work is wasted when using the Windows resolver. That will be improved later. There is no progress on the issue of resolving local names (NetBIOS over TCP) for now, so it's not a perfect replacement for the native gethostbyname yet. Misc nits and notes: - Including resolv.h in net.cc causes havoc because it pulls in cygwin/in.h which conflicts with winsock2.h. I worked around that by defining _CYGWIN_IN_H before including resolv.h - Because of the above, asm/byteorder.h doesn't get pulled in either, and I couldn't use some ntoh macros (see memcpy4to6). - I could have included asm/byteorder separately but that causes conflicts with the local ntoh definitions in net.cc. - Because arpa/nameser.h is now pulled in, IN6ADDRSZ etc are now defined, but in a way different from done in the snippet of code cut & pasted from bind. I didn't want to change that piece (in case you want to keep in intact for some reason) and I ended up undefining IN6ADDRSZ etc . - There is a new helper function dn_length1 which logically belongs in minires.c, although it shouldn't be exported by the dll. However if I place it in minires.c, then the linker doesn't find it. Fixing that probably involves some Makefile magic. - I structured the code with a helper function gethostby_helper. That will make it very easy to support a gethostbyaddress some day, if needed. - The helper function avoids using dup_ent (there is enough copying already). I created a new realloc_ent function, and call it from both dup_ent and the helper. That caused minor changes in the 4 versions of dup_ent, and I don't know exactly what format to use in the ChangeLog - This is much more complex than the first way of doing things. Needs testing! - The patch is long, see the attachment. There is also a test program attached. Pierre 2009-03-02 Pierre Humblet * net.cc: define _CYGWIN_IN_H and include resolv.h. (realloc_ent): New function. (dup_ent): Remove dst argument and call realloc_ent. (memcpy4to6): New function. (dn_length1): New function. (gethostby_helper): New function. (gethostbyname2): New function. * cygwin.din: Export gethostbyname2. * libc/minires.c (get_options): Look for "inet6" and apply bounds to "retry" and "retrans". (res_ninit): Set the default options at the beginning. (dn_expand): Fix "off by one". gethostbyname2_b.diff Description: Binary data try_gethostbyname.c Description: Binary data
Re: [Patch] gethostbyname2
- Original Message - From: "Corinna Vinschen" To: Sent: Thursday, February 26, 2009 11:05 AM Subject: Re: [Patch] gethostbyname2 | On Feb 26 10:29, Pierre A. Humblet wrote: | > At 04:52 AM 2/26/2009, Corinna Vinschen wrote: | > | >On Feb 25 23:03, Pierre A. Humblet wrote: | > | > > I tried to compile Exim with IPv6 enabled and Cygwin 1.7, but it needs | > | > > gethostbyname2. | > | > > Here is an implementation of that function. | > | > > In attachment I am including the same patch as well as a short test function. | > | > > | > | > | > | >This is way cool! I have this function on my TODO list for ages. | > | > | > | >But there's a problem. You're using DnsQuery_A directly, but this | > | >function only exists since Win2K. Would it be a big problem to rework | > | >the function to use the resolver functions instead? They are part of | > | >Cygwin now anyway and that would abstract gethostbyname2 from the | > | >underlying OS capabilities. | > | > I was afraid of that. Using res_query was my initial thought, but I realized that when using the | > Windows resolver I would undo in gethostbyname2 all the work done in minires. | | I'm sorry, but I really don't understand what you mean. How are you | undoing work in minires when using minires in gethostbyname2?!? Why | isn't it just possible to call res_query from there? It is possible of course. But the sequence would be the following 1) External DNS server sends compressed records to Windows resolver 2) Windows resolver uncompresses the records and puts them in nice structures 3) Minires takes the nice structures and recompresses them to wire format 4) Gethostbyname2 uncompresses them into nice structures then 5) calls dup_ent, which copies them in the tls.locals I would like to streamline the process: - With Windows resolver: 1, 2, 5 (that's the current patch, ~20% of which is cut&pasted from minires and could be restructured to use a common function) - Without: Straight fom wire format records to the tls.locals memory block Or: have a routine 2a) in minires that replaces 2. Then it would be 1, 2a, 5. | > I am still fighting one issue with Windows. On XP, when using the native gethostbyname | > I can resolve computers on my local net (through NetBIOS or such). But I can't get | > them with DnsQuery, except my own computer, despite what I think the doc says. | > Any insight? | | I never used the DnsQuery functions myself. There's a DnsQuery flag | called DNS_QUERY_NO_NETBT documented in MSDN, maybe there's something | switched off on your machine so that's the default? Perhaps. But where is it or how does the native gethostbyname turn it on? To answer Dave, "DNS Client" service is running, I have never experimented turning it off. Pierre
Re: [Patch] gethostbyname2
At 04:52 AM 2/26/2009, Corinna Vinschen wrote: | >On Feb 25 23:03, Pierre A. Humblet wrote: | > > I tried to compile Exim with IPv6 enabled and Cygwin 1.7, but it needs | > > gethostbyname2. | > > Here is an implementation of that function. | > > In attachment I am including the same patch as well as a short test function. | > > | > | >This is way cool! I have this function on my TODO list for ages. | > | >But there's a problem. You're using DnsQuery_A directly, but this | >function only exists since Win2K. Would it be a big problem to rework | >the function to use the resolver functions instead? They are part of | >Cygwin now anyway and that would abstract gethostbyname2 from the | >underlying OS capabilities. I was afraid of that. Using res_query was my initial thought, but I realized that when using the Windows resolver I would undo in gethostbyname2 all the work done in minires. I am wondering if gethostbyname2 should not be moved out of net.cc and integrated with minires. We could design shortcuts to use the most appropriate method. I have read RFC 2133, section 6.1 . Do we want to implement having a RES_OPTIONS in the environment, in /etc/resolv.conf, or only by setting the appropriate flag in _res? What does Linux do? I am still fighting one issue with Windows. On XP, when using the native gethostbyname I can resolve computers on my local net (through NetBIOS or such). But I can't get them with DnsQuery, except my own computer, despite what I think the doc says. Any insight? Pierre
Re: [Patch] Make cygcheck handle Windows paths with spaces
Oops, I didn't notice that one line required a double fix.
Pierre
2009-01-05 Pierre Humblet
* cygcheck.cc (dump_sysinfo_services): Quote the path for popen.
Index: cygcheck.cc
===
RCS file: /cvs/src/src/winsup/utils/cygcheck.cc,v
retrieving revision 1.106
diff -u -p -r1.106 cygcheck.cc
--- cygcheck.cc 31 Dec 2008 01:44:36 - 1.106
+++ cygcheck.cc 5 Jan 2009 15:05:56 -
@@ -1137,7 +1137,7 @@ dump_sysinfo_services ()
/* For verbose mode, just run cygrunsrv --list --verbose and copy output
verbatim; otherwise run cygrunsrv --list and then cygrunsrv --query for
each service. */
- snprintf (buf, sizeof (buf), (verbose ? "\"%s\" --list --verbose" : "%s
--list"),
+ snprintf (buf, sizeof (buf), (verbose ? "\"%s\" --list --verbose" : "\"%s\"
--list"),
cygrunsrv);
if ((f = popen (buf, "rt")) == NULL)
{
- Original Message -
From: "Pierre A. Humblet"
To:
Sent: Tuesday, December 30, 2008 3:14 PM
Subject: [Patch] Make cygcheck handle Window paths with spaces
| Formatting is more likely to be preserved in the attached files.
|
| Pierre
|
| 2008-12-30 Pierre Humblet
|
|* cygcheck.cc (pretty_id): Quote the path for popen.
|(dump_sysinfo_services): Ditto.
|
|
| Index: cygcheck.cc
| ===
| RCS file: /cvs/src/src/winsup/utils/cygcheck.cc,v
| retrieving revision 1.105
| diff -u -p -r1.105 cygcheck.cc
| --- cygcheck.cc 12 Sep 2008 22:43:10 - 1.105
| +++ cygcheck.cc 30 Dec 2008 19:20:32 -
| @@ -1032,9 +1032,10 @@ pretty_id (const char *s, char *cygwin,
| return;
| }
|
| - FILE *f = popen (id, "rt");
| -
| char buf[16384];
| + snprintf (buf, sizeof (buf), "\"%s\"", id);
| + FILE *f = popen (buf, "rt");
| +
| buf[0] = '\0';
| fgets (buf, sizeof (buf), f);
| pclose (f);
| @@ -1118,7 +1119,7 @@ dump_sysinfo_services ()
| }
|
| /* check for a recent cygrunsrv */
| - snprintf (buf, sizeof (buf), "%s --version", cygrunsrv);
| + snprintf (buf, sizeof (buf), "\"%s\" --version", cygrunsrv);
| if ((f = popen (buf, "rt")) == NULL)
| {
| printf ("Failed to execute '%s', skipping services check.\n", buf);
| @@ -1136,7 +1137,7 @@ dump_sysinfo_services ()
| /* For verbose mode, just run cygrunsrv --list --verbose and copy output
| verbatim; otherwise run cygrunsrv --list and then cygrunsrv --query for
| each service. */
| - snprintf (buf, sizeof (buf), (verbose ? "%s --list --verbose" : "%s
--list"),
| + snprintf (buf, sizeof (buf), (verbose ? "\"%s\" --list --verbose" : "%s
--list"),
| cygrunsrv);
| if ((f = popen (buf, "rt")) == NULL)
| {
| @@ -1167,7 +1168,7 @@ dump_sysinfo_services ()
| if (nchars > 0)
| for (char *srv = strtok (buf, "\n"); srv; srv = strtok (NULL, "\n"))
|{
| - snprintf (buf2, sizeof (buf2), "%s --query %s", cygrunsrv, srv);
| + snprintf (buf2, sizeof (buf2), "\"%s\" --query %s", cygrunsrv, srv);
| if ((f = popen (buf2, "rt")) == NULL)
|{
| printf ("Failed to execute '%s', skipping services check.\n", buf2);
|
[Patch] Make cygcheck handle Window paths with spaces
Formatting is more likely to be preserved in the attached files.
Pierre
2008-12-30 Pierre Humblet
* cygcheck.cc (pretty_id): Quote the path for popen.
(dump_sysinfo_services): Ditto.
Index: cygcheck.cc
===
RCS file: /cvs/src/src/winsup/utils/cygcheck.cc,v
retrieving revision 1.105
diff -u -p -r1.105 cygcheck.cc
--- cygcheck.cc 12 Sep 2008 22:43:10 - 1.105
+++ cygcheck.cc 30 Dec 2008 19:20:32 -
@@ -1032,9 +1032,10 @@ pretty_id (const char *s, char *cygwin,
return;
}
- FILE *f = popen (id, "rt");
-
char buf[16384];
+ snprintf (buf, sizeof (buf), "\"%s\"", id);
+ FILE *f = popen (buf, "rt");
+
buf[0] = '\0';
fgets (buf, sizeof (buf), f);
pclose (f);
@@ -1118,7 +1119,7 @@ dump_sysinfo_services ()
}
/* check for a recent cygrunsrv */
- snprintf (buf, sizeof (buf), "%s --version", cygrunsrv);
+ snprintf (buf, sizeof (buf), "\"%s\" --version", cygrunsrv);
if ((f = popen (buf, "rt")) == NULL)
{
printf ("Failed to execute '%s', skipping services check.\n", buf);
@@ -1136,7 +1137,7 @@ dump_sysinfo_services ()
/* For verbose mode, just run cygrunsrv --list --verbose and copy output
verbatim; otherwise run cygrunsrv --list and then cygrunsrv --query for
each service. */
- snprintf (buf, sizeof (buf), (verbose ? "%s --list --verbose" : "%s --list"),
+ snprintf (buf, sizeof (buf), (verbose ? "\"%s\" --list --verbose" : "%s
--list"),
cygrunsrv);
if ((f = popen (buf, "rt")) == NULL)
{
@@ -1167,7 +1168,7 @@ dump_sysinfo_services ()
if (nchars > 0)
for (char *srv = strtok (buf, "\n"); srv; srv = strtok (NULL, "\n"))
{
- snprintf (buf2, sizeof (buf2), "%s --query %s", cygrunsrv, srv);
+ snprintf (buf2, sizeof (buf2), "\"%s\" --query %s", cygrunsrv, srv);
if ((f = popen (buf2, "rt")) == NULL)
{
printf ("Failed to execute '%s', skipping services check.\n", buf2);
cygcheck.cc.diff
Description: Binary data
ChangeLog.cygcheck
Description: Binary data
Re: requires
- Original Message - From: "Corinna Vinschen" <[EMAIL PROTECTED]> To: Sent: Tuesday, December 09, 2008 4:23 AM Subject: Re: requires | On Dec 8 17:52, Yaakov (Cygwin/X) wrote: | > -BEGIN PGP SIGNED MESSAGE- | > Hash: SHA256 | > | > Pierre A. Humblet wrote: | > > Every version of man resolver that I have ever seen specifies: | > > | > > SYNOPSIS | > > #include | > > #include | > > #include | > > #include | > > | > > So it's up to the user to include the right files. | > | > Perhaps so, but: | > | > 1) already #includes all of those headers *except* for | > . | > | > 2) this does not match Linux behaviour: | > | > http://sourceware.org/cgi-bin/cvsweb.cgi/libc/resolv/resolv.h?cvsroot=glibc | > | > As I stated, my STC was based on a configure test which works on other | > platforms; I don't see why we shouldn't match that. | > | > > Sure we can make an exception for Cygwin, but the same program can then fail elsewhere. | > | > I agree that for portability, a program should not assume that #include | > automatically #include and use the latter's | > functions or typedefs. But the bottom line here is that | > requires struct sockaddr_in, so it needs that #include. | | Good point. Pierre? I don't know why the original resolv.h didn't include netinet/in.h and I have no problem adding it in Cygwin, given it was added in Linux. The minires package is nearing its life end, so I would make the change starting with the built-in resolver. Pierre
Re: requires
- Original Message - From: "Yaakov (Cygwin/X)" To: Sent: Monday, December 08, 2008 5:45 PM Subject: requires | -BEGIN PGP SIGNED MESSAGE- | Hash: SHA256 | | This affects both minires-1.02 and Cygwin 1.7.0-34. STC based on a | configure test: ** This is far from the 1st time that this issue comes up. resolv.h is completely standard, it comes from a bind distribution. Every version of man resolver that I have ever seen specifies: SYNOPSIS #include #include #include #include So it's up to the user to include the right files. Sure we can make an exception for Cygwin, but the same program can then fail elsewhere. Pierre
[Patch] Minires update
This patch syncs the built-in minires with the latest packaged version.
Also attaching the files to guarantee format preservation.
Pierre
2008-12-03 Pierre A. Humblet <[EMAIL PROTECTED]>
* libc/minires.c (open_sock): Set non blocking and close on exec.
(res_ninit): Set id pseudo-randomly.
(res_nsend): Do not set close on exec. Initialize server from id.
Flush socket. Tighten rules for answer acceptance.
(res_nmkquery): Update id using current data.
Index: minires.c
===
RCS file: /cvs/src/src/winsup/cygwin/libc/minires.c,v
retrieving revision 1.4
diff -u -p -r1.4 minires.c
--- minires.c 1 Apr 2008 10:22:33 - 1.4
+++ minires.c 3 Dec 2008 02:57:26 -
@@ -1,6 +1,6 @@
/* minires.c. Stub synchronous resolver for Cygwin.
- Copyright 2006 Red Hat, Inc.
+ Copyright 2008 Red Hat, Inc.
Written by Pierre A. Humblet <[EMAIL PROTECTED]>
@@ -225,6 +225,17 @@ static int open_sock(struct sockaddr_in
DPRINTF(debug, "socket(UDP): %s\n", strerror(errno));
return -1;
}
+ /* Set non-blocking */
+ if (fcntl64(fd, F_SETFL, O_NONBLOCK) < 0) {
+DPRINTF(debug, "fcntl: %s\n", strerror(errno));
+return -1;
+ }
+ /* Set close on exec flag */
+ if (fcntl64(fd, F_SETFD, 1) == -1) {
+DPRINTF(debug, "fcntl: %s\n", strerror(errno));
+return -1;
+ }
+
CliAddr->sin_family = AF_INET;
CliAddr->sin_addr.s_addr = htonl(INADDR_ANY);
CliAddr->sin_port = htons(0);
@@ -266,7 +277,10 @@ int res_ninit(res_state statp)
statp->use_os = 1;/* use os_query if available and allowed by
get_resolv */
statp->mypid = -1;
statp->sockfd = -1;
-
+ /* Use the pid and the ppid for random seed, from the point of view of an
outsider.
+ Mix the upper and lower bits as they are not used equally */
+ i = getpid();
+ statp->id = (ushort) (getppid() ^ (i << 8) ^ (i >> 8));
for (i = 0; i < DIM(statp->dnsrch); i++) statp->dnsrch[i] = 0;
/* resolv.conf (dns servers & search list)*/
@@ -420,22 +434,15 @@ int res_nsend( res_state statp, const un
/* Open a socket for this process */
if (statp->sockfd == -1) {
-/* Create a socket and bind it (to any port) */
+/* Create a non-blocking, close on exec socket and bind it (to any port) */
statp->sockfd = open_sock(& mySockAddr, debug);
if (statp->sockfd < 0 ) {
statp->res_h_errno = NETDB_INTERNAL;
return -1;
}
-/* Set close on exec flag */
-if (fcntl64(statp->sockfd, F_SETFD, 1) == -1) {
- DPRINTF(debug, "fcntl: %s\n",
- strerror(errno));
- statp->res_h_errno = NETDB_INTERNAL;
- return -1;
-}
statp->mypid = getpid();
if (SServ == 0X) /* Pseudo random */
- SServ = statp->mypid % statp->nscount;
+ SServ = statp->id % statp->nscount;
}
transNum = 0;
@@ -443,6 +450,26 @@ int res_nsend( res_state statp, const un
if ((wServ = SServ + 1) >= statp->nscount)
wServ = 0;
SServ = wServ;
+
+/* There exists attacks on DNS where many wrong answers with guessed id's
and
+ spoofed source address and port are generated at about the time when the
+ program is tricked into resolving a name.
+ This routine runs through the retry loop for each incorrect answer.
+ It is thus extremely likely that such attacks will cause a TRY_AGAIN
return,
+ probably causing the calling program to retry after a delay.
+
+ Note that valid late or duplicate answers to a previous questions also
cause
+ a retry, although this is minimized by flushing the socket before
sending the
+ new question.
+*/
+
+/* Flush duplicate or late answers */
+while ((rslt = cygwin_recvfrom( statp->sockfd, AnsPtr, AnsLength, 0, NULL,
NULL)) >= 0) {
+ DPRINTF(debug, "Flushed %d bytes\n", rslt);
+}
+DPRINTF(debug && (errno != EWOULDBLOCK),
+ "Unexpected errno for flushing recvfrom: %s", strerror(errno));
+
/* Send the message */
rslt = cygwin_sendto(statp->sockfd, MsgPtr, MsgLength, 0,
(struct sockaddr *) &statp->nsaddr_list[wServ],
@@ -481,59 +508,66 @@ int res_nsend( res_state statp, const un
statp->res_h_errno = NETDB_INTERNAL;
return -1;
}
-/*
- Prepare to retry with tcp
+DPRINTF(debug, "recvfrom: %d bytes from %08x\n", rslt,
dnsSockAddr.sin_addr.s_addr);
+/*
+ Prepare to retry with tcp
*/
for (tcp = 0; tcp < 2; tcp++) {
- /* Check if this is the message we expected */
- if ((*MsgPtr == *AnsPtr) /* Ids match */
- && (*(MsgPtr + 1) == *(AnsPtr + 1))
-/* We have stopped checking this because the question may not be present on
error,
- in particu
[Patch] libc/minires-os-if.c
This brings libc/minires up to minires 1.02,
but using the recently updated w32api/include/windns.h.
Pierre
2007-02-08 Pierre A. Humblet <[EMAIL PROTECTED]>
* libc/minires-os-if.c (write_record): Handle DNS_TYPE_SRV and
some obsolete types.
Index: minires-os-if.c
===
RCS file: /cvs/src/src/winsup/cygwin/libc/minires-os-if.c,v
retrieving revision 1.2
diff -u -p -r1.2 minires-os-if.c
--- minires-os-if.c 15 Dec 2006 09:50:32 - 1.2
+++ minires-os-if.c 9 Feb 2007 02:01:20 -
@@ -120,10 +120,13 @@ static u_char * write_record(unsigned ch
}
break;
case DNS_TYPE_MINFO:
+ case DNS_TYPE_RP:
PUTDOMAIN(rr->Data.MINFO.pNameMailbox, ptr);
PUTDOMAIN(rr->Data.MINFO.pNameErrorsMailbox, ptr);
break;
case DNS_TYPE_MX:
+ case DNS_TYPE_AFSDB:
+ case DNS_TYPE_RT:
if (ptr + 2 > EndPtr)
ptr += 2;
else
@@ -131,7 +134,9 @@ static u_char * write_record(unsigned ch
PUTDOMAIN(rr->Data.MX.pNameExchange, ptr);
break;
case DNS_TYPE_HINFO:
- case DNS_TYPE_TEXT:
+ case DNS_TYPE_ISDN:
+ case DNS_TYPE_TEXT:
+ case DNS_TYPE_X25:
{
unsigned int i, len;
for (i = 0; i < rr->Data.TXT.dwStringCount; i++) {
@@ -146,6 +151,16 @@ static u_char * write_record(unsigned ch
}
break;
}
+ case DNS_TYPE_SRV:
+if (ptr + 6 > EndPtr)
+ ptr += 6;
+else {
+ PUTSHORT(rr->Data.SRV.wPriority, ptr);
+ PUTSHORT(rr->Data.SRV.wWeight, ptr);
+ PUTSHORT(rr->Data.SRV.wPort, ptr);
+}
+PUTDOMAIN(rr->Data.SRV.pNameTarget, ptr);
+break;
default:
{
unsigned int len = rr->wDataLength;
[Patch] minires
2006-12-13 Pierre A. Humblet <[EMAIL PROTECTED]>
* libc/minires-os-if.c (cygwin_query): Remove ERROR_PROC_NOT_FOUND case.
(get_dns_info): Verify DnsQuery exists. Use autoloaded GetNetworkParams.
Index: minires-os-if.c
===
RCS file: /cvs/src/src/winsup/cygwin/libc/minires-os-if.c,v
retrieving revision 1.1
diff -u -p -r1.1 minires-os-if.c
--- minires-os-if.c 11 Dec 2006 19:59:06 - 1.1
+++ minires-os-if.c 15 Dec 2006 03:34:28 -
@@ -196,10 +196,6 @@ static int cygwin_query(res_state statp,
DPRINTF(debug, "DnsQuery: %lu (Windows)\n", res);
if (res) {
switch (res) {
-case ERROR_PROC_NOT_FOUND:
- errno = ENOSYS;
- statp->res_h_errno = NO_RECOVERY;
- break;
case ERROR_INVALID_NAME:
errno = EINVAL;
statp->res_h_errno = NETDB_INTERNAL;;
@@ -393,13 +389,12 @@ void get_dns_info(res_state statp)
DWORD dwRetVal;
IP_ADDR_STRING * pIPAddr;
FIXED_INFO * pFixedInfo;
- HINSTANCE kerneldll;
- typedef DWORD WINAPI (*GNPType)(PFIXED_INFO, PULONG);
- GNPType PGetNetworkParams;
int numAddresses = 0;
- if (statp->use_os) {
-DPRINTF(debug, "using dnsapi.dll\n");
+ if (statp->use_os
+ && ((dwRetVal = DnsQuery_A(NULL, 0, 0, NULL, NULL, NULL)) !=
ERROR_PROC_NOT_FOUND))
+ {
+DPRINTF(debug, "using dnsapi.dll %d\n", dwRetVal);
statp->os_query = (typeof(statp->os_query)) cygwin_query;
/* We just need the search list. Avoid loading iphlpapi. */
statp->nscount = -1;
@@ -408,17 +403,8 @@ void get_dns_info(res_state statp)
if (statp->nscount != 0)
goto use_registry;
- if (!(kerneldll = LoadLibrary("IPHLPAPI.DLL"))) {
-DPRINTF(debug, "LoadLibrary: error %lu (Windows)\n", GetLastError());
-goto use_registry;
- }
- if (!(PGetNetworkParams = (GNPType) GetProcAddress(kerneldll,
-"GetNetworkParams"))) {
-DPRINTF(debug, "GetProcAddress: error %lu (Windows)\n", GetLastError());
-goto use_registry;
- }
/* First call to get the buffer length we need */
- dwRetVal = PGetNetworkParams((FIXED_INFO *) 0, &ulOutBufLen);
+ dwRetVal = GetNetworkParams((FIXED_INFO *) 0, &ulOutBufLen);
if (dwRetVal != ERROR_BUFFER_OVERFLOW) {
DPRINTF(debug, "GetNetworkParams: error %lu (Windows)\n", dwRetVal);
goto use_registry;
@@ -427,7 +413,7 @@ void get_dns_info(res_state statp)
DPRINTF(debug, "alloca: %s\n", strerror(errno));
goto use_registry;
}
- if ((dwRetVal = PGetNetworkParams((FIXED_INFO *) pFixedInfo, &
ulOutBufLen))) {
+ if ((dwRetVal = GetNetworkParams(pFixedInfo, & ulOutBufLen))) {
DPRINTF(debug, "GetNetworkParams: error %lu (Windows)\n", dwRetVal);
goto use_registry;
}
Index: minires-os-if.c
===
RCS file: /cvs/src/src/winsup/cygwin/libc/minires-os-if.c,v
retrieving revision 1.1
diff -u -p -r1.1 minires-os-if.c
--- minires-os-if.c 11 Dec 2006 19:59:06 - 1.1
+++ minires-os-if.c 15 Dec 2006 03:34:22 -
@@ -196,10 +196,6 @@ static int cygwin_query(res_state statp,
DPRINTF(debug, "DnsQuery: %lu (Windows)\n", res);
if (res) {
switch (res) {
-case ERROR_PROC_NOT_FOUND:
- errno = ENOSYS;
- statp->res_h_errno = NO_RECOVERY;
- break;
case ERROR_INVALID_NAME:
errno = EINVAL;
statp->res_h_errno = NETDB_INTERNAL;;
@@ -393,13 +389,12 @@ void get_dns_info(res_state statp)
DWORD dwRetVal;
IP_ADDR_STRING * pIPAddr;
FIXED_INFO * pFixedInfo;
- HINSTANCE kerneldll;
- typedef DWORD WINAPI (*GNPType)(PFIXED_INFO, PULONG);
- GNPType PGetNetworkParams;
int numAddresses = 0;
- if (statp->use_os) {
-DPRINTF(debug, "using dnsapi.dll\n");
+ if (statp->use_os
+ && ((dwRetVal = DnsQuery_A(NULL, 0, 0, NULL, NULL, NULL)) !=
ERROR_PROC_NOT_FOUND))
+ {
+DPRINTF(debug, "using dnsapi.dll %d\n", dwRetVal);
statp->os_query = (typeof(statp->os_query)) cygwin_query;
/* We just need the search list. Avoid loading iphlpapi. */
statp->nscount = -1;
@@ -408,17 +403,8 @@ void get_dns_info(res_state statp)
if (statp->nscount != 0)
goto use_registry;
- if (!(kerneldll = LoadLibrary("IPHLPAPI.DLL"))) {
-DPRINTF(debug, "LoadLibrary: error %lu (Windows)\n", GetLastError());
-goto use_registry;
- }
- if (!(PGetNetworkParams = (GNPType) GetProcAddress(kerneldll,
-"GetNetworkParams"))) {
-DPRINTF(debug, "GetProcAddress: error %lu (Windows)\n", GetLastError());
-goto use_registry;
- }
/* First call to get the buffer length we need */
- dwRetVal = PGetNetworkParams((FIXED_
minires-1.01 for cygwin
Corinna knows what this is about. Pierre http://mysite.verizon.net/phumblet/minires-1.01.tgz
Addition to the testsuite & cygwin patch
The main purpose of this patch is to contribute the attached file to
testsuite/winsup.api. It checks that Cygwin can support a user
supplied version of malloc.
However the patch below is required to make it work and to
support versions of malloc that don't call sbrk.
Pierre
2006-05-09 Pierre Humblet [EMAIL PROTECTED]
* winsup.api/malloc.c: New file
2006-05-09 Pierre Humblet [EMAIL PROTECTED]
* heap.cc (heap_init): Only commit if allocsize is not zero.
Index: heap.cc
===
RCS file: /cvs/src/src/winsup/cygwin/heap.cc,v
retrieving revision 1.52
diff -u -p -b -r1.52 heap.cc
--- heap.cc 13 Mar 2006 21:10:14 - 1.52
+++ heap.cc 9 May 2006 21:47:40 -
@@ -83,7 +83,7 @@ heap_init ()
reserve_size, allocsize, page_const);
if (p != cygheap->user_heap.base)
api_fatal ("heap allocated at wrong address %p (mapped) != %p
(expected)", p, cygheap->user_heap.base);
- if (!VirtualAlloc (cygheap->user_heap.base, allocsize, MEM_COMMIT,
PAGE_READWRITE))
+ if (allocsize && !VirtualAlloc (cygheap->user_heap.base, allocsize,
MEM_COMMIT, PAGE_READWRITE))
api_fatal ("MEM_COMMIT failed, %E");
}
malloc.c
Description: Binary data
Re: [Patch] Make getenv() functional before the environment is initialized
- Original Message - From: "Christopher Faylor" To: Sent: Friday, April 21, 2006 5:39 PM Subject: Re: [Patch] Make getenv() functional before the environment is initialized I just talked to Corinna about this on IRC and neither of us really cares enough about this to merit a long discussion so I've just checked in a variation of the cmalloc patch. The only change that I made was to define a HEAP_2_STR value so that the HEAP_1_MAX usage is confined to cygheap.cc where I'd intended it. Thanks a lot, Chris & Corinna. Now that I am trying it, it doesn't work anymore when launched from Cygwin. I am starting to wonder if the current *ptr[len] == '=' is equivalent to the former *(*ptr + s) == '=' when s = len and ptr is a char ** Pierre
Re: [Patch] Make getenv() functional before the environment is initialized
- Original Message - From: "Christopher Faylor" <[EMAIL PROTECTED]> To: Sent: Friday, April 21, 2006 4:12 PM Subject: Re: [Patch] Make getenv() functional before the environment is initialized But doesn't the program then have a pointer to memory that has been freed? That pointer can also be accessed after forks. Isn't that always a possibility? You can't rely on the persistence of the stuff returned from getenv(). That's not my reading of http://www.opengroup.org/onlinepubs/95399/functions/getenv.html "The string pointed to may be overwritten by a subsequent call to getenv(), but shall not be overwritten by a call to any other function in this volume of IEEE Std 1003.1-2001." Athough Posix allows the string to be overwritten, indicating that persistence is implied, it does not allow the pointer to become invalid. See also http://developer.apple.com/documentation/Darwin/Reference/Manpages/man3/getenv.3.html which says that the environment semantics make it inherently leaky. That's why I didn't hesitate calling cmalloc Pierre
Re: [Patch] Make getenv() functional before the environment is initialized
- Original Message - From: "Christopher Faylor" <[EMAIL PROTECTED]> To: Sent: Friday, April 21, 2006 3:13 PM Subject: Re: [Patch] Make getenv() functional before the environment is initialized On Fri, Apr 21, 2006 at 02:52:06PM -0400, Pierre A. Humblet wrote: In particular GetEnvironmentStrings returns a big block of storage that should be free (which we can't do), and that is going to be lost on a fork, potentially leading to trouble. Thus I have another implementation using GetEnvironmentValue and cmalloc. (with HEAP_1_MAX, so that it will be released on the next exec). I also take advantage of spawn_info, whose existence I had forgotten. Overall it's also simpler. Here is another patch, sorry for not sending this earlier. I don't see any reason to permanently allocate memory with cmalloc. I think that using GetEnvironmentStrings is still the right choice here. You just have to make sure that it gets freed. I'm going to check in a cleanup of getearly which will move the rawenv variable to a static which will potentially be used by environ_init. Then environ_init will free it if it has been previously set. But doesn't the program then have a pointer to memory that has been freed? That pointer can also be accessed after forks. Pierre
Re: [Patch] Make getenv() functional before the environment is initialized
- Original Message -
From: "Corinna Vinschen"
To:
Sent: Friday, April 21, 2006 1:23 PM
Subject: Re: [Patch] Make getenv() functional before the environment is
initialized
On Apr 6 12:35, Pierre A. Humblet wrote:
* environ.cc (getearly): New function.
(getenv) : Call getearly if needed.
Thanks for the patch and sorry for the lng delay. I've applied a
slightly tweaked version of your patch, which uses a function pointer in
getenv, instead of adding a conditional.
Corinna,
Thanks! Since sending the patch, I have found some issues with it :(
In particular GetEnvironmentStrings returns a big block of
storage that should be free (which we can't do), and that is
going to be lost on a fork, potentially leading to trouble.
Thus I have another implementation using GetEnvironmentValue
and cmalloc. (with HEAP_1_MAX, so that it will be released
on the next exec).
I also take advantage of spawn_info, whose existence I had forgotten.
Overall it's also simpler.
Here is another patch, sorry for not sending this earlier.
Pierre
2006-04-21 Pierre Humblet [EMAIL PROTECTED]
* environ.cc (getearly): Use GetEnvironmentVariable and cmalloc
instead of GetEnvironmentStrings.
Index: environ.cc
===
RCS file: /cvs/src/src/winsup/cygwin/environ.cc,v
retrieving revision 1.140
diff -u -p -b -r1.140 environ.cc
--- environ.cc 21 Apr 2006 17:21:41 - 1.140
+++ environ.cc 21 Apr 2006 18:37:55 -
@@ -231,28 +231,21 @@ my_findenv (const char *name, int *offse
static char * __stdcall
getearly (const char * name, int *offset __attribute__ ((unused)))
{
- int s = strlen (name);
- char * rawenv;
- char ** ptr;
- child_info *get_cygwin_startup_info ();
- child_info_spawn *ci = (child_info_spawn *) get_cygwin_startup_info ();
+ int s;
+ char ** ptr, * ret;
- if (ci && (ptr = ci->moreinfo->envp))
+ if (spawn_info && (ptr = spawn_info->moreinfo->envp))
{
+ s = strlen (name);
for (; *ptr; ptr++)
if (strncasematch (name, *ptr, s)
&& (*(*ptr + s) == '='))
return *ptr + s + 1;
}
- else if ((rawenv = GetEnvironmentStrings ()))
-{
- while (*rawenv)
- if (strncasematch (name, rawenv, s)
- && (*(rawenv + s) == '='))
- return rawenv + s + 1;
- else
- rawenv = strchr (rawenv, 0) + 1;
-}
+ else if ((s = GetEnvironmentVariable (name, NULL, 0))
+ && (ret = (char *) cmalloc (HEAP_1_MAX, s))
+ && GetEnvironmentVariable (name, ret, s))
+return ret;
return NULL;
}
[Patch] Make getenv() functional before the environment is initialized
This makes getenv return sensibly before the environment is initialized.
The attached file should be properly formatted (Changelog & patch),
which my mailer can't do.
Pierre
2006-04-06 Pierre Humblet [EMAIL PROTECTED]
* environ.cc (getearly): New function.
(getenv) : Call getearly if needed.
Index: environ.cc
===
RCS file: /cvs/src/src/winsup/cygwin/environ.cc,v
retrieving revision 1.139
diff -u -p -b -r1.139 environ.cc
--- environ.cc 22 Mar 2006 16:42:44 - 1.139
+++ environ.cc 6 Apr 2006 16:06:05 -
@@ -224,6 +224,39 @@ my_findenv (const char *name, int *offse
}
/*
+ * getearly --
+ * Primitive getenv before the environment is built.
+ */
+
+static char *
+getearly (const char * name)
+{
+ int s = strlen (name);
+ char * rawenv;
+ char ** ptr;
+ child_info *get_cygwin_startup_info ();
+ child_info_spawn *ci = (child_info_spawn *) get_cygwin_startup_info ();
+
+ if (ci && (ptr = ci->moreinfo->envp))
+{
+ for (; *ptr; ptr++)
+ if (strncasematch (name, *ptr, s)
+ && (*(*ptr + s) == '='))
+ return *ptr + s + 1;
+}
+ else if ((rawenv = GetEnvironmentStrings ()))
+{
+ while (*rawenv)
+ if (strncasematch (name, rawenv, s)
+ && (*(rawenv + s) == '='))
+ return rawenv + s + 1;
+ else
+ rawenv = strchr (rawenv, 0) + 1;
+}
+ return NULL;
+}
+
+/*
* getenv --
* Returns ptr to value associated with name, if any, else NULL.
*/
@@ -232,7 +265,8 @@ extern "C" char *
getenv (const char *name)
{
int offset;
-
+ if (!__cygwin_environ)
+return getearly (name);
return my_findenv (name, &offset);
}
environ.diff
Description: Binary data
Patch to dcrt0.cc for dmalloc
2006-04-06 Pierre Humblet <[EMAIL PROTECTED]>
* drct0.cc (dll_crt0_1): Move malloc_init after
user_data->resourcelocks->Init.
diff -u -p -r1.303 dcrt0.cc
--- dcrt0.cc3 Apr 2006 17:33:07 - 1.303
+++ dcrt0.cc5 Apr 2006 16:07:53 -
@@ -784,7 +784,6 @@ static void
dll_crt0_1 (char *)
{
check_sanity_and_sync (user_data);
- malloc_init ();
#ifdef CGF
int i = 0;
const int n = 2 * 1024 * 1024;
@@ -794,6 +793,7 @@ dll_crt0_1 (char *)
user_data->resourcelocks->Init ();
user_data->threadinterface->Init ();
+ malloc_init ();
ProtectHandle (hMainProc);
ProtectHandle (hMainThread);
Re: [PATCH]: cygwin_internal
The situation is that exim has the concept that some groups are privileged and can have write access to the configuration file. They are normally initialized to hard values set at compile time. The Cygwin port of exim fakes things up so that the gid of Admins (obtained from cygwin_internal) is put in the list of exim's privileged groups. The problem is that the gid obtained by cygwin_internal (from the Admins sid) may not match the gid reported by stat, which is obtained by cygpsid::get_id () from the same Admins sid. Pierre - Original Message - From: "Pierre A. Humblet" <[EMAIL PROTECTED]> To: Sent: Friday, July 01, 2005 9:36 AM Subject: Re: [PATCH]: cygwin_internal > The situation is that exim has the concept that some groups > are privileged and can have write access to the configuration file. > They are normally initialized to hard values set at compile time. > > The Cygwin port of exim fakes things up so that the gid of Admins > (obtained from cygwin_internal) is put in the list of exim's privileged > groups. > The problem is that the gid obtained by cygwin_internal (from the > Admins sid) may not match the gid reported by stat, which is obtained by > cygpsid::get_id () from the same Admins sid. > > Pierre > > - Original Message - > From: "Corinna Vinschen" <[EMAIL PROTECTED]> > To: > Sent: Friday, July 01, 2005 4:42 AM > Subject: Re: [PATCH]: cygwin_internal > > > > On Jul 1 00:52, Pierre A. Humblet wrote: > > > > > > The patch below uses cygpsid::get_id to implement CW_GET_UID_FROM_SID > > > and CW_GET_GID_FROM_SID in cygwin_internal. Thus the sid is first > > > compared to the user (or primary group) sid, before looking up > > > the passwd (or group) file. > > > > > > This can make a difference when a sid appears multiple times in the > > > passwd or group file, e.g. when one has both 544 and 0. > > > This difference can cause exim (and perhaps others) to fail (it did > > > happen to me). > > > > Can you please describe the exact situation? I think I see it, but I > > want to be really sure. I'm not keen on accidentally breaking Cygserver's > > authentication routine. > > > > > > Corinna > > > > -- > > Corinna Vinschen Please, send mails regarding Cygwin to > > Cygwin Project Co-Leader mailto:cygwin@cygwin.com > > Red Hat, Inc. >
[PATCH]: cygwin_internal
The patch below uses cygpsid::get_id to implement CW_GET_UID_FROM_SID
and CW_GET_GID_FROM_SID in cygwin_internal. Thus the sid is first
compared to the user (or primary group) sid, before looking up
the passwd (or group) file.
This can make a difference when a sid appears multiple times in the
passwd or group file, e.g. when one has both 544 and 0.
This difference can cause exim (and perhaps others) to fail (it did
happen to me).
The patch also removes an obsolete member of the cygheap.
Pierre
2005-07-01 Pierre Humblet <[EMAIL PROTECTED]>
* cygheap.h (struct init_cygheap): Delete cygwin_regname member.
* external.cc (cygwin_internal): Use cygpsid::get_id for
CW_GET_UID_FROM_SID and CW_GET_GID_FROM_SID.
Turn CW_SET_CYGWIN_REGISTRY_NAME and CW_GET_CYGWIN_REGISTRY_NAME
into noops.
Index: cygheap.h
===
RCS file: /cvs/src/src/winsup/cygwin/cygheap.h,v
retrieving revision 1.105
diff -u -p -r1.105 cygheap.h
--- cygheap.h 1 Jun 2005 03:46:55 - 1.105
+++ cygheap.h 1 Jul 2005 04:38:56 -
@@ -278,7 +278,6 @@ struct init_cygheap
HANDLE shared_h;
HANDLE console_h;
HANDLE mt_h;
- char *cygwin_regname;
cwdstuff cwd;
dtable fdtab;
LUID luid[SE_NUM_PRIVS];
Index: external.cc
===
RCS file: /cvs/src/src/winsup/cygwin/external.cc,v
retrieving revision 1.75
diff -u -p -r1.75 external.cc
--- external.cc 19 May 2005 01:25:19 - 1.75
+++ external.cc 1 Jul 2005 04:40:36 -
@@ -197,16 +197,8 @@ cygwin_internal (cygwin_getinfo_types t,
}
case CW_SET_CYGWIN_REGISTRY_NAME:
- {
- const char *cr = va_arg (arg, char *);
- if (check_null_empty_str_errno (cr))
- return (DWORD) NULL;
- cygheap->cygwin_regname = (char *) crealloc (cygheap->cygwin_regname,
- strlen (cr) + 1);
- strcpy (cygheap->cygwin_regname, cr);
- }
case CW_GET_CYGWIN_REGISTRY_NAME:
- return (DWORD) cygheap->cygwin_regname;
+ return 0;
case CW_STRACE_TOGGLE:
{
@@ -280,17 +272,13 @@ cygwin_internal (cygwin_getinfo_types t,
}
case CW_GET_UID_FROM_SID:
{
- PSID psid = va_arg (arg, PSID);
- cygsid sid (psid);
- struct passwd *pw = internal_getpwsid (sid);
- return pw ? pw->pw_uid : (__uid32_t)-1;
+ cygpsid psid = va_arg (arg, PSID);
+ return psid.get_id (false, NULL);
}
case CW_GET_GID_FROM_SID:
{
- PSID psid = va_arg (arg, PSID);
- cygsid sid (psid);
- struct __group32 *gr = internal_getgrsid (sid);
- return gr ? gr->gr_gid : (__gid32_t)-1;
+ cygpsid psid = va_arg (arg, PSID);
+ return psid.get_id (true, NULL);
}
case CW_GET_BINMODE:
{
Re: [Patch]: mkdir -p and network drives
- Original Message -
From: "Corinna Vinschen"
To:
Sent: Wednesday, May 18, 2005 12:48 PM
Subject: Re: [Patch]: mkdir -p and network drives
> Hi Pierre,
>
> I don't see a reason why you moved telldir just a few lines up.
> Any reasoning, perhaps together with a ChangeLog entry?
Nope, it was an accidental cut and I pasted it back a few lines off.
>
> Why did you remove fhandler_cygdrive::telldir but not
> fhandler_cygdrive::seekdir? Both are just calling their base class
> variants.
I am still working on fhandler_cygdrive. I stopped to keep the size
of the patch small.
> > - else if (isvirtual_dev (dev.devn) && fileattr ==
INVALID_FILE_ATTRIBUTES)
> > -{
> > - error = dev.devn == FH_NETDRIVE ? ENOSHARE : ENOENT;
> > - return;
> > -}
>
> I don't understand this one. What's the rational behind removing
> these lines?
- They won't work the day we support writing to the registry.
- More generally, I think it's cleaner to do device specific error handling
in the fhandlers, instead of adding conditionals in path.cc
- In the case where one tries to create a file or directory on a virtual
device,
one gets EROFS with this patch, instead of ENOSHARE or ENOENT before.
That seems more logical.
Pierre
Re: [Patch]: mkdir -p and network drives
Here is the implementation of mkdir and rmdir with fhandlers.
To prepare the day where proc_registry will allow writes,
I have removed setting PATH_RO and an error return from path.cc
(it's all handled in the fhandlers).
I have also removed obsolete code about fhandler_cygdrive.
There is another patch coming with minor corner case fixes.
Pierre
2005-05-18 Pierre Humblet <[EMAIL PROTECTED]>
* devices.h: Delete FH_CYGDRIVE_A and FH_CYGDRIVE_Z.
* fhandler.h (fhandler_base::mkdir): New virtual method.
(fhandler_base::rmdir): Ditto.
(fhandler_disk_file:mkdir): New method.
(fhandler_disk_file:rmdir): Ditto.
(fhandler_cygdrive::iscygdrive_root): Delete method.
(fhandler_cygdrive::telldir): Delete declaration.
* dir.cc (mkdir): Implement with fhandlers.
(rmdir): Ditto.
* fhandler.cc (fhandler_base::mkdir): New virtual method.
(fhandler_base::rmdir): Ditto.
* fhandler_disk_file.cc: Remove all uses of
fhandler_cygdrive::iscygdrive_root.
(fhandler_disk_file::mkdir): New method.
(fhandler_disk_file::rmdir): Ditto.
(fhandler_cygdrive::telldir): Delete.
* path.cc (path_conv::check): For virtual devices, do not set PATH_RO
and
do not set error in case of non-existence.Index: devices.h
===
RCS file: /cvs/src/src/winsup/cygwin/devices.h,v
retrieving revision 1.18
diff -u -p -r1.18 devices.h
--- devices.h 10 May 2005 20:56:06 - 1.18
+++ devices.h 18 May 2005 04:22:41 -
@@ -106,8 +106,6 @@ enum fh_devices
DEV_CYGDRIVE_MAJOR = 98,
FH_CYGDRIVE= FHDEV (DEV_CYGDRIVE_MAJOR, 0),
- FH_CYGDRIVE_A= FHDEV (DEV_CYGDRIVE_MAJOR, 'a'),
- FH_CYGDRIVE_Z= FHDEV (DEV_CYGDRIVE_MAJOR, 'z'),
DEV_TCP_MAJOR = 30,
FH_TCP = FHDEV (DEV_TCP_MAJOR, 36),
Index: fhandler.h
===
RCS file: /cvs/src/src/winsup/cygwin/fhandler.h,v
retrieving revision 1.247
diff -u -p -r1.247 fhandler.h
--- fhandler.h 17 May 2005 20:34:15 - 1.247
+++ fhandler.h 18 May 2005 04:22:42 -
@@ -346,6 +346,8 @@ class fhandler_base
void operator delete (void *);
virtual HANDLE get_guard () const {return NULL;}
virtual void set_eof () {}
+ virtual int mkdir (mode_t mode);
+ virtual int rmdir ();
virtual DIR *opendir ();
virtual dirent *readdir (DIR *);
virtual _off64_t telldir (DIR *);
@@ -664,6 +666,8 @@ class fhandler_disk_file: public fhandle
int msync (HANDLE h, caddr_t addr, size_t len, int flags);
bool fixup_mmap_after_fork (HANDLE h, DWORD access, int flags,
_off64_t offset, DWORD size, void *address);
+ int mkdir (mode_t mode);
+ int rmdir ();
DIR *opendir ();
struct dirent *readdir (DIR *);
_off64_t telldir (DIR *);
@@ -678,11 +682,9 @@ class fhandler_cygdrive: public fhandler
const char *pdrive;
void set_drives ();
public:
- bool iscygdrive_root () { return !dev ().minor; }
fhandler_cygdrive ();
DIR *opendir ();
struct dirent *readdir (DIR *);
- _off64_t telldir (DIR *);
void seekdir (DIR *, _off64_t);
void rewinddir (DIR *);
int closedir (DIR *);
Index: dir.cc
===
RCS file: /cvs/src/src/winsup/cygwin/dir.cc,v
retrieving revision 1.86
diff -u -p -r1.86 dir.cc
--- dir.cc 13 May 2005 15:46:05 - 1.86
+++ dir.cc 18 May 2005 04:22:42 -
@@ -221,39 +221,21 @@ extern "C" int
mkdir (const char *dir, mode_t mode)
{
int res = -1;
- SECURITY_ATTRIBUTES sa = sec_none_nih;
- security_descriptor sd;
+ fhandler_base *fh = NULL;
- path_conv real_dir (dir, PC_SYM_NOFOLLOW | PC_WRITABLE);
+ if (!(fh = build_fh_name (dir, NULL, PC_SYM_NOFOLLOW | PC_WRITABLE)))
+goto done; /* errno already set */;
- if (real_dir.error)
+ if (fh->error ())
{
- set_errno (real_dir.case_clash ? ECASECLASH : real_dir.error);
- goto done;
+ debug_printf ("got %d error from build_fh_name", fh->error ());
+ set_errno (fh->error ());
}
+ else if (!fh->mkdir (mode))
+res = 0;
+ delete fh;
- nofinalslash (real_dir.get_win32 (), real_dir.get_win32 ());
-
- if (allow_ntsec && real_dir.has_acls ())
-set_security_attribute (S_IFDIR | ((mode & 0) & ~cygheap->umask),
- &sa, sd);
-
- if (CreateDirectoryA (real_dir.get_win32 (), &sa))
-{
- if (!allow_ntsec && allow_ntea)
- set_file_attribute (false, NULL, real_dir.get_win32 (),
- S_IFDIR | ((mode & 0) & ~cygheap->umask));
-#ifdef HIDDEN_DOT_FILES
- char *c = strrchr (real_dir.get_win32 (), '\\');
- if ((c && c[1] == '.') || *real_dir.get_win32 () == '.')
- SetFileAttributes (real_dir.get_win32 (), FILE_ATTRIBUTE_HIDDEN);
-#endif
- res = 0;
-}
- else
-__seterrno ();
-
-done:
+ done:
syscall_printf ("%d = mkdir (%s, %d)",
Re: [Patch]: mkdir -p and network drives
- Original Message - From: "Corinna Vinschen" <[EMAIL PROTECTED]> To: Sent: Wednesday, May 11, 2005 4:53 AM Subject: Re: [Patch]: mkdir -p and network drives > I don't like the idea of isrofs being an inline function in dir.cc. > Wouldn't that be better a method in path_conv? It would be helpful > for other functions, too. For instance, unlink and symlink_worker. > In the (not so) long run we should really move all of these functions > into the fhandlers, though. After looking into it, moving mkdir and rmdir to fhandlers should be quite simple. I will do that early next week. Pierre
Re: [Patch]: mkdir -p and network drives
At 01:27 AM 5/11/2005 +, Eric Blake wrote:
>> At 11:11 AM 5/10/2005 -0400, Christopher Faylor wrote:
>>
>> So I restrained mkdir to only act on FH_FS.
>>
>> Ideally mkdir & rmdir should be part of the various handlers, but
>> there is no current payoff in doing so as directories can only be
>> created/deleted on FH_FS
>
>Would it ever be possible (or desirable) to let /proc/registry be writable
as a means of creating registry keys (mkdir) and values (creat)?
Yes IMO, and that day we will need to distribute {mk, rm}dir to the handlers.
There doesn't seem to be much demand for that however, people seem happy with
regtool.
Pierre
Re: [Patch]: mkdir -p and network drives
At 11:11 AM 5/10/2005 -0400, Christopher Faylor wrote:
>Could we see this as a unified-diff please?
Oops, but in retrospect it's a good thing. I did some more tests.
If c:\dev exists, then mkdir /dev/tty created c:\dev\tty (ditto
for the other /dev/xxx ), but rmdir /dev/tty would not delete
c:\dev\tty
mkdir /cygdrive created c:\cygwin\cygdrive
So I restrained mkdir to only act on FH_FS.
Ideally mkdir & rmdir should be part of the various handlers, but
there is no current payoff in doing so as directories can only be
created/deleted on FH_FS
>On Mon, May 09, 2005 at 08:16:36PM -0400, Pierre A. Humblet wrote:
>>At 06:19 PM 5/9/2005 +, Eric Blake wrote:
>>
>>>Second, the sequence chdir("//"), mkdir("machine") creates machine in the
>>>current directory.
>>
>>Old bug.
>>chdir("/proc"), mkdir("machine") produces the same result.
>>And mkdir("/proc"), mkdir("/proc/machine") creates c:\proc\machine
>>
>>The fix sets errno to EROFS, which is what rmdir is already doing.
>>Is that OK for coreutils?
Pierre
2005-05-11 Pierre Humblet <[EMAIL PROTECTED]>
* dir.cc (isrofs): New function.
(mkdir): Check for FH_FS and use isrofs.
(rmdir): Use isrofs.
Index: dir.cc
===
RCS file: /cvs/src/src/winsup/cygwin/dir.cc,v
retrieving revision 1.84
diff -u -p -r1.84 dir.cc
--- dir.cc 16 Mar 2005 21:20:56 - 1.84
+++ dir.cc 11 May 2005 00:38:11 -
@@ -216,6 +216,13 @@ closedir (DIR *dir)
return res;
}
+inline bool
+isrofs(DWORD devn)
+{
+ return devn == FH_PROC || devn == FH_REGISTRY
+|| devn == FH_PROCESS || devn == FH_NETDRIVE;
+}
+
/* mkdir: POSIX 5.4.1.1 */
extern "C" int
mkdir (const char *dir, mode_t mode)
@@ -231,6 +238,14 @@ mkdir (const char *dir, mode_t mode)
set_errno (real_dir.case_clash ? ECASECLASH : real_dir.error);
goto done;
}
+ if (real_dir.get_devn () != FH_FS)
+{
+ if (isrofs (real_dir.get_devn ()))
+ set_errno (EROFS);
+ else
+ set_errno (EEXIST);
+ goto done;
+}
nofinalslash (real_dir.get_win32 (), real_dir.get_win32 ());
@@ -263,14 +278,12 @@ extern "C" int
rmdir (const char *dir)
{
int res = -1;
- DWORD devn;
path_conv real_dir (dir, PC_SYM_NOFOLLOW | PC_FULL);
if (real_dir.error)
set_errno (real_dir.error);
- else if ((devn = real_dir.get_devn ()) == FH_PROC || devn == FH_REGISTRY
- || devn == FH_PROCESS)
+ else if (isrofs (real_dir.get_devn ()))
set_errno (EROFS);
else if (!real_dir.exists ())
set_errno (ENOENT);
Re: [Patch]: mkdir -p and network drives
At 06:19 PM 5/9/2005 +, Eric Blake wrote:
>Second, the sequence chdir("//"), mkdir("machine") creates machine in the
>current directory.
Old bug.
chdir("/proc"), mkdir("machine") produces the same result.
And mkdir("/proc"), mkdir("/proc/machine") creates c:\proc\machine
The fix sets errno to EROFS, which is what rmdir is already doing.
Is that OK for coreutils?
Pierre
2005-05-10 Pierre Humblet <[EMAIL PROTECTED]>
* dir.cc (isrofs): New function.
(mkdir): Use isrofs.
(rmdir): Ditto.
Index: dir.cc
===
RCS file: /cvs/src/src/winsup/cygwin/dir.cc,v
retrieving revision 1.84
diff -r1.84 dir.cc
218a219,225
> inline bool
> isrofs(DWORD devn)
> {
> return devn == FH_PROC || devn == FH_REGISTRY
> || devn == FH_PROCESS || devn == FH_NETDRIVE;
> }
>
233a241,245
> else if (isrofs (real_dir.get_devn ()))
> {
> set_errno (EROFS);
> goto done;
> }
266d277
< DWORD devn;
272,273c283
< else if ((devn = real_dir.get_devn ()) == FH_PROC || devn == FH_REGISTRY
< || devn == FH_PROCESS)
---
> else if (isrofs (real_dir.get_devn ()))
Re: [Patch]: mkdir -p and network drives
- Original Message -
From: "Eric Blake"
To:
Sent: Monday, May 09, 2005 2:19 PM
Subject: Re: [Patch]: mkdir -p and network drives
> Pierre A. Humblet phumblet.no-ip.org> writes:
> >
> > Here is a patch to allow mkdir -p to easily work with network
> > drives and to allow future enumeration of computers and of
> > network drives by ls -l.
> >
> > It works by defining a new FH_NETDRIVE virtual handler for
> > names such as // and //machine.
> > This also makes chdir work without additional change.
>
> I've just downloaded the 20050508 snapshot to play with this, and it still
> needs some work before coreutils-5.3.0-6 can be released. But it is an
> improvement!
>
> First, `ls -ld // //machine' show that these directories are mode 111
> (searchable, but not readable). Yet opendir("//") and
opendir("//machine")
> succeed, although POSIX requires that opendir(2) fail with EACCESS if the
> directory to be opened is not readable.
That's currently a feature. Being compliant means writing extra code
that will be junked when we make the directories readable.
In what way does non-compliance affect coreutils or the user?
A similar case is that getfacl reports the directories as r-x.
> Second, the sequence chdir("//"), mkdir("machine") creates machine in the
> current directory.
That's a bug. I will look into it. Thanks.
Pierre
Re: [Patch]: mkdir -p and network drives
- Original Message - From: "Christopher Faylor" <[EMAIL PROTECTED]> To: Sent: Friday, May 06, 2005 10:58 AM Subject: Re: [Patch]: mkdir -p and network drives > On Fri, May 06, 2005 at 10:55:29AM -0400, Pierre A. Humblet wrote: > >- Original Message - > >From: "Christopher Faylor" <[EMAIL PROTECTED]> > >To: > >Sent: Friday, May 06, 2005 10:22 AM > >Subject: Re: [Patch]: mkdir -p and network drives > > > >>Well, that was kinda my point. If we can't remove the "//" handling > >>because it breaks bash then adding opendir/readdir stuff seems > >>premature except for the case of ls //foo which is entirely different > >>from ls //. > > > >Sigh. We need a bash maintainer. We need to have // working for mkdir > >-p to work, from what I understand of the code snippet that was sent to > >the list. > > I thought that Eric Blake implied that // *had* to be translated to /, > as per POSIX. I wonder how many programs out there translate a > standalone '//' to '/'. That's not Paul Eggert's position, http://cygwin.com/ml/cygwin/2005-05/msg00251.html I don't expect problems with //, we had it working in cvs for a while and only bash had issues. Program translating // to / should already have problems and they won't be affected if Cygwin keeps // Pierre
Re: [Patch]: mkdir -p and network drives
- Original Message - From: "Christopher Faylor" <[EMAIL PROTECTED]> To: Sent: Friday, May 06, 2005 10:22 AM Subject: Re: [Patch]: mkdir -p and network drives > Well, that was kinda my point. If we can't remove the "//" handling because > it breaks bash then adding opendir/readdir stuff seems premature except for > the case of ls //foo which is entirely different from ls //. Sigh. We need a bash maintainer. We need to have // working for mkdir -p to work, from what I understand of the code snippet that was sent to the list. Pierre
Re: [Patch]: mkdir -p and network drives
cgf wrote: > On Thu, May 05, 2005 at 10:57:08PM -0400, Pierre A. Humblet wrote: >>The code should handle "//" correctly, but path.cc still transforms it >>into "/", because of the bash bug. > Is that fixed in the current bash? AFAIK Corinna fixed it once, but the patch got lost and it's currently not fixed. > So, I'd appreciate it if you would just move your fhandler_netdrive > stuff to fhandler_netdrive.cc. Sure. Thanks for setting up the framework. > I didn't renumber FH_FS with above change. I wasn't sure why you did > that. I don't think that there was a requirement that it has to be the > lowest numbered minor device number. If there is a requirement like > that we should change it. OK. No requirement, just aesthetic. There seemed to be a pattern. >>About implementing readdir: PTC... > I was thinking about doing this but how would it ever be invoked? With "ls -l //" or "ls -l //machine" > You can't do an opendir on "//", right? Sure you can (thanks to existing code in the virtual driver). Just remove the code in path.cc that changes "//" into "/". It's only/mainly there because of bash. Pierre
[Patch]: mkdir -p and network drives
Here is a patch to allow mkdir -p to easily work with network
drives and to allow future enumeration of computers and of
network drives by ls -l.
It works by defining a new FH_NETDRIVE virtual handler for
names such as // and //machine.
This also makes chdir work without additional change.
The code for the new handler is currently in fhandler_virtual.cc, for
simplicity (not an expert on Makefile and fomit-frame-pointer).
It should eventually be placed in fhandler_netdrive.cc
The code should handle "//" correctly, but path.cc still transforms it
into "/", because of the bash bug.
I have directly edited devices.cc instead of using the devices.in
magic.
About implementing readdir: PTC...
Pierre
2005-05-05 Pierre Humblet <[EMAIL PROTECTED]>
* fhandler.h (class fhandler_netdrive): New class.
* fhandler_virtual.cc (fhandler_netdrive::fhandler_netdrive): New
constructor.
(fhandler_netdrive::exists): New method.
(fhandler_netdrive::fstat): Ditto.
(fhandler_netdrive::readdir): Ditto.
(fhandler_netdrive::open): Ditto.
(fhandler_netdrive::fill_filebuf): Ditto.
* dtable.cc (build_fh_pc): Handle case FH_NETDRIVE.
* path.cc (isvirtual_dev): Add FH_NETDRIVE.
(mount_info::conv_to_win32_path): Detect netdrive device.
* devices.h (enum fh_devices): Add FH_NETDRIVE and renumber FH_FS.
Declare dev_netdrive_storage and define netdrive_dev.
* devices.cc: Define dev_netdrive_storage.Index: fhandler.h
===
RCS file: /cvs/src/src/winsup/cygwin/fhandler.h,v
retrieving revision 1.243
diff -u -p -r1.243 fhandler.h
--- fhandler.h 22 Apr 2005 17:03:37 - 1.243
+++ fhandler.h 6 May 2005 02:42:37 -
@@ -1203,6 +1203,17 @@ class fhandler_proc: public fhandler_vir
bool fill_filebuf ();
};
+class fhandler_netdrive: public fhandler_virtual
+{
+ public:
+ fhandler_netdrive ();
+ int exists();
+ struct dirent *readdir (DIR *);
+ int open (int flags, mode_t mode = 0);
+ int __stdcall fstat (struct __stat64 *buf) __attribute__ ((regparm (2)));
+ bool fill_filebuf ();
+};
+
class fhandler_registry: public fhandler_proc
{
private:
Index: fhandler_virtual.cc
===
RCS file: /cvs/src/src/winsup/cygwin/fhandler_virtual.cc,v
retrieving revision 1.33
diff -u -p -r1.33 fhandler_virtual.cc
--- fhandler_virtual.cc 16 Mar 2005 21:20:56 - 1.33
+++ fhandler_virtual.cc 6 May 2005 02:42:37 -
@@ -258,3 +258,75 @@ fhandler_virtual::facl (int cmd, int nen
}
return res;
}
+
+
+/* FIXME: put in fhandler_netdrive.cc */
+
+
+/* Returns 0 if path doesn't exist, >0 if path is a directory,
+ -1 if path is a file, -2 if it's a symlink. */
+int
+fhandler_netdrive::exists ()
+{
+ return 1;
+}
+
+fhandler_netdrive::fhandler_netdrive ():
+ fhandler_virtual ()
+{
+}
+
+int
+fhandler_netdrive::fstat (struct __stat64 *buf)
+{
+ const char *path = get_name ();
+ debug_printf ("fstat (%s)", path);
+
+ (void) fhandler_base::fstat (buf);
+
+ buf->st_mode = S_IFDIR | S_IXUSR | S_IXGRP | S_IXOTH;
+
+ return 0;
+}
+
+struct dirent *
+fhandler_netdrive::readdir (DIR * dir)
+{
+ return NULL;
+}
+
+int
+fhandler_netdrive::open (int flags, mode_t mode)
+{
+ int res = fhandler_virtual::open (flags, mode);
+ if (!res)
+goto out;
+
+ nohandle (true);
+
+ if ((flags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
+{
+ set_errno (EEXIST);
+ res = 0;
+ goto out;
+}
+ else if (flags & O_WRONLY)
+{
+ set_errno (EISDIR);
+ res = 0;
+ goto out;
+}
+
+ res = 1;
+ set_flags ((flags & ~O_TEXT) | O_BINARY | O_DIROPEN);
+ set_open_status ();
+out:
+ syscall_printf ("%d = fhandler_netdrive::open (%p, %d)", res, flags, mode);
+ return res;
+}
+
+bool
+fhandler_netdrive::fill_filebuf ()
+{
+ return false;
+}
Index: dtable.cc
===
RCS file: /cvs/src/src/winsup/cygwin/dtable.cc,v
retrieving revision 1.149
diff -u -p -r1.149 dtable.cc
--- dtable.cc 5 Apr 2005 04:30:58 - 1.149
+++ dtable.cc 6 May 2005 02:42:38 -
@@ -454,6 +454,9 @@ build_fh_pc (path_conv& pc)
case FH_PROCESS:
fh = cnew (fhandler_process) ();
break;
+ case FH_NETDRIVE:
+ fh = cnew (fhandler_netdrive) ();
+ break;
case FH_TTY:
{
if (myself->ctty == TTY_CONSOLE)
Index: path.cc
===
RCS file: /cvs/src/src/winsup/cygwin/path.cc,v
retrieving revision 1.367
diff -u -p -r1.367 path.cc
--- path.cc 2 May 2005 03:50:07 - 1.367
+++ path.cc 6 May 2005 02:42:39 -
@@ -154,7 +154,8 @@ struct win_shortcut_hdr
(path_prefix_p (proc, (path), proc_len))
#define isvirtual_dev(devn) \
- (devn == FH_CYGDRIVE || devn == FH_PROC || devn == FH_REGISTRY
Re: Correct debugging output in seteuid32
I can see why would one think this is a bug, but it was meant
to be that way. Having a "wrong" gid can make seteuid fail.
Perhaps we could print the new and current uids and the current
gid to cover all cases.
Pierre
At 01:59 PM 4/14/2005 +0900, Kazuhiro Fujieda wrote:
>I'd submit a trivial patch after a long time.
>
>2005-04-14 Kazuhiro Fujieda <[EMAIL PROTECTED]>
>
> * syscalls.cc (setuid32): Correct debugging output.
>
>Index: syscalls.cc
>===
>RCS file: /cvs/src/src/winsup/cygwin/syscalls.cc,v
>retrieving revision 1.372
>diff -u -u -r1.372 syscalls.cc
>--- syscalls.cc11 Apr 2005 21:54:54 - 1.372
>+++ syscalls.cc14 Apr 2005 04:45:38 -
>@@ -1959,7 +1959,7 @@
> extern "C" int
> seteuid32 (__uid32_t uid)
> {
>- debug_printf ("uid: %u myself->gid: %u", uid, myself->gid);
>+ debug_printf ("uid: %u myself->uid: %u", uid, myself->uid);
>
> if (uid == myself->uid && !cygheap->user.groups.ischanged)
> {
>
>
> | AIST Kazuhiro Fujieda <[EMAIL PROTECTED]>
> | HOKURIKU School of Information Science
>o_/ 1990 Japan Advanced Institute of Science and Technology
>
>
[Patch]: hires_ms::usecs
The old test below will only detect the wraparound when "now" is
in the interval [0, inittime_ms), so it's likely to miss if
inittime_ms is very small (e.g. daemon starting at reboot).
With the new test, the wraparound will be detected if the function
is called at any time between the 25th and 49th day after startup.
Pierre
2005-03-27 Pierre Humblet <[EMAIL PROTECTED]>
*times.cc (hires_ms::usecs): Compare the difference.
Index: times.cc
===
RCS file: /cvs/src/src/winsup/cygwin/times.cc,v
retrieving revision 1.62
diff -u -p -r1.62 times.cc
--- times.cc2 Mar 2005 08:28:54 - 1.62
+++ times.cc27 Mar 2005 20:10:34 -
@@ -597,7 +597,7 @@ hires_ms::usecs (bool justdelta)
if (!minperiod) /* NO_COPY variable */
prime ();
DWORD now = timeGetTime ();
- if (now < initime_ms)
+ if (now - initime_ms < 0)
{
inited = 0;
prime ();
[Patch] hires.h
* timer.cc (nanosleep): Treat tv_sec < 0 as invalid.
Should be in signal.cc. But then I thought I had covered that case way
back. Turns out the root cause is in hires.h. With this patch, the nanosleep
patch can be reverted.
Pierre
2005-03-27 Pierre Humblet <[EMAIL PROTECTED]>
* hires.h: Add parentheses to HIRES_DELAY_MAX.
--- hires.h 23 Dec 2003 16:26:30 - 1.7
+++ hires.h 27 Mar 2005 19:54:04 -
@@ -19,7 +19,7 @@ details. */
The tv_sec argument in timeval structures cannot exceed
HIRES_DELAY_MAX / 1000 - 1, so that adding fractional part
and rounding won't exceed HIRES_DELAY_MAX */
-#define HIRES_DELAY_MAX (((UINT_MAX - 1) / 1000) * 1000) + 10
+#define HIRES_DELAY_MAX UINT_MAX - 1) / 1000) * 1000) + 10)
class hires_base
{
Re: [Patch]: Timer functions
- Original Message - From: "Christopher Faylor" To: Sent: Monday, March 07, 2005 11:28 AM Subject: Re: [Patch]: Timer functions > On Mon, Mar 07, 2005 at 11:20:40AM -0500, Pierre A. Humblet wrote: > >- Original Message - > >From: "Pierre A. Humblet" > >Sent: Sunday, March 06, 2005 11:40 PM > >Subject: Re: [Patch]: Timer functions > > > > > >> At 11:00 PM 3/6/2005 -0500, Christopher Faylor wrote: > >> >I am puzzled by a couple of things. > >> > > >> >Why did you decide to forego using th->detach in favor of (apparently) > >> >a: > >> > > >> > while (running) > >> > low_priority_sleep (0); > >> > >> These are not directly related. I got into this issue because of the bug > >> where cygthreads were not reused. I replaced th->detach by self_release > >> because that seemed to be the most natural and efficient way > >> to fix the problem. > > > >Also that frees the cygthread when the timer expires, not when it's > >rearmed (if ever). > > The design was that the thread is associated with the timer for as long > as the timer exists. The fact that it wasn't being detached when the > timer was deleted is a bug. Ah, I didn't realize you were willing to pay that price. > I don't see any reason to reinvent a less efficient way of detaching > from the thread when detach should do the job. Me neither. So now you detach from the delete call, not from the (rea)arming call. (The thread was being detached if the timer was rearmed while it was still timing out.) Pierre
Re: [Patch]: Timer functions
- Original Message - From: "Pierre A. Humblet" Sent: Sunday, March 06, 2005 11:40 PM Subject: Re: [Patch]: Timer functions > At 11:00 PM 3/6/2005 -0500, Christopher Faylor wrote: > >I am puzzled by a couple of things. > > > >Why did you decide to forego using th->detach in favor of (apparently) > >a: > > > > while (running) > > low_priority_sleep (0); > > These are not directly related. I got into this issue because of the bug > where cygthreads were not reused. I replaced th->detach by self_release > because that seemed to be the most natural and efficient way > to fix the problem. Also that frees the cygthread when the timer expires, not when it's rearmed (if ever). Pierre
Re: [Patch]: Timer functions
At 11:00 PM 3/6/2005 -0500, Christopher Faylor wrote:
>On Thu, Mar 03, 2005 at 11:45:45PM -0500, Pierre A. Humblet wrote:
>>The attached patch implements the alarm, ualarm, setitimer and
>>getitimer with the timer_xxx calls created by Chris last year.
>>
>>It has two objectives, both motivated by exim.
>>- The current implementation of alarm() opens a hidden window.
>>Thus, on Win9X, services calling alarm do not survive user logouts.
>>- When running exim as a service under a privileged (non system)
>>account on XP (trying out what's necessary on Win2003), I have hit
>>api_fatal ("couldn't create window, %E") with error 5.
>>
>>The implementation of getitimer has necessitated the development
>>of timer_gettime (not yet exported) and some changes to the logic
>>of the timer_thread. I have also fixed a FIXME about race condition
>>and two bugs:
>>- the initial code was not reusing the cygthreads (see attachment).
>>The fix involves using "auto_release" in the timer thread instead of
>>"detach" in the calling function.
>>- the mu_to was not reinitialized on forks (non-inheritable event).
>
>I've fixed the above two problems in the current code and am going
>through your patch trying to separate what looks like bug fixes from
>what looks like enhanced functionality.
>
>I am puzzled by a couple of things.
>
>Why did you decide to forego using th->detach in favor of (apparently)
>a:
>
> while (running)
> low_priority_sleep (0);
These are not directly related. I got into this issue because of the bug
where cygthreads were not reused. I replaced th->detach by self_release
because that seemed to be the most natural and efficient way
to fix the problem. I admit this is the first time ever that I looked
at cygthread.cc, and not everything is clear. Comments in detach() refer
to a "read thread", which is probably related to select. The correspondence
wasn't clear to me, and the code is complex. self_release is trivial.
With using self_release, I initially kept the old method where the
timer thread clears protect just before going away. But then I ran
into the issue discussed in the next paragraph.
The low_priority sleep waits for the timer cygthread to go away. This is
cleaner e.g. when deleting the timer. The thread still has a pointer
to the timer, and there was FIXME about that. Rightly so, the pointer
could point to deallocated memory.
When rearming a timer, the wait loop also insures that there is no race
to update sleepto_us (the timer thread could just start a new loop at
the time where settimer is called).
It's important to have external access to sleepto_us because gettime needs
it. That wasn't the case before, the timer thread didn't need to
communicate back..
>I never liked the idea that a muto had to be allocated for the timer
>functions regardless of whether they were going to be used.
Initially the muto was only used for the timer linked list. That seems
legitimate and hard to avoid.
I also use it for something else, although I went back and forth on this:
do we need to handle the case where two different threads try to arm the
same timer at the same time? I decided to be rather safe than sorry.
That's why the muto is used around the wait loop. That will serialize
access and avoid the creation of two timer threads for the same timer.
>You have
>extended this so that now an event will be allocated and a more
>complicated constructor will be called to fill out ttstart. Is that
>really necessary?
Not really. The event has always been there. It used to be created/deleted
each time a timer was (re)armed. Now the event is created/deleted once
for every timer. That seems more efficient.
The constructor is slightly more complicated, but there is only one when
there used to be two (there was one for ttstart and one for the dynamic
timers).
>FWIW, I checked in a change to muto initialization which protects the
>initialization with a CRITICAL_SECTION since I thought that I might be
>able to gain back the extra handle by protecting the initialization with
>a critical section but it looks like a critical section is more
>expensive than I thought it was, so I'll probably revert that change.
Not sure what you are talking about. I thought that muto's where always
created at process startup time (from dcrt0), so there is no need for
protection.
Pierre
Re: [Patch]: Timer functions
At 12:13 AM 3/4/2005 -0500, Christopher Faylor wrote: >On Thu, Mar 03, 2005 at 11:45:45PM -0500, Pierre A. Humblet wrote: >>- the mu_to was not reinitialized on forks (non-inheritable event). > >I just spent at least ten minutes looking for a "mu_to" in cygwin, >trying to figure out what you were referring to. I'm not sure why >you're putting an underscore in the middle there. Maybe you're thinking >that the "mu" and "to" have separate meanings but they really don't. Good question. Perhaps because it was late and there is an _ before the muto in new_muto. >Did you actually see mutos not getting created? Looking at the code >now, it seems like there would be a new muto created every time >there is a new instance of timer_tracker, which was certainly wrong but >it is different from mutos not being created after a fork. As far as I could see, the muto was only created by the constructor of ttstart and not recreated after a fork. But perhaps you are right and it was created every time. At any rate it should be OK now. Pierre
[Patch]: Timer functions
The attached patch implements the alarm, ualarm, setitimer and
getitimer with the timer_xxx calls created by Chris last year.
It has two objectives, both motivated by exim.
- The current implementation of alarm() opens a hidden window.
Thus, on Win9X, services calling alarm do not survive user logouts.
- When running exim as a service under a privileged (non system)
account on XP (trying out what's necessary on Win2003), I have hit
api_fatal ("couldn't create window, %E") with error 5.
The implementation of getitimer has necessitated the development
of timer_gettime (not yet exported) and some changes to the logic
of the timer_thread. I have also fixed a FIXME about race condition
and two bugs:
- the initial code was not reusing the cygthreads (see attachment).
The fix involves using "auto_release" in the timer thread instead of
"detach" in the calling function.
- the mu_to was not reinitialized on forks (non-inheritable event).
Pierre
2005-03-05 Pierre Humblet <[EMAIL PROTECTED]>
* window.cc (getitimer): Delete.
(setitimer): Ditto.
(ualarm): Ditto.
(alarm): Ditto.
* timer.cc (struct timetracker): Delete it, flags and a creator.
Add it_interval, interval_us, sleepto_us, running, init_muto(),
and gettime().
(timer_tracker::timer_tracker): Create event. Distinguish ttstart case.
(timer_tracker::init_muto): New method.
(to_us): Round up as per POSIX.
(timer_thread): Reorganize to match timer_tracker::settime and
timer_tracker::gettime. Call sig_send without wait. Call
th->auto_release.
(timer_tracker::settime): Reorganize logic to avoid race.
Call gettime to recover old value. Do not call th->detach.
(timer_tracker::gettime): New method.
(timer_gettime): New function.
(timer_delete): Stop the timer_thread, clear magic and close the event.
(fixup_timers_after_fork): Reinit ttstart and the mu_to.
(getitimer): New implementation.
(setitimer): Ditto.
(ualarm): Ditto.
(alarm): Ditto.Index: window.cc
===
RCS file: /cvs/src/src/winsup/cygwin/window.cc,v
retrieving revision 1.34
diff -u -p -r1.34 window.cc
--- window.cc 26 Nov 2004 04:15:09 - 1.34
+++ window.cc 4 Mar 2005 02:00:40 -
@@ -149,17 +149,6 @@ HWND ()
return hwnd;
}
-extern "C" int
-setitimer (int which, const struct itimerval *value, struct itimerval
*oldvalue)
-{
- if (which != ITIMER_REAL)
-{
- set_errno (ENOSYS);
- return -1;
-}
- return winmsg.setitimer (value, oldvalue);
-}
-
/* FIXME: Very racy */
int __stdcall
wininfo::setitimer (const struct itimerval *value, struct itimerval *oldvalue)
@@ -198,22 +187,6 @@ wininfo::setitimer (const struct itimerv
return 0;
}
-extern "C" int
-getitimer (int which, struct itimerval *value)
-{
- if (which != ITIMER_REAL)
-{
- set_errno (EINVAL);
- return -1;
-}
- if (value == NULL)
-{
- set_errno (EFAULT);
- return -1;
-}
- return winmsg.getitimer (value);
-}
-
/* FIXME: racy */
int __stdcall
wininfo::getitimer (struct itimerval *value)
@@ -236,39 +209,6 @@ wininfo::getitimer (struct itimerval *va
return 0;
}
-extern "C" unsigned int
-alarm (unsigned int seconds)
-{
- int ret;
- struct itimerval newt, oldt;
-
- newt.it_value.tv_sec = seconds;
- newt.it_value.tv_usec = 0;
- newt.it_interval.tv_sec = 0;
- newt.it_interval.tv_usec = 0;
- setitimer (ITIMER_REAL, &newt, &oldt);
- ret = oldt.it_value.tv_sec;
- if (ret == 0 && oldt.it_value.tv_usec)
-ret = 1;
- return ret;
-}
-
-extern "C" useconds_t
-ualarm (useconds_t value, useconds_t interval)
-{
- struct itimerval timer, otimer;
-
- timer.it_value.tv_sec = 0;
- timer.it_value.tv_usec = value;
- timer.it_interval.tv_sec = 0;
- timer.it_interval.tv_usec = interval;
-
- if (setitimer (ITIMER_REAL, &timer, &otimer) < 0)
-return (u_int)-1;
-
- return (otimer.it_value.tv_sec * 100) + otimer.it_value.tv_usec;
-}
-
bool
has_visible_window_station (void)
{
Index: timer.cc
===
RCS file: /cvs/src/src/winsup/cygwin/timer.cc,v
retrieving revision 1.5
diff -u -p -r1.5 timer.cc
--- timer.cc6 Jan 2005 16:33:59 - 1.5
+++ timer.cc4 Mar 2005 02:00:49 -
@@ -26,25 +26,26 @@ struct timer_tracker
unsigned magic;
clockid_t clock_id;
sigevent evp;
- itimerspec it;
+ timespec it_interval;
HANDLE cancel;
- int flags;
+ long long interval_us;
+ long long sleepto_us;
cygthread *th;
struct timer_tracker *next;
+ volatile bool running;
+ void init_muto ();
int settime (int, const itimerspec *, itimerspec *);
+ void gettime (bool, itimerspec *);
timer_tracker (clockid_t, const sigevent *);
- timer_tracker ();
};
-timer_tracker ttstart;
+/* Used for the alarm, ualarm and setitimer calls.
+ Also serves as
[Patch]: fhandler_socket::ioctl
This patch avoids the unnecessary creation of the win thread in window.cc It fails to create the invisible window (error 5) when running exim as a service under a privileged non SYSTEM account (XP home, SP 2). Pierre 2005-03-04 Pierre Humblet <[EMAIL PROTECTED]> * fhandler_socket.cc (fhandler_socket::ioctl): Only cancel WSAAsyncSelect when async mode is on. Index: fhandler_socket.cc === RCS file: /cvs/src/src/winsup/cygwin/fhandler_socket.cc,v retrieving revision 1.150 diff -u -p -r1.150 fhandler_socket.cc --- fhandler_socket.cc 28 Feb 2005 13:11:49 - 1.150 +++ fhandler_socket.cc 4 Mar 2005 00:32:57 - @@ -1594,7 +1594,7 @@ fhandler_socket::ioctl (unsigned int cmd /* We must cancel WSAAsyncSelect (if any) before setting socket to * blocking mode */ - if (cmd == FIONBIO && *(int *) p == 0) + if (cmd == FIONBIO && async_io () && *(int *) p == 0) WSAAsyncSelect (get_socket (), winmsg, 0, 0); res = ioctlsocket (get_socket (), cmd, (unsigned long *) p); if (res == SOCKET_ERROR)
Re: [Patch]: fs_info::update
Christopher Faylor wrote:
>
> On Fri, Jan 28, 2005 at 10:10:56AM -0500, Pierre A. Humblet wrote:
> >Corinna Vinschen wrote:
> >>This looks pretty much like a band-aid. I can see the use for checking
> >>the last error code, but shouldn't Cygwin opt for safety and not assume
> >>ACLs? Also, if there's no right to read a remote drive, there might be
> >>a good reason for that, which doesn't necessarily mean the drive has
> >>acls.
> >>
> >>After all, the effect of chmod -r can be reverted with Windows own
> >>means.
> >
> >Background: I noticed all of that when testing the
> >SetCurrentDirectory("c:\\"). Took me a while to understand why chmod
> >stopped working. On XP HOME there is no security gui, so I had to use
> >cacls. Not nice.
>
> Are you saying this is somehow a side-effect of
> SetCurrentDirectory("c:\\") in exit()? I can't imagine how that change
> could cause this behavior.
No, no. I was checking that the rmdir bug would come back if I removed
access to c:\ (It did).
Pierre
Re: [Patch]: fs_info::update
Corinna Vinschen wrote:
>
>
> This looks pretty much like a band-aid. I can see the use for checking
> the last error code, but shouldn't Cygwin opt for safety and not assume
> ACLs? Also, if there's no right to read a remote drive, there might be
> a good reason for that, which doesn't necessarily mean the drive has acls.
>
> After all, the effect of chmod -r can be reverted with Windows own means.
Background: I noticed all of that when testing the SetCurrentDirectory("c:\\").
Took me a while to understand why chmod stopped working. On XP HOME there
is no security gui, so I had to use cacls. Not nice.
By the time we call fs_info::update, we have done a successful
GetFileAttributes for a file on the disk. So we know we can access it OK.
I can't imagine any mechanism whereby GetVolumeInfo would return ACCESS_DENIED
if there were no acls. For remote drives has_acls is off by default (smbntsec).
Pierre
[Patch]: fs_info::update
When a user has no read access to the root of a drive, GetVolumeInformation
fails and has_acls is left unset. Consequently ntsec is off on that drive.
If a user "chmod -r" the root of a drive, ntsec is turned off and "chmod +r"
has no effect.
The patch does its best to set has_acls even in case of failure.
2005-01-28 Pierre Humblet <[EMAIL PROTECTED]>
* path.cc (fs_info::update) Set has_acls even in case of failure.
Index: path.cc
===
RCS file: /cvs/src/src/winsup/cygwin/path.cc,v
retrieving revision 1.340
diff -u -p -r1.340 path.cc
--- path.cc 26 Jan 2005 04:34:19 - 1.340
+++ path.cc 28 Jan 2005 02:48:54 -
@@ -381,6 +381,8 @@ fs_info::update (const char *win32_path)
debug_printf ("Cannot get volume information (%s), %E", root_dir);
has_buggy_open (false);
has_ea (false);
+ has_acls (GetLastError () == ERROR_ACCESS_DENIED
+&& (allow_smbntsec || !is_remote_drive ()));
flags () = serial () = 0;
return false;
}
Re: [Patch]: setting errno to ENOTDIR rather than ENOENT
Corinna Vinschen wrote:
>
> Well done! I looked into this a few hours ago and missed how easy a
> solution would be. *mumbling something about needing glasses*
Ah, I see your message on the list.
You found out that
lstat("dir/x") with dir non-existing. => ENOENT
So
> if (pcheck_case == PCHECK_STRICT)
> {
> case_clash = true;
> > - error = ENOENT;
> > + error = component?ENOTDIR:ENOENT;
shouldn't be done after all. OK?
Pierre
Re: [Patch]: setting errno to ENOTDIR rather than ENOENT
Corinna Vinschen wrote: > > Well done! I looked into this a few hours ago and missed how easy a > solution would be. *mumbling something about needing glasses* > > I guess this is ok to check in after adding some spaces... OK, will do. Do you agree about removing the unreachable (?) code? If there is a taker, the virtual stuff needs to be fixed, but the whole approach should perhaps be reworked. See FIXME on line 616 of path.cc Pierre
[Patch]: setting errno to ENOTDIR rather than ENOENT
This patch should take care of the error reported by
Eric Blake on the list, at least for disk files.
It also removes code under the condition
(opt & PC_SYM_IGNORE) && pcheck_case == PCHECK_RELAXED
which is never true, AFAICS.
It also gets rid of an obsolete function.
While testing, the assert (!i); on line 259 of pinfo.cc
kicks in. That's a feature because when flag & PID_EXECED
the code just loops, keeping the same h0 and mapname!
Am I the only one to see that?
Pierre
2005-01-25 Pierre Humblet <[EMAIL PROTECTED]>
* path.cc (path_conv::check): Return ENOTDIR rather than ENOENT
when a component is not a directory. Remove unreachable code.
(digits): Delete.
Index: path.cc
===
RCS file: /cvs/src/src/winsup/cygwin/path.cc,v
retrieving revision 1.338
diff -u -p -r1.338 path.cc
--- path.cc 18 Jan 2005 13:00:18 - 1.338
+++ path.cc 25 Jan 2005 20:08:53 -
@@ -655,12 +655,6 @@ path_conv::check (const char *src, unsig
full_path[3] = '\0';
}
- if ((opt & PC_SYM_IGNORE) && pcheck_case == PCHECK_RELAXED)
- {
- fileattr = GetFileAttributes (this->path);
- goto out;
- }
-
symlen = sym.check (full_path, suff, opt | fs.has_ea ());
if (sym.minor || sym.major)
@@ -680,7 +674,7 @@ path_conv::check (const char *src, unsig
if (pcheck_case == PCHECK_STRICT)
{
case_clash = true;
- error = ENOENT;
+ error = component?ENOTDIR:ENOENT;
goto out;
}
/* If pcheck_case==PCHECK_ADJUST the case_clash is remembered
@@ -706,6 +700,11 @@ path_conv::check (const char *src, unsig
error = sym.error;
if (component == 0)
add_ext_from_sym (sym);
+ else if (!(sym.fileattr & FILE_ATTRIBUTE_DIRECTORY))
+{
+ error = ENOTDIR;
+ goto out;
+}
if (pcheck_case == PCHECK_RELAXED)
goto out; // file found
/* Avoid further symlink evaluation. Only case checks are
@@ -939,15 +938,6 @@ path_conv::~path_conv ()
}
}
-static __inline int
-digits (const char *name)
-{
- char *p;
- int n = strtol (name, &p, 10);
-
- return p > name && !*p ? n : -1;
-}
-
/* Return true if src_path is a valid, internally supported device name.
In that case, win32_path gets the corresponding NT device name and
dev is appropriately filled with device information. */
[Patch] mkpasswd
This improves error reporting
Pierre
2005-01-11 Pierre Humblet <[EMAIL PROTECTED]>
* mkpasswd.c (print_win_error): Transform into macro.
(_print_win_error): Upgrade former print_win_error by
printing the line.
(current_user): Call _print_win_error.
(enum_users): Print name in case of lookup failure.
(enum_local_groups): Ditto.
Index: mkpasswd.c
===
RCS file: /cvs/src/src/winsup/utils/mkpasswd.c,v
retrieving revision 1.33
diff -u -p -r1.33 mkpasswd.c
--- mkpasswd.c 14 Nov 2003 19:14:43 - 1.33
+++ mkpasswd.c 11 Jan 2005 20:44:29 -
@@ -23,6 +23,8 @@
#include
#include
+#define print_win_error(x) _print_win_error(x, __LINE__)
+
static const char version[] = "$Revision: 1.20 $";
SID_IDENTIFIER_AUTHORITY sid_world_auth = {SECURITY_WORLD_SID_AUTHORITY};
@@ -111,7 +113,7 @@ uni2ansi (LPWSTR wcs, char *mbs, int siz
}
void
-print_win_error(DWORD code)
+_print_win_error(DWORD code, int line)
{
char buf[4096];
@@ -121,9 +123,9 @@ print_win_error(DWORD code)
code,
MAKELANGID (LANG_NEUTRAL, SUBLANG_DEFAULT),
(LPTSTR) buf, sizeof (buf), NULL))
-fprintf (stderr, "mkpasswd: [%lu] %s", code, buf);
+fprintf (stderr, "mkpasswd (%d): [%lu] %s", line, code, buf);
else
-fprintf (stderr, "mkpasswd: error %lu", code);
+fprintf (stderr, "mkpasswd (%d): error %lu", line, code);
}
void
@@ -159,10 +161,7 @@ current_user (int print_sids, int print_
|| (!CloseHandle (ptok) && (errpos = __LINE__)))
{
if (errpos)
- {
- print_win_error (GetLastError ());
- fprintf(stderr, " on line %d\n", errpos);
- }
+ _print_win_error (GetLastError (), errpos);
return;
}
@@ -309,6 +308,7 @@ enum_users (LPWSTR servername, int print
&acc_type))
{
print_win_error(GetLastError ());
+ fprintf(stderr, " (%s)\n", username);
continue;
}
else if (acc_type == SidTypeDomain)
@@ -327,6 +327,7 @@ enum_users (LPWSTR servername, int print
&acc_type))
{
print_win_error(GetLastError ());
+ fprintf(stderr, " (%s)\n", domname);
continue;
}
}
@@ -401,6 +402,7 @@ enum_local_groups (int print_sids)
&acc_type))
{
print_win_error(GetLastError ());
+ fprintf(stderr, " (%s)\n", localgroup_name);
continue;
}
else if (acc_type == SidTypeDomain)
@@ -418,6 +420,7 @@ enum_local_groups (int print_sids)
&acc_type))
{
print_win_error(GetLastError ());
+ fprintf(stderr, " (%s)\n", domname);
continue;
}
}
[Patch]: seteuid
Currently the process default dacl is changed in seteuid even
when seteuid fails. This is a potentially security hole.
The patch fixes it.
Also HKCU is not closed anymore, as it is not used by Cygwin.
It's now up to applications (if any) to close it, and they should
keep MS KB 199190 in mind.
Pierre
2005-01-08 Pierre Humblet <[EMAIL PROTECTED]>
* syscalls.cc (seteuid32): Only change the default dacl when
seteuid succeeds. Do not close HKCU.
Index: syscalls.cc
===
RCS file: /cvs/src/src/winsup/cygwin/syscalls.cc,v
retrieving revision 1.355
diff -u -p -r1.355 syscalls.cc
--- syscalls.cc 6 Jan 2005 22:10:08 - 1.355
+++ syscalls.cc 8 Jan 2005 00:56:42 -
@@ -2066,7 +2066,7 @@ seteuid32 (__uid32_t uid)
if (!wincap.has_security () && pw_new)
{
load_registry_hive (pw_new->pw_name);
-goto success_9x;
+ goto success_9x;
}
if (!usersid.getfrompw (pw_new))
{
@@ -2103,16 +2103,6 @@ seteuid32 (__uid32_t uid)
debug_printf ("Found token %d", new_token);
- /* Set process def dacl to allow access to impersonated token */
- if (sec_acl ((PACL) dacl_buf, true, true, usersid))
-{
- tdacl.DefaultDacl = (PACL) dacl_buf;
- if (!SetTokenInformation (ptok, TokenDefaultDacl,
- &tdacl, sizeof dacl_buf))
- debug_printf ("SetTokenInformation"
- "(TokenDefaultDacl), %E");
-}
-
/* If no impersonation token is available, try to
authenticate using NtCreateToken () or subauthentication. */
if (new_token == INVALID_HANDLE_VALUE)
@@ -2132,6 +2122,16 @@ seteuid32 (__uid32_t uid)
cygheap->user.internal_token = new_token;
}
+ /* Set process def dacl to allow access to impersonated token */
+ if (sec_acl ((PACL) dacl_buf, true, true, usersid))
+{
+ tdacl.DefaultDacl = (PACL) dacl_buf;
+ if (!SetTokenInformation (ptok, TokenDefaultDacl,
+ &tdacl, sizeof dacl_buf))
+ debug_printf ("SetTokenInformation"
+ "(TokenDefaultDacl), %E");
+}
+
if (new_token != ptok)
{
/* Avoid having HKCU use default user */
@@ -2166,11 +2166,8 @@ success_9x:
cygheap->user.set_name (pw_new->pw_name);
myself->uid = uid;
groups.ischanged = FALSE;
- if (!issamesid) /* MS KB 199190 */
-{
- RegCloseKey (HKEY_CURRENT_USER);
- user_shared_initialize (true);
-}
+ if (!issamesid)
+user_shared_initialize (true);
return 0;
failed:
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
At 12:54 AM 12/24/2004 -0500, Pierre A. Humblet wrote:
>At 12:25 AM 12/24/2004 -0500, Christopher Faylor wrote:
>>On Thu, Dec 23, 2004 at 11:59:59PM -0500, Pierre A. Humblet wrote:
>>>At 11:35 PM 12/23/2004 -0500, Christopher Faylor wrote:
>>>>I don't think you need it. You just need to tell a process which is
>>>>about to exec after having been execed to make sure that its
>>>>wr_proc_pipe is valid.
>>>
>>>Yes, that's the key. So the question is only about method. Either the
parent
>>>guarantees that the child has a valid handle, or the child must check
>>>that it already has a valid handle or wait until it does.
>>
>>I have just implemented code which causes an execed child to wait for the
>>parent to fill in its wr_proc_pipe if it is going to exec again. It uses
>>a busy loop but I think it's unlikely that the loop will be exercised too
>>often.
>
>It's late, but I am trying to go through all permutations.
>Here is a strange one.
>Cygwin process A started from Windows execs a Windows process B.
>We are in the case where A
> if (!myself->wr_proc_pipe)
> {
> myself.remember (true);
> wait_for_myself = true;
>
>The problem is that later there is
>if (wait_for_myself)
> waitpid (myself->pid, &res, 0);
>else
> ciresrv.sync (myself, INFINITE);
>
>Process A takes the first branch (waitpid), although it's the
>second branch that will call GetExitCodeProcess.
>So A will see its logical self terminate, but it won't get the
>exit status of B.
>Right? Going to sleep on this.
I think the way out is as follows:
Toward the end of spawn_guts:
ciresrv.sync (myself, INFINITE); [always]
if (wait_for_myself)
waitpid (myself->pid, &dummy, 0);
[For clarity, these two lines should be brought down
inside the case _P_OVERLAY: ]
and in pinfo::exit, change ExitProcess (n) to
ExitProcess (exitcode)
There is NO NEED for a Cygwin process started from Windows to
start a new exec'ed process in suspended state.
The ciressrv.sync will collect the exit status of any Windows
process.
The purpose of the waitpid is to wait for the process chain to be
finished. But waitpid will fail if the child had terminated before
the pipe could be duplicated. That't why waitpid uses "dummy".
At any rate the final return value of the chain is safely set in
the exitstatus.
The change in pinfo::exit is to handle the "norecord" case. In that
case the value of n is meaningless, the correct exit code is already
set in exitcode.
Pierre
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
At 12:25 AM 12/24/2004 -0500, Christopher Faylor wrote:
>On Thu, Dec 23, 2004 at 11:59:59PM -0500, Pierre A. Humblet wrote:
>>At 11:35 PM 12/23/2004 -0500, Christopher Faylor wrote:
>>>I don't think you need it. You just need to tell a process which is
>>>about to exec after having been execed to make sure that its
>>>wr_proc_pipe is valid.
>>
>>Yes, that's the key. So the question is only about method. Either the parent
>>guarantees that the child has a valid handle, or the child must check
>>that it already has a valid handle or wait until it does.
>
>I have just implemented code which causes an execed child to wait for the
>parent to fill in its wr_proc_pipe if it is going to exec again. It uses
>a busy loop but I think it's unlikely that the loop will be exercised too
>often.
It's late, but I am trying to go through all permutations.
Here is a strange one.
Cygwin process A started from Windows execs a Windows process B.
We are in the case where A
if (!myself->wr_proc_pipe)
{
myself.remember (true);
wait_for_myself = true;
The problem is that later there is
if (wait_for_myself)
waitpid (myself->pid, &res, 0);
else
ciresrv.sync (myself, INFINITE);
Process A takes the first branch (waitpid), although it's the
second branch that will call GetExitCodeProcess.
So A will see its logical self terminate, but it won't get the
exit status of B.
Right? Going to sleep on this.
Pierre
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
At 11:35 PM 12/23/2004 -0500, Christopher Faylor wrote: >On Thu, Dec 23, 2004 at 11:05:50PM -0500, Pierre A. Humblet wrote: >>At 10:50 PM 12/23/2004 -0500, Christopher Faylor wrote: >>>On Thu, Dec 23, 2004 at 09:54:20PM -0500, Pierre A. Humblet wrote: >>>>At 09:27 PM 12/23/2004 -0500, Christopher Faylor wrote: >>>>>On Thu, Dec 23, 2004 at 06:23:06PM -0500, Pierre A. Humblet wrote: >>> >>>FWIW, I modified cygwin so that it seems as if with a status of '1' if >>>you use ExitProcess and a status of "SIGTERM" (as in terminated by a >>>signal) if you kill a process via task manager (aka TerminateProcess). >> >>Good. >> >>>>>>This can be fixed with my lunch time ideas of yesterday. >>>>>>Looking at the code, I saw that most of them were already >>>>>>implemented. The only changes are: >>>>>>1) remove child_proc_info->parent_wr_proc_pipe stuff >>>>>>2) in pinfo::wait, duplicate into non-inheritable wr_proc_pipe >>>>> >>>>>As may not be too surprising, I already considered creating the pipe >>>>>before creating the process (somehow I am hearing echoes of John Kerry >>>>>here). I actually coded it that way to begin with but then chose to go >>>>>with the current plan. >>>> >>>>That's fine, and I am not suggesting that you should change your way. >>> >>>Ah. I didn't get that the first two times you explained it. Now I get >>>it. Sorry. >>> >>>>Perhaps I wasn't clear. Just duplicate the pipe into the new process, >>>>as you do now, but not inheritable. The new process makes it inheritable >>>>before execing a new new process. >>> >>>How about just using the present method but never making the handle >>>inheritable? Just duplicate the wr_proc_pipe to the child on a >>>fork/spawn, closing the original in the DuplicateHandle. >> >>Fine. >> >>> When a process >>>execs, use the same code to pass the pipe along to the newly created >>>"child". If the DuplicateHandle fails because the process has already >>>exited, it doesn't matter because the stub is exiting anyway. >> >>The problem in that case is that the logical parent won't get the exit >>status because the final process won't have the pipe open. > >It doesn't matter if the final process has the pipe open as long as the >final process filled out exitcode. The parent will notice that the >cygwin process exits when the stub exits in that case. Right, sorry for the bad explanation. But I see you got what I meant: >Although, hmm, a process which execs another process before getting its >wr_proc_pipe filled out will have a problem. > >>I see two ways to guarantee that the pipe is passed: >>1) always start the new exec'ed process in suspended state (not only >>when the parent was started from Windows > >I've never been clear on whether the other process needs to have the >shared memory mapped into its space for DuplicateHandle to fill in the >value. If it does need this then CREATE_SUSPEND won't work. Not sure I understand. The parent duplicates the handle into the child (it tells Windows to do it) and the new value is written in shared memory, which won't yet be opened in the child. So Windows knows that the child "has" the handle, and the child will know its value when it maps the shared memory. >>2) Make the pipe inheritable before exec. I prefer 2 because it's >>bound to be slightly faster. > >I don't think you need it. You just need to tell a process which is >about to exec after having been execed to make sure that its >wr_proc_pipe is valid. Yes, that's the key. So the question is only about method. Either the parent guarantees that the child has a valid handle, or the child must check that it already has a valid handle or wait until it does. >>>I think it ends up being fewer number of DuplicateHandles in that case >>>because you won't have to make the handle noninheritable again if the >>>CreateProcess fails. >>> >>>I've implemented this code and it seems to work. >> >>Perhaps, but I think there is a race. >> >>To avoid having the "undo" of the inheritance, I was suggesting after >>point 4) a couple of mails ago that we keep track of the inheritance >>state of the pipe. The rules are simple: - if the pipe is inheritable >>when fork/spawn: make it non inheritable - if the pipe is >>non-inheritable when exec: make it inheritable. So if an exec >>CreateProcess fails, there is nothing to do until the next fork/spawn, >>if any. > >That just delays the setting of "noninheritance" to the next potential fork >or spawn, adding a test to every fork/spawn. Right, but that test is negligible compared to the overhead of Duplicating the handle and forking/spawning. Very often there is a master process that forks repeatedly and rarely execs, while the children execs. So to minimize the amount of DuplicateHandle, forked children should start with a inheritable handle. If they exec, nothing to do. If they fork, they must turn inheritance off. Those are really secondary considerations, sorry to waste time. The essential is clear. Pierre
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
At 10:50 PM 12/23/2004 -0500, Christopher Faylor wrote: >On Thu, Dec 23, 2004 at 09:54:20PM -0500, Pierre A. Humblet wrote: >>At 09:27 PM 12/23/2004 -0500, Christopher Faylor wrote: >>>On Thu, Dec 23, 2004 at 06:23:06PM -0500, Pierre A. Humblet wrote: > >FWIW, I modified cygwin so that it seems as if with a status of '1' if >you use ExitProcess and a status of "SIGTERM" (as in terminated by a >signal) if you kill a process via task manager (aka TerminateProcess). Good. >>>>This can be fixed with my lunch time ideas of yesterday. >>>>Looking at the code, I saw that most of them were already >>>>implemented. The only changes are: >>>>1) remove child_proc_info->parent_wr_proc_pipe stuff >>>>2) in pinfo::wait, duplicate into non-inheritable wr_proc_pipe >>> >>>As may not be too surprising, I already considered creating the pipe >>>before creating the process (somehow I am hearing echoes of John Kerry >>>here). I actually coded it that way to begin with but then chose to go >>>with the current plan. >> >>That's fine, and I am not suggesting that you should change your way. > >Ah. I didn't get that the first two times you explained it. Now I get >it. Sorry. > >>Perhaps I wasn't clear. Just duplicate the pipe into the new process, >>as you do now, but not inheritable. The new process makes it inheritable >>before execing a new new process. > >How about just using the present method but never making the handle >inheritable? Just duplicate the wr_proc_pipe to the child on a >fork/spawn, closing the original in the DuplicateHandle. Fine. > When a process >execs, use the same code to pass the pipe along to the newly created >"child". If the DuplicateHandle fails because the process has already >exited, it doesn't matter because the stub is exiting anyway. The problem in that case is that the logical parent won't get the exit status because the final process won't have the pipe open. I see two ways to guarantee that the pipe is passed: 1) always start the new exec'ed process in suspended state (not only when the parent was started from Windows 2) Make the pipe inheritable before exec. I prefer 2 because it's bound to be slightly faster. >I think >it ends up being fewer number of DuplicateHandles in that case because >you won't have to make the handle noninheritable again if the >CreateProcess fails. > >I've implemented this code and it seems to work. Perhaps, but I think there is a race. To avoid having the "undo" of the inheritance, I was suggesting after point 4) a couple of mails ago that we keep track of the inheritance state of the pipe. The rules are simple: - if the pipe is inheritable when fork/spawn: make it non inheritable - if the pipe is non-inheritable when exec: make it inheritable. So if an exec CreateProcess fails, there is nothing to do until the next fork/spawn, if any. >>>I'll have to think about my reasons for not implementing it that way. >>>I don't remember what they are right now. >> >>OK. >> >>I am looking at the code again. As I see it, a cygwin process A >>started from Windows that execs another process B will create the pipe >>after B has been created. There is a possibility that B has already >>terminated at that moment, perhaps after having exec'ed another process >>C. Is that race taken care of? Perhaps a Cygwin process started from >>Windows should start an exec'ed process in the suspended state. > >I thought I tested that case but, you're right, it's a problem. I'll >create the process in a suspended state when !myself->wr_proc_pipe. > OK. Pierre
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
At 09:27 PM 12/23/2004 -0500, Christopher Faylor wrote: >On Thu, Dec 23, 2004 at 06:23:06PM -0500, Pierre A. Humblet wrote: > >The side effect of doing things this way is that the program will seem >to have exited with a SIGTERM value if it calls ExitProcess, too. >That is a depature from previous behavior that I'm sure I'll hear about. Good point. But even if you leave it to be 0, the complain will be that the exit value won't be passed. Of course a Cygwin program should call exit. >>If my spawn(P_DETACH) program of yesterday is terminated from >>Windows during the sleep interval, then the parent process does >>not notice the termination and keeps waiting. > >I think I already mentioned this as a side effect of the new code. I must have missed it. >>This can be fixed with my lunch time ideas of yesterday. >>Looking at the code, I saw that most of them were already >>implemented. The only changes are: >>1) remove child_proc_info->parent_wr_proc_pipe stuff >>2) in pinfo::wait, duplicate into non-inheritable wr_proc_pipe > >As may not be too surprising, I already considered creating the pipe >before creating the process (somehow I am hearing echoes of John Kerry >here). I actually coded it that way to begin with but then chose to go >with the current plan. That's fine, and I am not suggesting that you should change your way. Perhaps I wasn't clear. Just duplicate the pipe into the new process, as you do now, but not inheritable. The new process makes it inheritable before execing a new new process. >I'll have to think about my reasons for not implementing it that way. >I don't remember what they are right now. OK. I am looking at the code again. As I see it, a cygwin process A started from Windows that execs another process B will create the pipe after B has been created. There is a possibility that B has already terminated at that moment, perhaps after having exec'ed another process C. Is that race taken care of? Perhaps a Cygwin process started from Windows should start an exec'ed process in the suspended state. Pierre
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
I have run some more tests with a built from cvs this afternoon and peeked at some of the changes, so here is some feedback. When a process is terminated from Windows, the reported exit code is 0. This can easily be fixed by initializing exitcode to the value it should take when a process is terminated. If my spawn(P_DETACH) program of yesterday is terminated from Windows during the sleep interval, then the parent process does not notice the termination and keeps waiting. This can be fixed with my lunch time ideas of yesterday. Looking at the code, I saw that most of them were already implemented. The only changes are: 1) remove child_proc_info->parent_wr_proc_pipe stuff 2) in pinfo::wait, duplicate into non-inheritable wr_proc_pipe 3) make wr_proc_pipe inheritable just before exec 4) make wr_proc_pipe non-inheritable when starting after exec (or better? at the first fork or spawn, leaving 2) as currently done) Comments on pinfo.cc comments: Delete: "but, unfortunately, reparenting is still needed ..." It's really gone, isn't it? Update: "We could just let this happen automatically when the process.." to indicate it's needed by P_DETACH (at least with current code) Pierre
Re: [PATCH]: Still stripping
Christopher Faylor wrote: > > On Thu, Dec 23, 2004 at 11:42:15AM -0500, Pierre A. Humblet wrote: > >In a case such as "abc..exe", the posix_path "abc." should not be > >stripped. The patch below only strips the posix path if the win32 > >path was stripped. I don't think that the posix path can be empty > >in that case. > > > >2004-12-23 Pierre Humblet <[EMAIL PROTECTED]> > > > > * path.h (path_conv::set_normalized_path): Add second argument. > > * path.cc (path_conv::check): Declare, set and use "strip_tail". > > (path_conv::set_normalized_path): Add and use second argument, > > replacing all tail stripping tests. > > > > I'm not sure that your assumption of dot stripping is true in the first > case of set_normalized_path in build_fh_dev in dtable.cc. Not sure I understand what you mean. At any rate there are two cases where build_fh_dev is called with a non-empty second argument: handler_tty.cc: console = (fhandler_console *) build_fh_dev (*console_dev, "/dev/ttym"); path.cc: fhandler_virtual *fh = (fhandler_virtual *) build_fh_dev (dev, path_copy); Neither is about a disk path. > I do like the > idea of letting the previously derived path_conv tail stripping test > control whether set_normalized_path does stripping or not, though. > > I have grown to dislike default parameters in c++. I'm not sure why because > I used to think they were pretty nifty. OK. Same here, having been burned. > So, I'll check in your patch minus > the default and keeping the original while loop in set_normalized_path > more or less intact. Aren't they the same? I thought it was nice to avoid having a strlen in one case and a strchr in another. Pierre
[PATCH]: Still stripping
In a case such as "abc..exe", the posix_path "abc." should not be
stripped. The patch below only strips the posix path if the win32
path was stripped. I don't think that the posix path can be empty
in that case.
Pierre
2004-12-23 Pierre Humblet <[EMAIL PROTECTED]>
* path.h (path_conv::set_normalized_path): Add second argument.
* path.cc (path_conv::check): Declare, set and use "strip_tail".
(path_conv::set_normalized_path): Add and use second argument,
replacing all tail stripping tests.
Index: path.h
===
RCS file: /cvs/src/src/winsup/cygwin/path.h,v
retrieving revision 1.67
diff -u -p -r1.67 path.h
--- path.h 2 Oct 2004 02:20:20 - 1.67
+++ path.h 23 Dec 2004 16:07:45 -
@@ -214,7 +214,7 @@ class path_conv
unsigned __stdcall ndisk_links (DWORD);
char *normalized_path;
size_t normalized_path_size;
- void set_normalized_path (const char *) __attribute__ ((regparm (2)));
+ void set_normalized_path (const char *, bool strip=false) __attribute__
((regparm (2)));
DWORD get_symlink_length () { return symlink_length; };
private:
DWORD symlink_length;
Index: path.cc
===
RCS file: /cvs/src/src/winsup/cygwin/path.cc,v
retrieving revision 1.333
diff -u -p -r1.333 path.cc
--- path.cc 22 Dec 2004 11:31:30 - 1.333
+++ path.cc 23 Dec 2004 16:24:52 -
@@ -424,21 +424,18 @@ path_conv::fillin (HANDLE h)
}
void
-path_conv::set_normalized_path (const char *path_copy)
+path_conv::set_normalized_path (const char *path_copy, bool strip_tail)
{
char *eopath = strchr (path, '\0');
- size_t n;
+ char *p = strchr (path_copy, '\0');
- if (dev.devn != FH_FS || !*path_copy || strncmp (path_copy, "//./", 4) == 0)
-n = strlen (path_copy) + 1;
- else
+ if (strip_tail)
{
- char *p = strchr (path_copy, '\0');
- while (*--p == '.' || *p == ' ')
- continue;
- p[1] = '\0';
- n = 2 + p - path_copy;
+ while (p[-1] == '.' || p[-1] == ' ')
+p--;
+ *p = '\0';
}
+ size_t n = p + 1 - path_copy;
normalized_path = path + sizeof (path) - n;
if (normalized_path > eopath)
@@ -804,6 +801,7 @@ path_conv::check (const char *src, unsig
add_ext_from_sym (sym);
out:
+ bool strip_tail = false;
/* If the user wants a directory, do not return a symlink */
if (!need_directory || error)
/* nothing to do */;
@@ -836,7 +834,10 @@ out:
if (!tail)
/* nothing */;
else if (tail[-1] != '\\')
- *tail = '\0';
+ {
+ *tail = '\0';
+ strip_tail = true;
+ }
else
{
error = ENOENT;
@@ -901,7 +902,7 @@ out:
{
if (tail < path_end && tail > path_copy + 1)
*tail = '/';
- set_normalized_path (path_copy);
+ set_normalized_path (path_copy, strip_tail);
}
#if 0
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
"Pierre A. Humblet" wrote: > > "Pierre A. Humblet" wrote: > > > > When running try_spawn with the snapshot, during the sleep period > > ps reports > > > > 690 443 6902320 11054 10:32:21 > > 464 690 6904640 11054 10:32:21 > > /c/WINNT/system32/notepad > > FWIW, I was thinking about this during lunch. > The basic issue is that the pipe to the parent is not closed in the spawned > Windows process. One way out is to make the pipe non-inheritable and > duplicate it either in the parent (fork and spawn, except detach) > or in the child (exec). Now that subproc_ready is back, it doesn't matter > that an exec'ed Windows process does not duplicate the pipe. Actually, the parent is supposed to disappear anyway in the case of an exec. So it could make the pipe inheritable just before the exec. In multithreaded programs, other threads should be forbidden to fork and spawn once a thread has called exec. Pierre
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
"Pierre A. Humblet" wrote: > > When running try_spawn with the snapshot, during the sleep period > ps reports > > 690 443 6902320 11054 10:32:21 > 464 690 6904640 11054 10:32:21 > /c/WINNT/system32/notepad FWIW, I was thinking about this during lunch. The basic issue is that the pipe to the parent is not closed in the spawned Windows process. One way out is to make the pipe non-inheritable and duplicate it either in the parent (fork and spawn, except detach) or in the child (exec). Now that subproc_ready is back, it doesn't matter that an exec'ed Windows process does not duplicate the pipe. Pierre
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
I tried my spawn(P_DETACH) example (updated since yesterday)
with the latest snapshot, this time on NT.
#include
#include
#include
main()
{
spawnl(_P_DETACH, "/c/WINNT/system32/notepad", "notepad", 0);
printf("Spawn done\n");
/* Keep working */
sleep(10);
printf("Exiting\n");
}
New problem is with gcc (gcc version 3.3.3 (cygwin special))
~/try> uname -a
CYGWIN_NT-4.0 usched40576 1.5.12(0.116/4/2) 2004-11-10 08:34 i686 unknown
unknown Cygwin
~/try> gcc -o try_spawn try_spawn.c
~/try>
~/try> uname -a
CYGWIN_NT-4.0 usched40576 1.5.13s(0.117/4/2) 20041221 16:19:37 i686 unknown
unknown Cygwin
~/try> gcc -o xxx try_spawn.c
In file included from /usr/include/stdio.h:45,
from try_spawn.c:1:
/usr/include/sys/reent.h:810: internal compiler error: Segmentation fault
Please submit a full bug report,
with preprocessed source if appropriate.
See http://gcc.gnu.org/bugs.html> for instructions.
When running try_spawn with the snapshot, during the sleep period
ps reports
690 443 6902320 11054 10:32:21
464 690 6904640 11054 10:32:21
/c/WINNT/system32/notepad
Similarly when running try_spawn in the background,
bash reports "Done" before the job has terminated:
~/try> ./try_spawn.exe &
[1] 740
~/try> Spawn done
[1]+ Done./try_spawn.exe
~/try> Exiting
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
At 12:43 AM 12/4/2004 -0500, you wrote:
>On Thu, Dec 02, 2004 at 09:13:11PM -0500, Pierre A. Humblet wrote:
>>- Non cygwin processes started by cygwin are not shown by ps
>> anymore and cannot be killed.
>>
>>- spawn(P_DETACH) does not work correctly when spawning non-cygwin
>> processes.
> The above are both unintentional and fixable.
Sorry for the slow response, but I still see the second problem.
The following program does not behave the same in 1.5.12 and
CYGWIN_ME-4.90 hpn5170 1.5.13s(0.117/4/2) 20041218 11:42:51
#include
#include
main()
{
spawnl(_P_DETACH, "/c/WINDOWS/notepad", "notepad", 0);
}
Re: Patch to allow trailing dots on managed mounts
Christopher Faylor wrote: > I meant remove the code that I sent here and which I just removed after > Corinna's response. OK. BTW, similar code can also be removed from normalize_win32_path. Pierre
Re: Patch to allow trailing dots on managed mounts
Christopher Faylor wrote:
>
> On Mon, Dec 20, 2004 at 11:18:05AM -0500, Pierre A. Humblet wrote:
> >
> >Christopher Faylor wrote:
> >>
> >> On Mon, Dec 20, 2004 at 10:53:34AM -0500, Pierre A. Humblet wrote:
> >> >Stripping from the Posix path can't be done during normalize_
> >> >because it would apply to all paths (not only disk).
> >>
> >> Why can't we just strip the dots from the path in
> >> path_conv::set_normalized path?
> >
> >You can, after checking the device. But why do it all
> >the time if it's only needed by chdir?
>
> It just seems more consistent and safer to do the same thing to both the
> win32 and posix paths. chroot probably needs it too. After removing
> the code from normalize_posix_path, this is probably not a performance
> hit.
Good points. normalize_posix_path already finds the length, so the
performance hit will be really small.
Not sure what you mean by "removing the code from normalize_posix_path".
It's still important that normalize_{posix,win32}_path strip the final
'.' in "/.", because :check looks for a final /
Pierre
Re: Patch to allow trailing dots on managed mounts
Christopher Faylor wrote: > > On Mon, Dec 20, 2004 at 10:53:34AM -0500, Pierre A. Humblet wrote: > >Stripping from the Posix path can't be done during normalize_ > >because it would apply to all paths (not only disk). > > Why can't we just strip the dots from the path in > path_conv::set_normalized path? You can, after checking the device. But why do it all the time if it's only needed by chdir? Pierre
Re: Patch to allow trailing dots on managed mounts
"Pierre A. Humblet" wrote: > We should also strip win32_cwd there because it will be used > to build an absolute path in normalize_win32_path. Scratch that, sorry. win32_cwd is already stripped. Pierre
Re: Patch to allow trailing dots on managed mounts
Christopher Faylor wrote: > > On Mon, Dec 20, 2004 at 11:23:29AM +0100, Corinna Vinschen wrote: > >On Dec 19 21:57, Pierre A. Humblet wrote: > >> At 09:44 PM 12/18/2004 -0500, Christopher Faylor wrote: > >> > > >> >For now, I'm disallowing all use of '.\' and ' \' in a path. It seems > >> >more consistent to disallow everything than to allow some stuff. I > >> >didn't change the symlink code to disallow "ln -s foo bar..." If someone > >> >actually complains about this, maybe I will. > >> > > >> >So, "ls /bin." works, "ls /bin./pwd.exe" doesn't work and "ls > >> >/cygwin/c/cygwin/bin./pwd.exe" doesn't work either. Nor does > >> >"ls c:\cygwin\bin.\pwd.exe". I don't know if we'll hear complaints about > >> >this one or not. > > > >I guess we will. The trailing dots are not removed from the POSIX path > >in case of chdir, but the chdir itself succeeds. That leads to an > >unexpected result: > > > >$ cd /bin... > >$ pwd > >/bin...<- This was printed as /bin before > >$ ls sh.exe > >ls: sh.exe: No such file or directory > > > >In terms of consistancy it should be impossible to chdir already, > >shouldn't it? > > If we're allowing trailing dots then I guess we should strip them from the > posix path as well as the windows path. chdir should be the only case where this matters. We can either disallow it, or strip the tail. I prefer the latter. Stripping from the Posix path can't be done during normalize_ because it would apply to all paths (not only disk). It's easy to fix posix_cwd at the end of cwdstuff::set, only in the case where "doit" is true. We should also strip win32_cwd there because it will be used to build an absolute path in normalize_win32_path. > >> Do you intent to remove the dot checking code in normalize_xxx_path? > >> It now seems to be useless and even counterproductive. > > > >AFAICS, this code could go. > > We're talking about this code, right? Right. Pierre
Re: Patch to allow trailing dots on managed mounts
At 09:44 PM 12/18/2004 -0500, Christopher Faylor wrote: > >For now, I'm disallowing all use of '.\' and ' \' in a path. It seems >more consistent to disallow everything than to allow some stuff. I >didn't change the symlink code to disallow "ln -s foo bar..." If someone >actually complains about this, maybe I will. > >So, "ls /bin." works, "ls /bin./pwd.exe" doesn't work and "ls >/cygwin/c/cygwin/bin./pwd.exe" doesn't work either. Nor does >"ls c:\cygwin\bin.\pwd.exe". I don't know if we'll hear complaints about >this one or not. Excellent decisions. Although ls would work with 'bin./', NtCreateFile couldn't open the file and things requiring a handle (like the inode) would be reported incorrectly. Do you intent to remove the dot checking code in normalize_xxx_path? It now seems to be useless and even counterproductive. >>Also, for my info, what is the unc\ in >> !strncasematch (this->path + 4, "unc\\", 4))) >>around line 868? I have never seen that documented. > >I've always wondered about that myself. I am pretty sure it predates >me. I've removed that test. It doesn't make any sense to me either. Thanks. I made some progress on that one, looking at path_conv::get_nt_native_path. \\.\unc\computer\share indicates a remote share. So apparently the intention was to add a final \ in that case, but not on \\.\c: (according to the comment) nor on \\.\c:\somedir (why not??), and never with PC_FULL. Is there ever any reason to add a \ to a Windows path? Now, I checked (on XP Home) that "dir \\.\c:" does NOT work, while "dir \\.\c:\" does work, which seems to contradict the intention in the comment. Also "ls //./c:" does not work, but neither does "ls //./c:/", because Cygwin strips the final '/' and it's needed for GetFileAttributes (fails with WinError 87). path_conv will probably never be completely right! Pierre
Re: Patch to allow trailing dots on managed mounts
Christopher Faylor wrote: > While I detest the trailing dot crap, I don't want cygwin to be inconsistent. > I don't want ls /bin./ls.exe to fail but ls /cygdrive/c/bin./ls.exe to work. Assuming a normal install, the first one is c:\cygwin\bin.\ls.exe, which would NOT fail, while the second is c:\bin.\ls.exe, which would fail as expected (not due to dots). You probably mean /usr/bin./ls.exe (fail) vs. /cygdrive/c/cygwin/bin./ls.exe (no fail, although NtCreateFile fails and Cygwin backs off to alternate code). Cygwin has always behaved that way (still does), without generating complaints about inconsistencies. We can't fix Windows, but I don't see why we should add processing to disallow behavior that it allows, or mimic its crazy behavior when we don't have to. > I'm not sure that it makes sense for ln -s foo "bar." to actually create a > file > with a trailing dot on a non-managed mount either. That seems to expose an > implementation detail of the way links are handled and it seems inconsistent > to me. Perhaps, but nobody has complained about it over the years! Look at it positively: Cygwin can implement symbolic link names in a Posix way, without tail dot/space limitations. Ditto with /proc/registry. Actually if one is porting some code that has a hardcoded filename ending with a dot, it's nice to have a simple way (symbolic link to valid Windows name) of making it work. > If we are "fixing" this (I firmly believe that the code in path_conv is never > really going to be right) then I don't want to add inconsistencies. I agree that path_conv is never going to be "right". I would not reduce functionality nor open new holes merely to reduce inconsistencies due to Windows. Would it be better to eliminate the inconsistency by allowing ls /usr/bin./ls.exe (and how many dots? spaces?) or by disallowing ls /cygdrive/c/cygwin/bin./ls.exe (and ls c:/cygwin/bin./ls.exe) ? I can argue either way, with a preference for disallowing (to avoid accidental aliasing, although it breaks precedent, and because on NT, "touch /cygdrive/c/cygwin/bin./ls.exe" fails naturally). Overall I would leave the inconsistency as it is, and blame it on Windows and on Cygwin tradition. There are repeated complaints about /usr/bin/somefile not having the same binary/text mode as /cygdrive/c/cygwin/bin/somefile or c:\cygwin\bin\somefile. We rightly dismiss them, although that can be seen as an inconsistency, and it's purely a Cygwin issue. Pierre
Re: Patch to allow trailing dots on managed mounts
Christopher Faylor wrote: > > On Thu, Dec 16, 2004 at 10:43:47PM -0500, Pierre A. Humblet wrote: > > > >The key point in my patch is that it's the output Win32 path > >that must be checked, not the input path. > > How can that be? As I mentioned previously, if you don't perform the > fixups prior to inspecting the mount table then "ls /bin.." > won't work. Huh? In a normal Cygwin install, /bin... uses the mount point for / and is translated to c:\cygwin\bin... Windows maps this to c:\cygwin\bin, and we can fix the output path in :check() so that NtCreateFile does the same. Note in passing that "ls /bin..." "works" in a rather strange way: CYGWIN_NT-4.0 usched40576 1.5.12(0.116/4/2) 2004-11-10 08:34 i686 ls: /bin.../znew: No such file or directory This has always been the behavior. Continuing on your example, ls /usr/bin... only "works" (as above) since the 2004/04 changes. Before that date, one could mount c:\cygwin\bin -> /usr/bin c:\something -> /usr/bin... and ls /usr/bin... would list c:\something If you try that today, ls /usr/bin... lists c:\cygwin\bin, but ls /usr/bin.../somefile lists c:\something\somefile !!! I think we should revert to the old behavior, i.e. try to be as Posix like as possible. Before 2004/04 one could also have an executable "abc...exe" and typing "abc.." would invoke it. Again I think we should revert to that behavior Yesterday I gave a similar example with symbolic links. They show that the input tail isn't always the same as the output tail. Windows only cares about the output tail, and so should Cygwin (but only when Windows does). Pierre
Re: Patch to allow trailing dots on managed mounts
At 10:27 PM 12/16/2004 -0500, Christopher Faylor wrote: >On Thu, Dec 16, 2004 at 10:26:27PM -0500, Christopher Faylor wrote: >>I don't see how it could be correct for the slash checking code not to >>be "in the loop". Won't this cause a problem if you've done > >Ah, nevermind. I see that your patch handles that. > OK. The key point in my patch is that it's the output Win32 path that must be checked, not the input path. The reason we don't care about the input path is that check() only makes simple Windows calls. They handle the tails as they judge best (and that worked OK until NtCreateFile was introduced), we don't have to do anything special. We want to fix the output path (only for real disk files that are not escaped with //./) so that: 1) NtCreateFile mimics the Windows rules 2) the path hash is invariant to the path tail 3) chdir something/... is prevented Pierre
Re: Patch to allow trailing dots on managed mounts
At 11:06 AM 12/16/2004 -0500, Christopher Faylor wrote:
>On Thu, Dec 16, 2004 at 05:03:22PM +0100, Corinna Vinschen wrote:
>>On Dec 16 10:57, Christopher Faylor wrote:
>>> On Thu, Dec 16, 2004 at 04:53:39PM +0100, Corinna Vinschen wrote:
>>> >Since the mount code is called from path_conv anyway, wouldn't it be
>>> >better to pass the information "managed mount or not" up to path_conv?
>>>
>>> How about just doing the pathname munging in `conv_to_win32_path' if/when
>>> it's needed?
>>
>>Erm... I'm not quite sure, but didn't the "remove trailing dots and spaces"
>>code start there and has been moved to path_conv by Pierre to circumvent
>>some problem? I recall only very vaguely right now.
>
>One problem that it would circumvent is that currently, if you do this:
>
>ls /bin..
>
>You'll get a listing of the bin directory. If you move the code to
>conv_to_win32_path that may not be as easy to get right.
The initial trailing dots and space test was put in normalize_posix path,
not conv_to_win32_path. That was done to fix a side effect of
NtCreateFile, without considering all the many issues.
Putting it in conv_to_win32_path will forbid files ending in .lnk
or .exe but that are called without these suffixes.
This should not happen:
~: ln -s /etc 'abc . .'
~: ls abc*
ls: abc . .: No such file or directory
~: rm 'abc . ..lnk'
rm: remove `abc . ..lnk'? y
It's also called during each iteration of the check() loop, which is
unnecessary.
Putting it in mount_item::build_win32 (as Mark as just done) suffers
from the same problems, and misses a number of cases where it's needed.
The attached patch puts the test at the end of check(), and only if the
file doesn't start with //./
I can't test for the moment due to the state of my sandbox.
I believe that the tests for in normalize_{posix,win32}_path are now
irrelevant, but I'd like Corinna to confirm (she introduced the test
on 2003-10-25).
Due to those tests, suffixes consisting entirely of dots are still
disallowed.
Also, for my info, what is the unc\ in
!strncasematch (this->path + 4, "unc\\", 4)))
around line 868? I have never seen that documented.
Pierre
* path.cc (path_conv::check): Check the output Win32 path for trailing
spaces and dots, not the input path.
Index: path.cc
===
RCS file: /cvs/src/src/winsup/cygwin/path.cc,v
retrieving revision 1.326
diff -u -p -r1.326 path.cc
--- path.cc 3 Dec 2004 02:00:37 - 1.326
+++ path.cc 17 Dec 2004 02:58:57 -
@@ -546,25 +546,12 @@ path_conv::check (const char *src, unsig
/* Detect if the user was looking for a directory. We have to strip
the
trailing slash initially while trying to add extensions but take it
into account during processing */
- if (tail > path_copy + 1)
+ if (tail > path_copy + 1 && isslash (tail[-1]))
{
- if (isslash (tail[-1]))
- {
- need_directory = 1;
- tail--;
- }
- /* Remove trailing dots and spaces which are ignored by Win32
functions but
-not by native NT functions. */
- while (tail[-1] == '.' || tail[-1] == ' ')
- tail--;
- if (tail > path_copy + 1 && isslash (tail[-1]))
- {
- error = ENOENT;
- return;
- }
+ need_directory = 1;
+ *--tail = '\0';
}
path_end = tail;
- *tail = '\0';
/* Scan path_copy from right to left looking either for a symlink
or an actual existing file. If an existing file is found, just
@@ -835,6 +822,18 @@ out:
if (dev.devn == FH_FS)
{
+ if (strncmp (path, ".\\", 4))
+{
+ /* Windows ignores trailing dots and spaces */
+ char *tail = strchr (path, '\0');
+ while (tail[-1] == ' ' || tail[-1] == '.')
+tail[-1] = '\0';
+ if (tail[-1] == '\\')
+{
+ error = ENOENT;
+ return;
+}
+}
if (fs.update (path))
{
debug_printf ("this->path(%s), has_acls(%d)", path, fs.has_acls ());
Re: Patch to allow trailing dots on managed mounts
cgf wrote: > Is it correct to assume that only fhandler_base::open cares about >trailing dots? Good point. This bring back memories. The initial motivation was to fix problems introduced by the use of NtCreateFile http://www.cygwin.com/ml/cygwin/2004-04/msg01250.html and there were successive changes 2004-04-30 Corinna Vinschen <[EMAIL PROTECTED]> * path.cc (normalize_posix_path): Remove trailing dots and spaces. http://cygwin.com/ml/cygwin-patches/2004-q2/msg00053.html 2004-05-06 Pierre Humblet <[EMAIL PROTECTED]> * path.cc (path_conv::check): Strip trailing dots and spaces and return error if the final component had only dots and spaces. (normalize_posix_path): Revert 2004-04-30. However, as a side effect, checking the tail in :check also cleanly fixed longstanding dormant issues with the path hash and with chroot (at least). So checking the tail in fhandler_base::open() is too late. It should be done before exiting :check(), perhaps only in the case where the path refers to a disk file, preferably with little processing overhead. Although it wasn't done before 2004/04, we should also make sure (I have no free time for the moment) that nothing goes wrong inside :check() while we lookup symbolic links. Pierre
Re: Patch to allow trailing dots on managed mounts
Here is an untested patch.
I hope Mark can test it (on managed and unmanaged mounts,
including basenames consisting entirely of dots and spaces)
and possibly make adjustments, without having to file the
paperwork.
Pierre
* path.cc (path_conv::check): Do not strip trailing dots and spaces.
* fhandler.cc (fhandler_base::open): Strip trailing dots and spaces.
Index: path.cc
===
RCS file: /cvs/src/src/winsup/cygwin/path.cc,v
retrieving revision 1.326
diff -u -p -r1.326 path.cc
--- path.cc 3 Dec 2004 02:00:37 - 1.326
+++ path.cc 16 Dec 2004 14:42:58 -
@@ -546,25 +546,12 @@ path_conv::check (const char *src, unsig
/* Detect if the user was looking for a directory. We have to strip the
trailing slash initially while trying to add extensions but take it
into account during processing */
- if (tail > path_copy + 1)
+ if (tail > path_copy + 1 && isslash (tail[-1]))
{
- if (isslash (tail[-1]))
- {
- need_directory = 1;
- tail--;
- }
- /* Remove trailing dots and spaces which are ignored by Win32
functions but
-not by native NT functions. */
- while (tail[-1] == '.' || tail[-1] == ' ')
- tail--;
- if (tail > path_copy + 1 && isslash (tail[-1]))
- {
- error = ENOENT;
- return;
- }
+ need_directory = 1;
+ *--tail = '\0';
}
path_end = tail;
- *tail = '\0';
/* Scan path_copy from right to left looking either for a symlink
or an actual existing file. If an existing file is found, just
Index: fhandler.cc
===
RCS file: /cvs/src/src/winsup/cygwin/fhandler.cc,v
retrieving revision 1.207
diff -u -p -r1.207 fhandler.cc
--- fhandler.cc 20 Nov 2004 23:42:36 - 1.207
+++ fhandler.cc 16 Dec 2004 14:43:51 -
@@ -537,6 +537,17 @@ fhandler_base::open (int flags, mode_t m
UNICODE_STRING upath = {0, sizeof (wpath), wpath};
pc.get_nt_native_path (upath);
+ /* Remove trailing dots and spaces which are ignored by Win32 functions but
+ not by native NT functions. */
+ WCHAR *tail = upath.Buffer + upath.Length;
+ while (tail[-1] == '.' || tail[-1] == ' ')
+tail--;
+ if (tail[-1] == '\\')
+{
+ set_errno (ENOENT);
+ return 0;
+}
+
if (RtlIsDosDeviceName_U (upath.Buffer))
return fhandler_base::open_9x (flags, mode);
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
Christopher Faylor wrote: > > With the current CVS, I am seeing the same (suboptimal) behavior on > Windows Me that I do in 1.5.12. > > If I type a bunch of "sleep 60&" at the command line, then "bash" won't > exit until the last sleep 60 has terminated. I can't explain why this > is. It doesn't work that way on XP, of course. > > While "bash" is waiting, I see no sign of it in the process table so > it's odd behavior. 1.5.12, the current official release? I have never observed it there. Also my recollection is that the delay was not necessarily equal to the sleep duration. > The current CVS should work better with exim now, though. Are you done with the changes? I will try a snapshot and look at the code in the coming days. Not much free time currently. Pierre
Re: [Fwd: [que_andrewBOOHyahoo.com: FOLLOWUP: 1.5.12: problems without registry keys]]
Corinna Vinschen wrote: > Is that ok to apply or is there any good reason not to release the muto > when get_drive() has finished? I can't see any, FWIW. Oops, please apply ASAP of course. Pierre
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
At 12:33 PM 12/4/2004 -0500, Christopher Faylor wrote:
>On Sat, Dec 04, 2004 at 11:45:28AM -0500, Pierre A. Humblet wrote:
>>At 12:43 AM 12/4/2004 -0500, Christopher Faylor wrote:
>>>I wrote a simple test case to check this and I don't see it -- on XP. I
>>>can't easily run Me anymore. Does the attached program demonstrate this
>>>behavior when you run it? It should re-exec itself every time you hit
>>>CTRL-C.
>>
>>That test case has no problem, but the attached one does.
>>Use kill -30 pid
>
>Sigh. Works fine on XP, AFAICT.
More details
CYGWIN_ME-4.90 hpn5170 1.5.13s(0.116/4/2) 20041125 23:34:52 i686 unknown
unknown Cygwin
I added a printf at the top, showing the current pid and ppid
(attached)
~: ./a
pid 556021 ppid 890585
~: ps | fgrep /A
36793321 1 556021 42581739750 740 12:47:22 /c/HOME/PIERRE/A
~: kill -30 36793321
got signal 30
execing myself
~: pid 36793321 ppid 36793321
~: ps | fgrep /A
36765717 1 556021 42582015790 740 12:47:44 /c/HOME/PIERRE/A
The problem is that the execed process has itself as ppid.
So it forks again.
That must be history by now, but I think it's coming from
if (!myself->wr_proc_pipe)
{
myself.hProcess = pi.hProcess;
myself.remember ();
wait_for_myself = true;
}
with wr_proc_pipe having been reset to NULL.
Pierre
#include
#include
#include
#include
void ouch (int sig)
{
printf ("got signal %d\n", sig);
return;
}
int
main (int argc, char **argv)
{
printf("pid %d ppid %d\n", getpid(), getppid());
if (getppid() != 1 && fork())
exit(0);
signal (SIGUSR1, ouch);
while (pause ())
{
puts ("execing myself");
execv (argv[0], argv);
}
exit (0);
}
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
At 12:43 AM 12/4/2004 -0500, Christopher Faylor wrote:
>
>I wrote a simple test case to check this and I don't see it -- on XP. I
>can't easily run Me anymore. Does the attached program demonstrate this
>behavior when you run it? It should re-exec itself every time you hit
>CTRL-C.
That test case has no problem, but the attached one does.
Use kill -30 pid
Pierre
#include
#include
#include
#include
void ouch (int sig)
{
printf ("got signal %d\n", sig);
return;
}
int
main (int argc, char **argv)
{
if (getppid() != 1 && fork())
exit(0);
signal (SIGUSR1, ouch);
while (pause ())
{
puts ("execing myself");
execv (argv[0], argv);
}
exit (0);
}
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
At 11:29 PM 11/25/2004 -0500, Christopher Faylor wrote: >On Sat, Nov 20, 2004 at 01:23:39AM -0500, Christopher Faylor wrote: >>On Tue, Nov 16, 2004 at 10:56:40AM -0500, Christopher Faylor wrote: >>>The simplification of the code from removing all of the reparenting >>>considerations is not something that I'm going to give up on easily. >> >>Well, the code seems to be slightly faster now than the old method, so >>that's something. I think it's also a lot simpler. > >I've checked in my revamp of the exec/wait code. There are still some >other ways to do what I did and maybe I'll experiment with using >multiple threads running WaitForMultipleObjects, but, for now, cygwin is >using the one thread per process technique. I have downloaded and run a recent snapshot on WinME, CYGWIN_ME-4.90 hpn5170 1.5.13s(0.116/4/2) 20041125 23:34:52 i686 unknown unknown Cygwin and tried a few things. I have also gone through the diff. Here are my initial comments: - Non cygwin processes started by cygwin are not shown by ps anymore and cannot be killed. - spawn(P_DETACH) does not work correctly when spawning non-cygwin processes. This is due to using a pipe to detect process termination. - >AFAIK, the only problem with the current code is if a parent process >forks a process, calls setuid, and execs a non-cygwin process it is >possible that the parent process won't be able to retrieve the exit >value of the non-cygwin process. I assume you are referring to the use of OpenProcess to reparent, instead of duplicating the child process handle. This patch uses very innovative methods, but IMHO the net result goes against Cygwin tradition. It decreases features (the support of non-cygwin processes) to simplify code. In fact there are many design issues and choices that have been lumped together. They can be separated, at least partially, and discussed individually. 1) parent getting handle to child: A) child duplicates handle [security issue] B) parent duplicates handle C) parent opens process [access issue] 2) child termination detection A) WaitFor(child process) B) pipe [problem spawning non-cygwin processes] C) Windows select and socket [like pipes, but untested & risky] 3) number of waiting threads A) one thread per 63 processes [WaitFor only] B) one thread per process [pipe or WaitFor] C) one thread for all processes [perhaps..., with select] 4) communication with parent A) common sig pipe B) process termination detection pipe 5) support for non-cygwin processes A) subproc_ready event B) don't support 6) reporting exit status A) GetExitCodeProcess only B) pinfo->exit_status + fallback with GetExitCodeProcess Here is my analysis and recommendation for the 6 issues: 1) Use B), it has no drawback 2) Use A), it has no drawback, although B is tempting due to its simplicity. Perhaps worth more than spawn(P_DETACH). 3) Which one is faster? 4) Dictated by choice for 2) 5) Support it. 6) Use B), it must be slightly faster. Other points: - The on demand creation of the pid_handles is a good idea - The name of the waiting threads should not be "sig" - opening the pinfo of ppid in set_myself(), just to close parent->wr_proc_pipe, looks simple but is costly. I understand that it's nicer than closing the handle in dcrt0.cc (from the childinfo). A middle ground would be to have a ppid_wr_proc_pipe in the child pinfo. - I did not expect that you would always start the new process in suspended state due to reparenting. This is likely to degrade speed. It can be avoided without increase in code complexity. - I think there is a race condition during spawn. The logical parent can terminate just after the SIGREPARENT signal has been sent, causing an infinite loop in. if (exec_cygstarted) while (myself->cygstarted == exec_cygstarted) low_priority_sleep (0); The same issue was addressed (with a detailed comment) in my initial patch in this thread. while (myself->isreparenting && myself->ppid != 1 && my_parent_is_alive ()) low_priority_sleep (0); - It is said in spawn_guts; "/* If wr_proc_pipe doesn't exist then this process was not started by a cygwin process. " but in pinfo::alert_parent() the pipe is set to NULL when the cygwin parent has disappeared. - Re-execing does not always work properly. This can be demonstrated with exim running as daemon (exim -bd). Note that it was started from cygwin but ppid = 1. BEFORE ~: ps -a | fgrep EXIM 159801 1 159801 4294807495? 400 00:32:08 /c/PROGRAM FILES/CYGNUS/BIN/EXIM-4.43-2 ~: kill -HUP 159801 [This causes re-execing] ~: ps -a | fgrep EXIM 159801 1 159801 4294856995? 400 19:16:32 /c/PROGRAM FILES/CYGNUS/BIN/EXIM-4.43-2 Note the new winpid, but same cygpid AFTER ~: ps | fgrep -i exim 1109249 1 1109249
Re: [Patch] Loading the registry hive on Win9x (part 2)
At 10:25 AM 11/22/2004 -0500, Christopher Faylor wrote: > >Other than that, the change looks fine. Done Pierre
Re: [Patch] Loading the registry hive on Win9x (part 2)
Christopher Faylor wrote:
>
> On Sun, Nov 21, 2004 at 09:55:38PM -0500, Pierre A. Humblet wrote:
> >- got_something_from_registry = regopt ("default");
> > if (myself->progname[0])
> >-got_something_from_registry = regopt (myself->progname) ||
> >got_something_from_registry;
> >+got_something_from_registry = regopt (myself->progname);
> >+ got_something_from_registry = got_something_from_registry || regopt
> >("default");
>
> Doesn't this change the sense of the "default" key so that it will never
> get used if a key exists for myself->progname rather than always get
> used, regardless? Maybe I'm the only person in the world who relies on
> that behavior, but I do rely on it.
Hmm, I thought that what went on before was that the "default"
key was always read, but that it was overwritten if the other key
existed. Is it the case that there is no complete overwriting,
it's the union that counts? If so, I will put it back that way.
I didn't know that every program was trying to read 4 items in the
registry. Wouldn't it make sense to keep inheritable keys to the Cygwin
registry branches on the cygheap, instead of walking down the hierarchy
four times?
By the way, perhaps others in the world would also find that feature
useful, but AFAIK it's a well kept secret. FAQ or users' guide alert?
Pierre
[Patch] Loading the registry hive on Win9x (part 2)
This is the second part of the patch to also load the registry hive
on Win9x during seteuid, and to apply the method recommended in
MS KB 199190 to avoid using HKCU.
The main change is a new reg_key constructor that does not use
HKCU and that can also use HKLM.
There are collateral changes in path.cc, which is now 50 lines
shorter, as well as in environ.cc and shared.cc.
The two cygdrive bugs shown below are fixed.
~: mount -p
Prefix Type Flags
~: umount -c
umount: remove_cygdrive_prefix: No error<=== NOT RIGHT
~: ls -ld /cygdrive/c
drwxr-xr-x 14 pierre all 0 Dec 31 1969 /cygdrive/c/
~: mount -c /xyz
~: mount -p
Prefix Type Flags
/xyzsystem binmode
~: ls -ld /xyz/c
ls: /xyz/c: No such file or directory < NOT RIGHT
Pierre
2004-11-22 Pierre Humblet <[EMAIL PROTECTED]>
* registry.h (reg_key::reg_key): Change arguments.
* shared_info.h (class mount_info): Remove had_to_create_mount_areas.
* registry.cc (reg_key::reg_key): Change constructors to always handle
HKLM and to avoid relying on HKCU.
Do not set mount_table->had_to_create_mount_areas.
* path.cc (mount_info::conv_to_win32_path): Improve update of
sys_mount_table_counter.
(mount_info::read_mounts): Use new reg_key constructor.
(mount_info::add_reg_mount): Ditto.
(mount_info::del_reg_mount): Ditto.
(mount_info::read_cygdrive_info_from_registry): Ditto.
(mount_info::write_cygdrive_info_to_registry): Ditto.
Update cygwin_shared->sys_mount_table_counter after registry update.
(mount_info::get_cygdrive_info): Ditto.
* shared.cc (shared_info::heap_chunk_size): Use new reg_key constructor.
* environ.cc (regopt): Ditto.
(environ_init): Optimize calls to regopt.? debug.diff
? hive2.diff
? hive3.diff
? syscalls.cc.diff
Index: environ.cc
===
RCS file: /cvs/src/src/winsup/cygwin/environ.cc,v
retrieving revision 1.102
diff -u -p -r1.102 environ.cc
--- environ.cc 7 Oct 2004 21:28:57 - 1.102
+++ environ.cc 21 Nov 2004 21:02:21 -
@@ -640,29 +640,22 @@ static bool __stdcall
regopt (const char *name)
{
bool parsed_something = false;
- /* FIXME: should not be under mount */
- reg_key r (KEY_READ, CYGWIN_INFO_PROGRAM_OPTIONS_NAME, NULL);
char buf[CYG_MAX_PATH];
char lname[strlen (name) + 1];
strlwr (strcpy (lname, name));
- if (r.get_string (lname, buf, sizeof (buf) - 1, "") == ERROR_SUCCESS)
+ for (int i = 0; i < 2; i++)
{
- parse_options (buf);
- parsed_something = true;
-}
- else
-{
- reg_key r1 (HKEY_LOCAL_MACHINE, KEY_READ, "SOFTWARE",
- CYGWIN_INFO_CYGNUS_REGISTRY_NAME,
- CYGWIN_INFO_CYGWIN_REGISTRY_NAME,
- CYGWIN_INFO_PROGRAM_OPTIONS_NAME, NULL);
- if (r1.get_string (lname, buf, sizeof (buf) - 1, "") == ERROR_SUCCESS)
+ reg_key r (i, KEY_READ, CYGWIN_INFO_PROGRAM_OPTIONS_NAME, NULL);
+
+ if (r.get_string (lname, buf, sizeof (buf) - 1, "") == ERROR_SUCCESS)
{
parse_options (buf);
parsed_something = true;
+ break;
}
}
+
MALLOC_CHECK;
return parsed_something;
}
@@ -678,7 +671,7 @@ environ_init (char **envp, int envc)
char *newp;
int sawTERM = 0;
bool envp_passed_in;
- bool got_something_from_registry;
+ bool got_something_from_registry = false;
static char NO_COPY cygterm[] = "TERM=cygwin";
static int initted;
@@ -692,9 +685,9 @@ environ_init (char **envp, int envc)
initted = 1;
}
- got_something_from_registry = regopt ("default");
if (myself->progname[0])
-got_something_from_registry = regopt (myself->progname) ||
got_something_from_registry;
+got_something_from_registry = regopt (myself->progname);
+ got_something_from_registry = got_something_from_registry || regopt
("default");
/* Set ntsec explicit as default, if NT is running */
if (wincap.has_security ())
Index: path.cc
===
RCS file: /cvs/src/src/winsup/cygwin/path.cc,v
retrieving revision 1.325
diff -u -p -r1.325 path.cc
--- path.cc 28 Oct 2004 01:46:01 - 1.325
+++ path.cc 21 Nov 2004 21:02:27 -
@@ -120,8 +120,6 @@ create_shortcut_header (void)
}
}
-#define CYGWIN_REGNAME (cygheap->cygwin_regname ?:
CYGWIN_INFO_CYGWIN_REGISTRY_NAME)
-
/* Determine if path prefix matches current cygdrive */
#define iscygdrive(path) \
(path_prefix_p (mount_table->cygdrive, (path), mount_table->cygdrive_len))
@@ -1400,8 +1398,9 @@ mount_info::conv_to_win32_path (const ch
bool chroot_ok = !cygheap->root.exists ();
while (sys_mount_table_counter < cygwin_shared->sys_mount_table_counter)
{
+ int current = cygwin_shared->sys_mount_table_counter;
init ();
- sys_mount_table_cou
Re: [Patch] debug_printf edits
At 02:36 PM 11/20/2004 -0500, Christopher Faylor wrote: >On Sat, Nov 20, 2004 at 02:27:37PM -0500, Pierre A. Humblet wrote: >>At 02:20 PM 11/20/2004 -0500, Christopher Faylor wrote: >>>On Sat, Nov 20, 2004 at 01:51:16PM -0500, Pierre A. Humblet wrote: >>>>Here are minor changes that facilitate grepping traces. >>>> >>>>Pierre >>>> >>>>2004-11-20 Pierre Humblet <[EMAIL PROTECTED]> >>>> >>>>* fhandler.cc (fhandler::write): Remove debug_printf. >>>>* pipe.cc (fhandler_pipe::create): Edit syscall_printf format. >>> >>>What does the inclusion of the word "pipe" add for grepping traces? >>>So, you can do a grep on '[0-9]+ = [a-z]'? >> >>Most syscall_printf have the form "res = function (args)", >>but the one in pipe.cc is missing "function". >>So the change facilitates grepping, in particular when looking >>for many functions at once. > >It didn't make sense to me that you were removing this usage in the case >of fhandler::write but tweaking it in the case of fhandler_pipe::create. The debug_printf in fhandler::write is redundant because there is an equivalent syscall_printf in write() (and a simple grep catches both). Note that there is no debug_printf in many other fhandler_xxx::write. Alternatively the debug_printf could be changed to avoid looking like a syscall_printf (that's what's happening in fhandler::read). Strictly speaking the syscall_printf in fhandler_pipe::create should be removed, and reintroduced in both pipe and _pipe, but I thought this would be going overboard. Pierre
Re: [Patch] debug_printf edits
At 02:20 PM 11/20/2004 -0500, Christopher Faylor wrote: >On Sat, Nov 20, 2004 at 01:51:16PM -0500, Pierre A. Humblet wrote: >>Here are minor changes that facilitate grepping traces. >> >>Pierre >> >>2004-11-20 Pierre Humblet <[EMAIL PROTECTED]> >> >> * fhandler.cc (fhandler::write): Remove debug_printf. >> * pipe.cc (fhandler_pipe::create): Edit syscall_printf format. > >What does the inclusion of the word "pipe" add for grepping traces? >So, you can do a grep on '[0-9]+ = [a-z]'? Most syscall_printf have the form "res = function (args)", but the one in pipe.cc is missing "function". So the change facilitates grepping, in particular when looking for many functions at once. Pierre
[Patch] debug_printf edits
Here are minor changes that facilitate grepping traces.
Pierre
2004-11-20 Pierre Humblet <[EMAIL PROTECTED]>
* fhandler.cc (fhandler::write): Remove debug_printf.
* pipe.cc (fhandler_pipe::create): Edit syscall_printf format.
Index: fhandler.cc
===
RCS file: /cvs/src/src/winsup/cygwin/fhandler.cc,v
retrieving revision 1.206
diff -u -p -r1.206 fhandler.cc
--- fhandler.cc 12 Sep 2004 03:47:56 - 1.206
+++ fhandler.cc 20 Nov 2004 18:52:33 -
@@ -914,7 +914,6 @@ fhandler_base::write (const void *ptr, s
}
}
- debug_printf ("%d = write (%p, %d)", res, ptr, len);
return res;
}
Index: pipe.cc
===
RCS file: /cvs/src/src/winsup/cygwin/pipe.cc,v
retrieving revision 1.64
diff -u -p -r1.64 pipe.cc
--- pipe.cc 12 Sep 2004 03:47:56 - 1.64
+++ pipe.cc 20 Nov 2004 18:52:33 -
@@ -380,7 +380,7 @@ fhandler_pipe::create (fhandler_pipe *fh
}
}
- syscall_printf ("%d = ([%p, %p], %d, %p)", res, fhs[0], fhs[1], psize,
mode);
+ syscall_printf ("%d = pipe ([%p, %p], %d, %p)", res, fhs[0], fhs[1],
psize, mode);
return res;
}
Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.
At 01:23 AM 11/20/2004 -0500, Christopher Faylor wrote:
>Here's the good news/bad news.
>
>On Tue, Nov 16, 2004 at 10:56:40AM -0500, Christopher Faylor wrote:
>>The simplification of the code from removing all of the reparenting
>>considerations is not something that I'm going to give up on easily.
>
>Well, the code seems to be slightly faster now than the old method,
>so that's something. I think it's also a lot simpler.
>
>There are some ancillary benefits of this new approach. I've fixed the
>old problem where if you run a process from a windows command prompt and
>that process execs another process and it execs another process, each
>process will wait around into the final process in the chain dies.
>
>I've also added an 'exitcode' field to _pinfo so that a Cygwin process
>will set the error code in a UNIX fashion based on whether it is exiting
>due to a signal or with a normal exit(). Unfortunately, this means that
>I don't know quite what to do with exit codes from Windows processes.
>This is the last remaining problem before I check things in. This
>problem just occurred to me as I was typing in the ChangeLog and it may
>be the one reason why you actually need to do the reparenting tango.
For Windows process, why can't you keep doing what was done before
case WAIT_OBJECT_0:
sigproc_printf ("subprocess exited");
DWORD exitcode;
if (!GetExitCodeProcess (pi.hProcess, &exitcode))
exitcode = 1;
and copy the Windows exit code to the exitcode field? Or did you remove the
subproc_ready as well?
Pierre
