Re: /proc//{cwd, root} links to for cygrunsrv, daemons, and shells
On Apr 9 10:38, Brian Inglis wrote: > On 2024-04-09 07:08, Corinna Vinschen wrote: > > That's typically a permission problem. On Linux you get something like > > > >ls: cannot read symbolic link '/proc/1/cwd': Permission denied > > Thanks Corinna, > > That now makes sense, as Cygwin ps -a and btop showed the processes, > although procps and top did not, and other info is visible, I never thought > about permissions as there were links, but I see from elevated admin sh: > [...] > so I think perms on these should be 440 or 550 not 444 or 555, but that may > involve a lot of work to decide that for each entry? Not really. Have a look into fhandler/proc.cc, fhandler/process.cc, etc. We can add a permisions member to struct virt_tab_t and add this as static info to every member in the list. Doesn't sound overly complicated to me (*nudge, nudge*). Changing to a "Permission denied" when trying to open a virtual symlink may be a bit more involved, but maybe not very much. Corinna
Re: /proc//{cwd, root} links to for cygrunsrv, daemons, and shells
On Apr 7 13:34, Brian Inglis wrote:
> ISTM anomalous that for cygrunsrv, daemons, cron processes, and shells
> /proc//{cwd,root} have bad symlinks to "", normally a process
> or exe status:
>
> /proc/732/exe -> /usr/bin/cygrunsrv
> /proc/732/root ->
> /proc/732/cwd ->
> | /proc/733/exe -> /usr/sbin/cygserver
> ->/proc/733/root ->
>/proc/733/cwd ->
> /proc/740/exe -> /usr/bin/cygrunsrv
> /proc/740/root ->
> /proc/740/cwd ->
> | /proc/741/exe -> /usr/sbin/syslog-ng
> ->/proc/741/root ->
>/proc/741/cwd ->
> /proc/748/exe -> /usr/bin/cygrunsrv
> /proc/748/root ->
> /proc/748/cwd ->
> | /proc/749/exe -> /usr/sbin/cron
> ->/proc/749/root ->
>/proc/749/cwd ->
>| /proc/2080/exe -> /usr/sbin/cron
> ->/proc/2080/root ->
> /proc/2080/cwd ->
> | /proc/2082/exe -> /usr/bin/bash
>->/proc/2082/root ->
> /proc/2082/cwd ->
>
> Should we consider changing that to root "/", or nothing, null, or something
> meaningful?
That's typically a permission problem. On Linux you get something like
ls: cannot read symbolic link '/proc/1/cwd': Permission denied
But on Cygwin the content of those links require to open the processes'
signal pipe and send/receive a message containing the information. I
didn't look into the code for a while but it seems we don't check why we
couldn't connect to a process to fetch the info. IIRC the current
fhandler_process framework doesn't have a way to communicate that
info.
If you want to change that, feel free!
Corinna
Re: [PATCH] winsup/cygwin/fhandler/proc.cc: format_proc_cpuinfo() Linux 6.8 cpuinfo flags
On Mar 18 11:21, Brian Inglis wrote: > On 2024-03-18 09:45, Corinna Vinschen wrote: > > I see. I just don't understands the difference between, say, > > > >ftcprint (features1, 21, "avx512ifma"); /* vec int FMA */ > > + /* ftcprint (features1, 22, ""); */ /* unused */ > >ftcprint (features1, 23, "clflushopt"); /* cache line flush opt */ > > > > and > > > >ftcprint (features1, 3, "xsaves"); /* xsaves/xrstors */ > > + /* ftcprint (features1, 4, "xfd"); */ /* eXtended Feature Disabling */ > > > > The latter makes sense, of course, but why is the first comment "unused", > > rather than something like "PCOMMIT instruction" as in the cpuid output? > > > > Note that I'm not saying that you have to change that, but I would like > > to understand it. > > Hi Corinna, > > The cpuid output is not always up to date with the kernel, and there are a > lot of bits defined, so if Linux does not use the bit I will not mention it, > except where uses and visibility may vary because of merge/patch revisions, > as happened recently with shstk and lam handling changes. > > The cpuinfo capflags are generated by running the Linux build script > mkcapflags.sh, from the feature symbol suffix, unless overriden by a quoted > string at the start of the comment, and "" suppresses cpuinfo flag output. > > In my weekly pulls of relevant rc sources, I generate a couple of summary > logs to merge the cpuinfo capflags with the comments and feature bits, and > diff everything relevant vs the previous tagged release. > > I keep an eye on those diffs, and when the next release is no longer a > candidate, I pull up the Linux changes and look at how they can be added to > Cygwin. > > I sometimes add features commented out to document bits used in a feature > word, but not yet displayed on cpuinfo, just to make it easier to compare > with Linux, or more obvious that an unused bit has not been missed. > The latest additions are the result of uncertainties raised during my last > cross check. > > Below is a sample of the info used to display Linux cpuinfo flags, which I > use to support Cygwin's, relevant to those you mentioned. > Linux feature word 9 bit 22 is unused, and word 10 bit 4 is not displayed. > [...] > Many of the synthetic Linux features and flags are derived from hw boot or > MSR info, which we can not yet access from Cygwin, so I ignore those > changes, unless the feature can be derived from information readily > available as a user in the cpu, Windows, or Cygwin. > > I cross check the Linux and Cygwin sources occasionally to ensure I have not > missed anything added or removed, spelling changes, Linux tweaks, or > readability. > > I have so far ignored feature disabling depending on conditions, and cpu > errata checks and output, as some of that requires MSR info or low level > access. > > I have looked at trying to extract or generate tables from the Linux sources > to drive our cpuinfo, use gcc cpuid and cpuinfo headers, automate or at > least simplify maintenance, but there are many exceptions which we can not > determine to output, and Intel's practices are not as architecturally > structured as AMD's, so require code to decide.] Thanks for the explanation. Pushed. Corinna
Re: [PATCH] winsup/cygwin/fhandler/proc.cc: format_proc_cpuinfo() Linux 6.8 cpuinfo flags
On Mar 18 08:10, Brian Inglis wrote: > On 2024-03-18 03:33, Corinna Vinschen wrote: > > Hi Brian, > > > > On Mar 16 10:44, Brian Inglis wrote: > > > add Linux 6.8 cpuinfo flags: > > > Intel 0x0007:1 eax:17 fredFlexible Return and Event > > > Delivery; > > > AMD 0x801f eax:4 sev_snp SEV secure nested paging; > > > document unused and some unprinted bits that could look like omissions; > > > fix typos and misalignments; > > > > I'm a bit puzzled about the "unused" slots. You're adding them > > only in some places. What makes them "look like omissions"? > > Mainly because single bits are omitted, presumably because they do not want > to pollute the symbol space with as yet unused features, just as they do not > output all features as cpuinfo flags, until it indicates something about the > build and/or system. > > Compare the minimal common standard feature bits defined in the gcc lib > cpuid.h and gcc cpuinfo.h headers, with Linux cpuinfo cpufeatures.h, and the > output of the cpuid utility, where almost all bits in older cpuid entries > are defined. I see. I just don't understands the difference between, say, ftcprint (features1, 21, "avx512ifma"); /* vec int FMA */ + /* ftcprint (features1, 22, ""); */ /* unused */ ftcprint (features1, 23, "clflushopt"); /* cache line flush opt */ and ftcprint (features1, 3, "xsaves"); /* xsaves/xrstors */ + /* ftcprint (features1, 4, "xfd"); */ /* eXtended Feature Disabling */ The latter makes sense, of course, but why is the first comment "unused", rather than something like "PCOMMIT instruction" as in the cpuid output? Note that I'm not saying that you have to change that, but I would like to understand it. Thanks, Corinna
Re: [PATCH] winsup/cygwin/fhandler/proc.cc: format_proc_cpuinfo() Linux 6.8 cpuinfo flags
Hi Brian, On Mar 16 10:44, Brian Inglis wrote: > add Linux 6.8 cpuinfo flags: > Intel 0x0007:1 eax:17 fredFlexible Return and Event > Delivery; > AMD 0x801f eax:4 sev_snp SEV secure nested paging; > document unused and some unprinted bits that could look like omissions; > fix typos and misalignments; I'm a bit puzzled about the "unused" slots. You're adding them only in some places. What makes them "look like omissions"? Thanks, Corinna
Re: newlib-cygwin build fails on dumper
On Mar 14 10:10, Brian Inglis wrote:
> Hi folks,
>
> During newlib-cygwin build to test patches, with latest current stable
> packages as of last weekend, and yesterday's repo main/master, get a
> warning, then errors building dumper:
>
> ...
>
> CC libc/reent/libc_a-getentropyr.o
> /usr/src/newlib-cygwin/newlib/libc/reent/getentropyr.c: In function
> ‘_getentropy_r’:
> /usr/src/newlib-cygwin/newlib/libc/reent/getentropyr.c:48:14: warning:
> implicit declaration of function ‘_getentropy’; did you mean ‘getentropy’?
> [-Wimplicit-function-declaration]
>48 | if ((ret = _getentropy (buf, buflen)) == -1 && errno != 0)
> | ^~~
> | getentropy
>
> ...
>
> CXX dumper-dumper.o
> In file included from /usr/src/newlib-cygwin/winsup/utils/dumper.cc:23:
> /usr/include/bfd.h:2748:1: error: expected initializer before
> ‘ATTRIBUTE_WARN_UNUSED_RESULT’
> 2748 | ATTRIBUTE_WARN_UNUSED_RESULT;
> | ^~~~
> /usr/include/bfd.h:2751:1: error: expected initializer before
I'm pretty sure this isn't the latest newlib-cygwin main.
The getentropy warning has been fixed on 2023-09-25 by commit
a9e8e3d1cb82 ("newlib: Add missing prototype for _getentropy")
The ATTRIBUTE_WARN_UNUSED_RESULT error message has been fixed on
2024-02-12 by commit 10c8c1cf4f94 ("include/ansidecl.h: import from
binutils-gdb")
Corinna
Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.
On Mar 12 08:17, Takashi Yano wrote:
> Subject: [PATCH v4] Cygwin: pipe: Make sure to set read pipe non-blocking for
> cygwin apps.
>
> If pipe reader is a non-cygwin app first, and cygwin process reads
> the same pipe after that, the pipe has been set to bclocking mode
> for the cygwin app. However, the commit 9e4d308cd592 assumes the
> pipe for cygwin process always is non-blocking mode. With this patch,
> the pipe mode is reset to non-blocking when cygwin app is started.
>
> Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255644.html
> Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag
> for read pipe.")
> Reported-by: wh
> Reviewed-by: Corinna Vinschen
> Signed-off-by: Takashi Yano
> ---
> winsup/cygwin/fhandler/pipe.cc | 63 +
> winsup/cygwin/local_includes/fhandler.h | 3 ++
> winsup/cygwin/spawn.cc | 34 +
> 3 files changed, 68 insertions(+), 32 deletions(-)
Looks good, please push.
Thanks,
Corinna
Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.
Hi Takashi,
this looks much better. Just one question and a few comment
changes...
On Mar 11 22:18, Takashi Yano wrote:
> Subject: [PATCH v2] Cygwin: pipe: Make sure to set read pipe non-blocking for
> cygwin apps.
>
> If pipe reader is a non-cygwin app first, and cygwin process reads
> the same pipe after that, the pipe has been set to bclocking mode
> for the cygwin app. However, the commit 9e4d308cd592 assumes the
> pipe for cygwin process always is non-blocking mode. With this patch,
> the pipe mode is reset to non-blocking when cygwin app is started.
>
> Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255644.html
> Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag
> for read pipe.")
> Reported-by: wh
> Reviewed-by: Corinna Vinschen
> Signed-off-by: Takashi Yano
> ---
> winsup/cygwin/fhandler/pipe.cc | 54 +
> winsup/cygwin/local_includes/fhandler.h | 2 +
> winsup/cygwin/spawn.cc | 35 +---
> 3 files changed, 58 insertions(+), 33 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> index 29d3b41d9..b29726dcb 100644
> --- a/winsup/cygwin/fhandler/pipe.cc
> +++ b/winsup/cygwin/fhandler/pipe.cc
> @@ -18,6 +18,7 @@ details. */
> #include "pinfo.h"
> #include "shared_info.h"
> #include "tls_pbuf.h"
> +#include "sigproc.h"
> #include
>
> /* This is only to be used for writing. When reading,
> @@ -1288,3 +1289,56 @@ close_proc:
> }
>return NULL;
> }
> +
> +void
> +fhandler_pipe::spawn_worker (bool iscygwin, int fileno_stdin,
> + int fileno_stdout, int fileno_stderr)
> +{
> + bool need_send_noncygchld_sig = false;
> +
> + /* Set read pipe itself always non-blocking for cygwin process. blocking/
> + non-blocking is simulated in the raw_read(). As for write pipe, follow
> + the is_nonblocking(). */
You can drop the articles here, i.e.
...non-blocking is simulated in raw_read(). For write pipe follow
is_nonblocking().
> + int fd;
> + cygheap_fdenum cfd (false);
> + while ((fd = cfd.next ()) >= 0)
> +if (cfd->get_dev () == FH_PIPEW
> + && (fd == fileno_stdout || fd == fileno_stderr))
> + {
> + fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> + bool mode = iscygwin ? pipe->is_nonblocking () : false;
> + pipe->set_pipe_non_blocking (mode);
What bugs me here is that we now run the loop for cygwin children
as well. The old code only did that for non-cygwin children.
This looks like quite a performance hit, potentially. Especially
if the process has many open descriptors. Let's say, a recursive
make or so. Did you find this is necessary? No way to avoid that?
> +
> + /* Setup for query_ndl stuff. Read the comment below. */
> + if (!iscygwin && pipe->request_close_query_hdl ())
> + need_send_noncygchld_sig = true;
> + }
> +else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin)
> + {
> + fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> + pipe->set_pipe_non_blocking (iscygwin);
> + }
> +
> + /* If multiple writers including non-cygwin app exist, the non-cygwin
> + app cannot detect pipe closure on the read side when the pipe is
> + created by system account or the the pipe creator is running as
^^^
Thanks,
Corinna
Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.
Hi Takashi,
On Mar 10 19:31, Takashi Yano wrote:
> @@ -590,6 +591,10 @@ child_info_spawn::worker (const char *prog_arg, const
> char *const *argv,
> {
> fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> pipe->set_pipe_non_blocking (false);
> + pipew_duped = (fhandler_pipe *)
> + ccalloc (HEAP_FHANDLER, 1, sizeof (fhandler_pipe));
> + pipew_duped = new (pipew_duped) fhandler_pipe;
> + pipe->dup (pipew_duped, 0);
> if (pipe->request_close_query_hdl ())
> need_send_sig = true;
> }
The code setting up pipes and the dummy_tty is sufficiently complex,
so that I wonder if it shouldn't have
- its own methods and
- comments to describe why this stuff is necessary.
What about adding two methods, kind of like (the names are only
suggestion, albeit bad ones):
child_info_spawn::noncygwin_child_pre_fork()
to keep the above stuff together (plus comments) and
child_info_spawn::noncygwin_child_post_fork()
for the below code?
> @@ -597,6 +602,10 @@ child_info_spawn::worker (const char *prog_arg, const
> char *const *argv,
> {
> fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> pipe->set_pipe_non_blocking (false);
> + piper_duped = (fhandler_pipe *)
> + ccalloc (HEAP_FHANDLER, 1, sizeof (fhandler_pipe));
> + piper_duped = new (piper_duped) fhandler_pipe;
> + pipe->dup (piper_duped, 0);
> }
>
> if (need_send_sig)
> @@ -905,6 +914,19 @@ child_info_spawn::worker (const char *prog_arg, const
> char *const *argv,
> term_spawn_worker.cleanup ();
> term_spawn_worker.close_handle_set ();
> }
> + if (pipew_duped)
> + {
> + bool is_nonblocking = pipew_duped->is_nonblocking ();
> + pipew_duped->set_pipe_non_blocking (is_nonblocking);
Is that really right? You're asking pipew_duped for its
nonblocking flag and then set pipew_duped to the same value...?
> + pipew_duped->close ();
> + cfree (pipew_duped);
> + }
Thanks,
Corinna
Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.
On Mar 6 03:42, Takashi Yano wrote: > On Tue, 5 Mar 2024 17:54:19 +0100 > Corinna Vinschen wrote: > > On Mar 5 23:47, Takashi Yano wrote: > > > On Tue, 5 Mar 2024 11:14:46 +0100 > > > Corinna Vinschen wrote: > > > > This doesn't affect your patch, but while looking into this, what > > > > strikes me as weird is that fhandler_pipe::temporary_query_hdl() calls > > > > NtQueryObject() and assembles the pipe name via swscanf() every time it > > > > is called. > > > > > > > > Wouldn't it make sense to store the name in the fhandler's > > > > path_conv::wide_path/uni_path at creation time instead? > > > > The wide_path member is not used at all in pipes, ostensibly. > > > > > > Is the patch attached as you intended? > > > > Yes, but it looks like it misses a few potential simplifications: > > [...] > Thanks for advice. I have revised the patch. Looks good, thanks! Corinna
Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.
On Mar 5 23:47, Takashi Yano wrote:
> On Tue, 5 Mar 2024 11:14:46 +0100
> Corinna Vinschen wrote:
> > This doesn't affect your patch, but while looking into this, what
> > strikes me as weird is that fhandler_pipe::temporary_query_hdl() calls
> > NtQueryObject() and assembles the pipe name via swscanf() every time it
> > is called.
> >
> > Wouldn't it make sense to store the name in the fhandler's
> > path_conv::wide_path/uni_path at creation time instead?
> > The wide_path member is not used at all in pipes, ostensibly.
>
> Is the patch attached as you intended?
Yes, but it looks like it misses a few potential simplifications:
> diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> index c877d89d7..0611dd1c3 100644
> --- a/winsup/cygwin/fhandler/pipe.cc
> +++ b/winsup/cygwin/fhandler/pipe.cc
> @@ -93,6 +93,19 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode,
> int64_t uniq_id)
> even with FILE_SYNCHRONOUS_IO_NONALERT. */
> set_pipe_non_blocking (get_device () == FH_PIPER ?
> true : is_nonblocking ());
> +
> + /* Store pipe name to path_conv pc for query_hdl check */
> + if (get_dev () == FH_PIPEW)
> +{
> + ULONG len;
> + tmp_pathbuf tp;
> + OBJECT_NAME_INFORMATION *ntfn = (OBJECT_NAME_INFORMATION *) tp.w_get
> ();
> + NTSTATUS status = NtQueryObject (f, ObjectNameInformation, ntfn,
> +65536, );
> + if (NT_SUCCESS (status) && ntfn->Name.Buffer)
> + pc.set_nt_native_path (>Name);
We don't have to call NtQueryObject. The name is created in nt_create()
and we know the unique id, so the name is
"%S%S-%u-pipe-nt-%p", _u_ntfs, >installation_key,
GetCurrentProcessId (), unique_id);
Do you think it's cheaper to call NtQueryObject()? If so, no worries,
but NtQueryObject() has to call into the kernel, while just creating
the name by ourselves doesn't.
> @@ -1149,6 +1162,9 @@ fhandler_pipe::temporary_query_hdl ()
>tmp_pathbuf tp;
>OBJECT_NAME_INFORMATION *ntfn = (OBJECT_NAME_INFORMATION *) tp.w_get ();
>
> + UNICODE_STRING *name = pc.get_nt_native_path (NULL);
> + name->Buffer[name->Length / sizeof (WCHAR)] = L'\0';
The string returned by get_nt_native_path() is always NUL-terminated.
>/* Try process handle opened and pipe handle value cached first
> in order to reduce overhead. */
>if (query_hdl_proc && query_hdl_value)
> @@ -1161,14 +1177,7 @@ fhandler_pipe::temporary_query_hdl ()
>status = NtQueryObject (h, ObjectNameInformation, ntfn, 65536, );
>if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
> goto hdl_err;
> - ntfn->Name.Buffer[ntfn->Name.Length / sizeof (WCHAR)] = L'\0';
> - uint64_t key;
> - DWORD pid;
> - LONG id;
> - if (swscanf (ntfn->Name.Buffer,
> -L"\\Device\\NamedPipe\\%llx-%u-pipe-nt-0x%x",
> -, , ) == 3 &&
> - key == pipename_key && pid == pipename_pid && id == pipename_id)
> + if (RtlEqualUnicodeString (name, >Name, FALSE))
> return h;
> hdl_err:
>CloseHandle (h);
> @@ -1178,19 +1187,9 @@ cache_err:
>query_hdl_value = NULL;
> }
>
> - status = NtQueryObject (get_handle (), ObjectNameInformation, ntfn,
> - 65536, );
> - if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
> + if (name->Length == 0 || name->Buffer == NULL)
> return NULL; /* Non cygwin pipe? */
> - WCHAR name[MAX_PATH];
> - int namelen = min (ntfn->Name.Length / sizeof (WCHAR), MAX_PATH-1);
> - memcpy (name, ntfn->Name.Buffer, namelen * sizeof (WCHAR));
> - name[namelen] = L'\0';
> - if (swscanf (name, L"\\Device\\NamedPipe\\%llx-%u-pipe-nt-0x%x",
> -_key, _pid, _id) != 3)
> -return NULL; /* Non cygwin pipe? */
> -
> - return get_query_hdl_per_process (name, ntfn); /* Since Win8 */
> + return get_query_hdl_per_process (name->Buffer, ntfn); /* Since Win8 */
Given the name is stored in the fhandler, get_query_hdl_per_process()
doesn't need it as argument, and get_query_hdl_per_process() can just
call RtlCompareUnicodeString() instead of adding a \0 and calling
wcscmp().
Thanks,
Corinna
Re: [PATCH v3] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.
On Mar 5 23:48, Takashi Yano wrote:
> Non-cygwin app may call ReadFile() for empty pipe, which makes
> NtQueryObject() for ObjectNameInformation block in fhandler_pipe::
> get_query_hdl_per_process. Therefore, do not to try to get query_hdl
> for non-cygwin apps.
>
> Addresses: https://github.com/msys2/msys2-runtime/issues/202
>
> Fixes: b531d6b06eeb ("Cygwin: pipe: Introduce temporary query_hdl.")
> Reported-by: Alisa Sireneva, Johannes Schindelin
> Reviewed-by: Corinna Vinschen
> Signed-off-by: Takashi Yano
> ---
> winsup/cygwin/fhandler/pipe.cc | 57 --
> winsup/cygwin/release/3.5.2| 4 +++
> 2 files changed, 17 insertions(+), 44 deletions(-)
Looks good, thanks!
Corinna
Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.
On Mar 5 09:06, Takashi Yano wrote: > On Mon, 4 Mar 2024 18:38:07 +0100 > Corinna Vinschen wrote: > > On Mar 4 16:45, ASSI wrote: > > > Corinna Vinschen writes: > > > > Right you are. We always said that independent Cygwin installations > > > > are supposed to *stay* independent. > > > > > > > > Keep in mind that they don't share the same shared objects in the native > > > > NT namespace and they don't know of each other. It's not only the > > > > process table but also in-use FIFO stuff, pty info, etc. > > > > > > What I was getting at is that a process not showing up in the process > > > list in one Cygwin installation doesn't automatically mean it's a native > > > Windows process, it could be a process started by an independent Cygwin > > > installation. So this way of checking for "native" Windows processes > > > may or may not do what was originally intended. > > > > But that was my point. A "foreign" Cygwin process from another > > installation is not a Cygwin process. Lots of interoperability > > just won't work, so it's basically a native process. > > Actually, I think query_hdl can be retrieved from the process > from another installation of cygwin using NtQueryInformationProcess() > with ProcessHandleInformation. However, I cannot imagne the case > that the pipe is made by one cygwin installation but the reader > process is from another installation of cygwin. > > BTW, what about v2 patch itself? It does the job with less code and less memory, which is good. I would change the comment stop to try to get query_hdl for non-cygwin apps to something like don't try to fetch query_hdl from non-cygwin apps "stop trying" is a bit of a back-reference to the old code, which is not necessary, I think. This doesn't affect your patch, but while looking into this, what strikes me as weird is that fhandler_pipe::temporary_query_hdl() calls NtQueryObject() and assembles the pipe name via swscanf() every time it is called. Wouldn't it make sense to store the name in the fhandler's path_conv::wide_path/uni_path at creation time instead? The wide_path member is not used at all in pipes, ostensibly. Thanks, Corinna
Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.
On Mar 4 16:45, ASSI wrote: > Corinna Vinschen writes: > > Right you are. We always said that independent Cygwin installations > > are supposed to *stay* independent. > > > > Keep in mind that they don't share the same shared objects in the native > > NT namespace and they don't know of each other. It's not only the > > process table but also in-use FIFO stuff, pty info, etc. > > What I was getting at is that a process not showing up in the process > list in one Cygwin installation doesn't automatically mean it's a native > Windows process, it could be a process started by an independent Cygwin > installation. So this way of checking for "native" Windows processes > may or may not do what was originally intended. But that was my point. A "foreign" Cygwin process from another installation is not a Cygwin process. Lots of interoperability just won't work, so it's basically a native process. Corinna
Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.
On Mar 3 20:36, Takashi Yano wrote: > On Sun, 03 Mar 2024 11:39:40 +0100 > ASSI wrote: > > Takashi Yano writes: > > >> After noticing that we enumerate all the processes (which is an expensive > > >> operation) just to skip all of the non-Cygwin ones anyway, I wonder if it > > >> wouldn't be smarter to go through the internal list of cygpids and take > > >> it > > >> from there, skipping the `SystemProcessInformation` calls altogether. > > > > > > Yeah, that makes sens. I'll submit v2 patch. > > > > Keep in mind that there may be different independent Cygwin > > installations running on the same nachine. > > That's possible. But how can we know that is a process in another > installaion of cygwin? > > If it is difficult, I think it is not so nonsense to treat it as > same as non-cygwin process. Right you are. We always said that independent Cygwin installations are supposed to *stay* independent. Keep in mind that they don't share the same shared objects in the native NT namespace and they don't know of each other. It's not only the process table but also in-use FIFO stuff, pty info, etc. Corinna
Re: [PATCH 2/2] Cygwin: remove ENOSHARE and ECASECLASH from _sys_errlist[]
On Feb 27 17:26, Christian Franke wrote: > Hi Corinna, > > Corinna Vinschen wrote: > > On Feb 27 13:18, Christian Franke wrote: > > > ... > > > > > > diff --git a/winsup/cygwin/errno.cc b/winsup/cygwin/errno.cc > > > index 7d58e62ec..d8c057e51 100644 > > > --- a/winsup/cygwin/errno.cc > > > +++ b/winsup/cygwin/errno.cc > > > @@ -167,8 +167,8 @@ const char *_sys_errlist[] = > > > /* ESTALE 133 */ "Stale NFS file handle", > > > /* ENOTSUP 134 */ "Not supported", > > > /* ENOMEDIUM 135 */ "No medium found", > > > -/* ENOSHARE 136 */ "No such host or network path", > > > -/* ECASECLASH 137 */ "Filename exists with different case", > > > + NULL, /* Was ENOSHARE 136, no longer used. */ > > > + NULL, /* Was ECASECLASH 137, no longer used. */ > > In terms of politenness, wouldn't it be better to define them as > > empty strings? This may be one crash less in already existing > > binaries... > > Indeed, I missed that case. Patch attached. > > Christian > > From 151da4ef76f84cd0343e6f49aa23de398ca73d1c Mon Sep 17 00:00:00 2001 > From: Christian Franke > Date: Tue, 27 Feb 2024 17:21:45 +0100 > Subject: [PATCH 2/2] Cygwin: set ENOSHARE and ECASECLASH _sys_errlist[] > entries to empty > > These errno values are no longer used by Cygwin. Change the entries > to empty strings instead of NULL to avoid crashes in existing > binaries directly accessing the table. Enhance strerror_worker() > such that empty strings also result in "Unknown error ..." messages. > Also add a static_assert check for the _sys_errlist[] size. > > Signed-off-by: Christian Franke > --- > winsup/cygwin/errno.cc | 12 +--- > 1 file changed, 9 insertions(+), 3 deletions(-) Pushed. Thanks, Corinna
Re: [PATCH 2/2] Cygwin: remove ENOSHARE and ECASECLASH from _sys_errlist[]
Hi Christian, On Feb 27 13:18, Christian Franke wrote: > From f495fb0e7c2bd3a42f16f81af18c64ffaba9a860 Mon Sep 17 00:00:00 2001 > From: Christian Franke > Date: Tue, 27 Feb 2024 13:05:36 +0100 > Subject: [PATCH 2/2] Cygwin: remove ENOSHARE and ECASECLASH from > _sys_errlist[] > > These errno values are no longer used by Cygwin. Also add a > static_assert check for _sys_errlist[] size. > > Signed-off-by: Christian Franke > --- > winsup/cygwin/errno.cc | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/winsup/cygwin/errno.cc b/winsup/cygwin/errno.cc > index 7d58e62ec..d8c057e51 100644 > --- a/winsup/cygwin/errno.cc > +++ b/winsup/cygwin/errno.cc > @@ -167,8 +167,8 @@ const char *_sys_errlist[] = > /* ESTALE 133 */ "Stale NFS file handle", > /* ENOTSUP 134 */ "Not supported", > /* ENOMEDIUM 135 */"No medium found", > -/* ENOSHARE 136 */ "No such host or network path", > -/* ECASECLASH 137 */ "Filename exists with different case", > + NULL, /* Was ENOSHARE 136, no longer used. */ > + NULL, /* Was ECASECLASH 137, no longer used. */ In terms of politenness, wouldn't it be better to define them as empty strings? This may be one crash less in already existing binaries... Corinna
Re: [PATCH 1/4] Cygwin: introduce constexpr errmap_size and errmap[] consistency checks
On Feb 26 15:21, Christian Franke wrote: > From 947daa02b0b64131626c2ecedb74ca6893aab6c6 Mon Sep 17 00:00:00 2001 > From: Christian Franke > Date: Mon, 26 Feb 2024 13:37:33 +0100 > Subject: [PATCH 1/4] Cygwin: introduce constexpr errmap_size and errmap[] > consistency checks > > Use constexpr instead of const for errmap[] to allow static_assert > checks on its values. > > Signed-off-by: Christian Franke > --- > winsup/cygwin/errno.cc| 2 +- > winsup/cygwin/local_includes/errmap.h | 11 ++- > 2 files changed, 11 insertions(+), 2 deletions(-) :+1: Patchset pushed. Thanks, Corinna
Re: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to ENODEV
On Feb 26 12:14, Christian Franke wrote: > Corinna Vinschen wrote: > > Why so many? I used winerror.h to populate the list not too long ago, > > so I wonder why it suddenly has so many more error codes? > > "Required for mozilla-central." - 850 insertions: > https://sourceforge.net/p/mingw-w64/mingw-w64/ci/ddeb05a > > Most or all would possible never occur with the NTDLL/Win32 API subset used > by Cygwin. > > Includes interesting codes like "ERROR_NO_WORK_DONE" :-) LOL. The only hint I found on that one is in Wine ChangeLog: "FormatMessage() now reports ERROR_NO_WORK_DONE error for empty string." Corinna
Re: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to ENODEV
On Feb 25 10:12, Christian Franke wrote: > Corinna Vinschen wrote: > > So the default was EPERM at first and has been changed to EACCES > > because it "is better for the unknown error case". > > > > I'm open to ideas for an improved error mapping. > > I have no better suggestion for a default errno. Adding a cygwin specific > one (like ENMFILE, ENOSHARE and ECASECLASH added 2000-2001) is possibly not > desired. ENOSHARE and ECASECLASH are not used anymore, fortunately, and ENMFILE is hopefully never returned to userspace. It might be a good idea to remove it from Cygwin's code as well. > Some thoughts about minor improvements of the errmap.h file: > - Add error number to each /* ERROR_... */ comment, e.g. /* 2: > ERROR_FILE_NOT_FOUND */. Ok. > - Update /* NUMBER */ comments using current MinGW-w64's winerror.h (~850 > changes). Why so many? I used winerror.h to populate the list not too long ago, so I wonder why it suddenly has so many more error codes? > - Max errno is 143, so data type size could be reduced from int to uint8_t > aka unsigned char. Could even add a compile time check by using C++11's > braced initializers which do not allow narrowing conversions. Yeah, we could do that. > - Remove trailing entries which only map to 0. > - Append a static_assert which checks whether array size matches the last > mapped error number. Yeah, not so sure about that. I'm aware that we only map errors below 3000 somewhere, but it's no safe bet that it stays that way. For instance, we handle NT status codes STATUS_TRANSACTIONAL_CONFLICT and STATUS_TRANSACTION_NOT_ENLISTED and those translate into the TxF error range between 6800 and 6899. We don't convert those to userspace errno yet, but consider having to add them at one point and thus having to add the 3000 entries from the last used one up the newly used one. The reason to keep them is to allow us to be lazy about it. The list also just takes ~36K, and with the change to uint8_t it only takes 9K, so what? > I could provide separate patches if desired. Always welcome! Thanks, Corinna
Re: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to ENODEV
On Feb 23 19:14, Christian Franke wrote: > Experiments with damaged USB flash drives and ddrescue revealed that the > current mapping of these Win32 errors to the fallback EACCES could be > improved. > > BTW: I wonder why EACCES was selected as the fallback. Source code control > forensics suggest that this was decided in the last millennium. A related > comment from CGF added August 2000 persists until today :-) > /* FIXME: what's so special about EACCESS? */ This goes back until 1997 in pre-CVS times. There's a ChangeLog entry Wed Oct 29 22:43:57 1997 Geoffrey Noer [...] * syscalls.cc (seterrno): on failure, set EACCES instead of EPERM which is better for the unknown error case So the default was EPERM at first and has been changed to EACCES because it "is better for the unknown error case". I'm open to ideas for an improved error mapping. > From 8aa19c7fd13dc3790dc271dede8954539bffcd4d Mon Sep 17 00:00:00 2001 > From: Christian Franke > Date: Fri, 23 Feb 2024 19:01:09 +0100 > Subject: [PATCH] Cygwin: Map ERROR_NO_SUCH_DEVICE and ERROR_MEDIA_CHANGED to > ENODEV > > If a removable (USB) device is disconnected after opening its raw > device, R/W attempts fail with ERROR_NO_SUCH_DEVICE(433). If the > raw device of a partition is used, ERROR_MEDIA_CHANGED(1110) is > returned instead. Both are mapped to ENODEV(19) because > does not offer a value which better matches ERROR_MEDIA_CHANGED. > > Signed-off-by: Christian Franke > --- > winsup/cygwin/local_includes/errmap.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Pushed. Thanks, Corinna
Re: [PATCH] Cygwin: console: Fix exit code for non-cygwin process.
On Feb 4 00:04, Takashi Yano wrote: > On Sat, 3 Feb 2024 15:27:06 +0100 (CET) > Johannes Schindelin wrote: > > On IRC, you reported that the thread would crash if `cons` was not fixed > > up. The symptom was that that crash would apparently prevent the exit code > > from being read, and it would be left at 0, indicating potentially > > incorrectly that the non-Cygwin process succeeded. > > > > I wonder: What would it take to change this logic so that the crash would > > be detected (and not be misinterpreted as exit code 0)? > > I am not sure, but I think it is necessary to modify: > pinfo::exit() > pinfo::meybe_set_exit_code_from_windows() > pinfo::set_exit_code() > > I guess detecting crash of sbub process needs modification of > spawn.cc. Dumb question: If, as Johannes said, the error code cannot be fetched, can't we set the error code to a POSIX return code indicating a signal? I.e., checking for WIFSIGNALED() returns 1 and WTERMSIG() returns, say, SIGKILL or something? Corinna
Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 27 15:12, Jon Turney wrote: > On 26/01/2024 11:52, Corinna Vinschen wrote: > > > - Create a named mutex with a reproducible name (no need to use > > >the name as parameter) and immediately grab it. > > > - Call CreateProcess to start the debugger with CREATE_SUSPENDED > > >flag. > > > - Create a HANDLE array with the mutex and the process HANDLE. > > > > On second thought, it might be a good idea to make this > > interruptible as well, but given this is called from the > > exception handler this may have weird results... > > > - Call ResumeThread on the primary debugger thread. > > > - Call WFMO with timeout. > > > > > > Later on, the debugger either fails and exits or it calls > > > ReleaseMutex after having attached to the process. > > > > > > - WFMO returns > > > - If the mutex has triggered, we're being debugged (but check > > >IsDebuggerPresent() just to be sure) > > > - If the process has triggered, the debugger exited > > > - If the timeout triggers... oh well. > > This seems like quite a lot of work, for very marginal benefit. > > And doing lots of complex work inside the process when we're in the middle > of handling a SEGV seems like asking for trouble. > > I think I'll leave this alone for the moment, and we can see what (if any) > problems surface. Ok, no worries. Corinna
Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 26 12:12, Corinna Vinschen wrote: > On Jan 25 20:03, Jon Turney wrote: > > On 25/01/2024 18:21, Corinna Vinschen wrote: > > > On Jan 25 14:50, Jon Turney wrote: > > > > On 24/01/2024 14:39, Corinna Vinschen wrote: > > > > > On Jan 24 13:28, Jon Turney wrote: > > > > > > On 23/01/2024 14:29, Corinna Vinschen wrote: > > > > > > > On Jan 23 14:20, Jon Turney wrote: > > [...] > > > > So this situation with a JIT debugger is even stranger than my > > > > emendations > > > > to the documentation say. > > > > > > > > Because we're hitting try_to_debug() in exception::handle(), which has > > > > some > > > > code to replay the exception via ExceptionContinueExecution (which I > > > > guess > > > > the debugger will catch as a first-chance) (and goes into a mode where > > > > it > > > > ignores the next half-million exceptions... no idea what that's about!) > > > > > > > > That's not the same as signal_exit() with a coredumping signal (haven't > > > > checked if those are all generated from exceptions, but seems probable, > > > > so > > > > the try_to_debug() there maybe isn't doing anything?), where we're > > > > going to > > > > exit thereafter. > > > > > > try_to_debug() is only calling IsDebuggerPresent() as test, and that's > > > nothing but a flag in the PEB which is set by the OS after a debugger > > > attached to the process. So the test is by definition extremely > > > flaky, if the debugger is connecting and disconnecting, as you > > > already pointed out. > > > > > > I'm wondering if we can't define our own way to attach to a process, > > > allowing to "WaitForDebugger" as long as the debugger is a Cygwin > > > debugger. If we define a matching function (along the lines of > > > prctl(2) on Linux), we could change our debuggers, core dumpers > > > and stracers to call this attach function. > > > > > > The idea would be to define some shared mutex object, the inferior > > > waits for and the debugger releases after having attached. > > > > > > Is there really any need to support non-Cygwin debuggers? > > > > idk > > > > I think something like that used to exist a long time ago, see commit > > 8abeff1ead5f3824c70111bc0ff74ff835dafa55 > > Yeah, just, as was the default at the time, without any trace of a > *rational* why it has been removed. Also, it was too simple anyway. > > First, if we want to support WIndows debugger, the inferior has to check > if the debugger is a Cygwin or native debugger. If a native debugger, > just stick to what we have today. Otherwise: > > - Create a named mutex with a reproducible name (no need to use > the name as parameter) and immediately grab it. > - Call CreateProcess to start the debugger with CREATE_SUSPENDED > flag. > - Create a HANDLE array with the mutex and the process HANDLE. On second thought, it might be a good idea to make this interruptible as well, but given this is called from the exception handler this may have weird results... > - Call ResumeThread on the primary debugger thread. > - Call WFMO with timeout. > > Later on, the debugger either fails and exits or it calls > ReleaseMutex after having attached to the process. > > - WFMO returns > - If the mutex has triggered, we're being debugged (but check > IsDebuggerPresent() just to be sure) > - If the process has triggered, the debugger exited > - If the timeout triggers... oh well. Corinna
Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 25 20:03, Jon Turney wrote: > On 25/01/2024 18:21, Corinna Vinschen wrote: > > On Jan 25 14:50, Jon Turney wrote: > > > On 24/01/2024 14:39, Corinna Vinschen wrote: > > > > On Jan 24 13:28, Jon Turney wrote: > > > > > On 23/01/2024 14:29, Corinna Vinschen wrote: > > > > > > On Jan 23 14:20, Jon Turney wrote: > [...] > > > So this situation with a JIT debugger is even stranger than my emendations > > > to the documentation say. > > > > > > Because we're hitting try_to_debug() in exception::handle(), which has > > > some > > > code to replay the exception via ExceptionContinueExecution (which I guess > > > the debugger will catch as a first-chance) (and goes into a mode where it > > > ignores the next half-million exceptions... no idea what that's about!) > > > > > > That's not the same as signal_exit() with a coredumping signal (haven't > > > checked if those are all generated from exceptions, but seems probable, so > > > the try_to_debug() there maybe isn't doing anything?), where we're going > > > to > > > exit thereafter. > > > > try_to_debug() is only calling IsDebuggerPresent() as test, and that's > > nothing but a flag in the PEB which is set by the OS after a debugger > > attached to the process. So the test is by definition extremely > > flaky, if the debugger is connecting and disconnecting, as you > > already pointed out. > > > > I'm wondering if we can't define our own way to attach to a process, > > allowing to "WaitForDebugger" as long as the debugger is a Cygwin > > debugger. If we define a matching function (along the lines of > > prctl(2) on Linux), we could change our debuggers, core dumpers > > and stracers to call this attach function. > > > > The idea would be to define some shared mutex object, the inferior > > waits for and the debugger releases after having attached. > > > > Is there really any need to support non-Cygwin debuggers? > > idk > > I think something like that used to exist a long time ago, see commit > 8abeff1ead5f3824c70111bc0ff74ff835dafa55 Yeah, just, as was the default at the time, without any trace of a *rational* why it has been removed. Also, it was too simple anyway. First, if we want to support WIndows debugger, the inferior has to check if the debugger is a Cygwin or native debugger. If a native debugger, just stick to what we have today. Otherwise: - Create a named mutex with a reproducible name (no need to use the name as parameter) and immediately grab it. - Call CreateProcess to start the debugger with CREATE_SUSPENDED flag. - Create a HANDLE array with the mutex and the process HANDLE. - Call ResumeThread on the primary debugger thread. - Call WFMO with timeout. Later on, the debugger either fails and exits or it calls ReleaseMutex after having attached to the process. - WFMO returns - If the mutex has triggered, we're being debugged (but check IsDebuggerPresent() just to be sure) - If the process has triggered, the debugger exited - If the timeout triggers... oh well. > That long predates my involvement with cygwin so I've no idea why that was > removed. It doesn't predate mine, but I know just as much as you do. Maybe the mailing list archives help, but tht's no safe bet. > > > The practical upshot of this is if the JIT debugger doesn't terminate or > > > fix > > > the erroring process, we'll just replay the faulting instruction and > > > invoke > > > the JIT debugger again. > > > > Hmm, ok. This signal stuff *is* complicated and I'd be happy > > if anybody finds out how to fix that... > > To be clear, not a problem with "core dumping signals", as the process now > always end up exiting, one way or another. > > It's only a problem when someone has set "CYGWIN=error_start:true" or > something equally dumb. Ok. Corinna
Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 25 14:50, Jon Turney wrote: > On 24/01/2024 14:39, Corinna Vinschen wrote: > > On Jan 24 13:28, Jon Turney wrote: > > > On 23/01/2024 14:29, Corinna Vinschen wrote: > > > > On Jan 23 14:20, Jon Turney wrote: > > > > > > > > > Even then this is clearly not totally bullet-proof. Maybe the right > > > > > thing to > > > > > do is add a suitable timeout here, so even if we fail to notice the > > > > > DebugActiveProcess() (or there's a custom JIT debugger which just > > > > > writes the > > > > > fact a process crashed to a logfile or something), we'll exit > > > > > eventually? > > > > > > > > Timeouts are just that tiny little bit more bullet-proof, they still > > > > aren't totally bullet-proof. > > > > > > > > What timeout were you thinking of? milliseconds? > > > > > > Oh no, tens of seconds or something, just as a fail-safe. > > > > Uh, sounds a lot. 10 secs? Not longer, I think. > > > > If you want to do that for 3.5, please do it this week. You can > > push the change without waiting for approval. > > Thanks. > > I pushed a small change adding this timeout. > > > > (Ofc, all this is working around the fact that Win32 API doesn't have a > > > WaitForDebuggerPresent(timeout) function) > > > > Yeah, and there's no alternative way using the native API afaics :( > > So this situation with a JIT debugger is even stranger than my emendations > to the documentation say. > > Because we're hitting try_to_debug() in exception::handle(), which has some > code to replay the exception via ExceptionContinueExecution (which I guess > the debugger will catch as a first-chance) (and goes into a mode where it > ignores the next half-million exceptions... no idea what that's about!) > > That's not the same as signal_exit() with a coredumping signal (haven't > checked if those are all generated from exceptions, but seemly probable, so > the try_to_debug() there maybe isn't doing anything?), where we're going to > exit thereafter. try_to_debug() is only calling IsDebuggerPresent() as test, and that's nothing but a flag in the PEB which is set by the OS after a debugger attached to the process. So the test is by definition extremely flaky, if the debugger is connecting and disconnecting, as you already pointed out. I'm wondering if we can't define our own way to attach to a process, allowing to "WaitForDebugger" as long as the debugger is a Cygwin debugger. If we define a matching function (along the lines of prctl(2) on Linux), we could change our debuggers, core dumpers and stracers to call this attach function. The idea would be to define some shared mutex object, the inferior waits for and the debugger releases after having attached. Is there really any need to support non-Cygwin debuggers? > The practical upshot of this is if the JIT debugger doesn't terminate or fix > the erroring process, we'll just replay the faulting instruction and invoke > the JIT debugger again. Hmm, ok. This signal stuff *is* complicated and I'd be happy if anybody finds out how to fix that... Corinna
Re: [PATCH] Cygwin: pthread: Fix handle leak in pthread_once.
On Jan 24 23:48, Takashi Yano wrote: > On Wed, 24 Jan 2024 15:40:22 +0100 > Corinna Vinschen wrote: > > (You don't have to CC me, btw., I only get the same mail twice then > > and I look into this mailing list constantly anyway) > > Perhaps, CC: is added automatically by git send-email if > Reviewed-by: exists. I'll try --no-cc option next time. No worries. No reason for jumping through hoops, just for an extra mail. Thanks, Corinna
Re: [PATCH] Cygwin: pthread: Fix handle leak in pthread_once.
On Jan 24 22:44, Takashi Yano wrote:
> If pthread_once() is called with pthread_once_t initialized using
> PTREAD_ONCE_INIT, pthread_once does not release pthread_mutex used
> internally. This patch fixes that by calling pthread_mutex_destroy()
> in the thread which has called init_routine.
>
> Reviewed-by: Corinna Vinschen
> Signed-off-by: Takashi Yano
> ---
> winsup/cygwin/thread.cc | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> index 7bb4f9fc8..0f8327831 100644
> --- a/winsup/cygwin/thread.cc
> +++ b/winsup/cygwin/thread.cc
> @@ -2060,6 +2060,9 @@ pthread::once (pthread_once_t *once_control, void
> (*init_routine) (void))
> {
>init_routine ();
>once_control->state = 1;
> + pthread_mutex_unlock (_control->mutex);
> + while (pthread_mutex_destroy (_control->mutex) == EBUSY);
> + return 0;
> }
>/* Here we must remove our cancellation handler */
>pthread_mutex_unlock (_control->mutex);
> --
> 2.43.0
Sure, please push.
(You don't have to CC me, btw., I only get the same mail twice then
and I look into this mailing list constantly anyway)
Thanks,
Corinna
Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 24 13:28, Jon Turney wrote: > On 23/01/2024 14:29, Corinna Vinschen wrote: > > On Jan 23 14:20, Jon Turney wrote: > > > > > Even then this is clearly not totally bullet-proof. Maybe the right thing > > > to > > > do is add a suitable timeout here, so even if we fail to notice the > > > DebugActiveProcess() (or there's a custom JIT debugger which just writes > > > the > > > fact a process crashed to a logfile or something), we'll exit eventually? > > > > Timeouts are just that tiny little bit more bullet-proof, they still > > aren't totally bullet-proof. > > > > What timeout were you thinking of? milliseconds? > > Oh no, tens of seconds or something, just as a fail-safe. Uh, sounds a lot. 10 secs? Not longer, I think. If you want to do that for 3.5, please do it this week. You can push the change without waiting for approval. > (Ofc, all this is working around the fact that Win32 API doesn't have a > WaitForDebuggerPresent(timeout) function) Yeah, and there's no alternative way using the native API afaics :( Corinna
Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 23 14:20, Jon Turney wrote: > On 12/01/2024 14:09, Jon Turney wrote: > > Pre-format a command to be executed on a fatal error to run 'dumper' > > (using an absolute path). > > > > Factor out executing a pre-formatted command, so we can use that for > > invoking the JIT debugger in try_to_debug() (if error_start is present > > in the CYGWIN env var) and to invoke dumper when a fatal error occurs. > > > > So, there is a small problem with this change: because dumper itself > terminates the dumped process, it doesn't go on to exit with the signal+128 > exit status. > > (In fact, it seems to exit with status 0 when terminated by an attached > debugger terminating, which isn't great) > > That's relatively easy to fix: just use the '-n' option to dumper so it > detaches before exiting, to prevent that terminating the dumped process, but > then we run into the difficulties of reliably detecting that dumper has > attached and done it's work, so it's safe for us to exit. > > Attached patch does that, and documents the expectations on the error_start > command a bit more clearly. Please push. > Even then this is clearly not totally bullet-proof. Maybe the right thing to > do is add a suitable timeout here, so even if we fail to notice the > DebugActiveProcess() (or there's a custom JIT debugger which just writes the > fact a process crashed to a logfile or something), we'll exit eventually? Timeouts are just that tiny little bit more bullet-proof, they still aren't totally bullet-proof. What timeout were you thinking of? milliseconds? Corinna
Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 16 13:52, Jon Turney wrote: > On 15/01/2024 14:28, Corinna Vinschen wrote: > > On Jan 15 13:27, Jon Turney wrote: > > > On 15/01/2024 09:46, Corinna Vinschen wrote: > > > > On Jan 13 14:20, Jon Turney wrote: > > > > > On 12/01/2024 14:09, Jon Turney wrote: > > > > > > + > > > > > > + PWCHAR cp = dumper_command; > > > > > > + cp = wcpcpy (cp, L"\""); > > > > > > + cp = wcpcpy (cp, dll_dir); > > > > > > + cp = wcpcpy (cp, L"\\dumper.exe"); > > > > > > + cp = wcpcpy (cp, L"\" "); > > > > > > + cp = wcpcpy (cp, L"\""); > > > > > > + cp = wcpcpy (cp, global_progname); > > > > > > > > > > I wonder if this should be program_invocation_short_name, so that the > > > > > coredump is created in the cwd, rather than next to the executable. > > So, when I actually look further into this, what I wrote is utter nonsense. > dumper/minidumper includes the following: > > > ssize_t len = cygwin_conv_path (CCP_POSIX_TO_WIN_A | CCP_RELATIVE, > > *(argv + optind), NULL, 0); > > char *win32_name = (char *) alloca (len); > > cygwin_conv_path (CCP_POSIX_TO_WIN_A | CCP_RELATIVE, *(argv + > > optind), > > win32_name, len); > > if ((p = strrchr (win32_name, '\\'))) > > p++; > > else > > p = win32_name; > > My eyes moving over this lightly, my brain assumes it just converts from a > Win32 path to a POSIX path, but actually it does: > > 1) convert from POSIX path to Windows path (assuming it's a no-op if the > path is already in a Windows form > 2) (now having a consistent path delimiter) use the part after the last > delimiter (if any), as the basename. > > So: no problem here, after all. coredump file is already created in the cwd. :+1:
Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 15 13:27, Jon Turney wrote: > On 15/01/2024 09:46, Corinna Vinschen wrote: > > On Jan 13 14:20, Jon Turney wrote: > > > On 12/01/2024 14:09, Jon Turney wrote: > > > > + > > > > + PWCHAR cp = dumper_command; > > > > + cp = wcpcpy (cp, L"\""); > > > > + cp = wcpcpy (cp, dll_dir); > > > > + cp = wcpcpy (cp, L"\\dumper.exe"); > > > > + cp = wcpcpy (cp, L"\" "); > > > > + cp = wcpcpy (cp, L"\""); > > > > + cp = wcpcpy (cp, global_progname); > > > > > > I wonder if this should be program_invocation_short_name, so that the > > > coredump is created in the cwd, rather than next to the executable. > > > > program_invocation_short_name would be nice, but does it really matter? > > > > Because... > > > > > But then, there's then no way to get similar behaviour if you decide you > > > want to use minidumps instead (by setting > > > CYGWIN="error_start=minidumper"), > > > as the first argument to dumper/minidump is the full path to the program > > > (to > > > match the 'prog procID' style of invoking gdb), but they only use it to > > > add > > > an .core/.dmp extension to name the file to write. > > > > > > I guess that could by fixed by adding an option to the dumpers to strip > > > paths, or more control about how the JIT command is formatted. > > > > dumper/minidumper are both called with the current working directory set > > to the ... current working directory, right? With the full pathname as > > input, and the CWD already set the same as the dumped application, they > > can easily generate any target path for the corefile they like. > > > > Given the actual path of the corefile can be generated by the dumpers, > > the question is how to specify where to store the corefile. For instance > > > > - no option: CWD > > - some option -c/--coredir for anywhere else > > > > Under Linux versions using systemd, corefiles are by default not stored > > in the CWD anymore, but to /var/lib/systemd/coredump, so there is a > > use case for arbitrary corefile paths. > > Yeah, I guess an option to the dumper to control where the file is written > is probably the best way to address this, which is something which can > perhaps be added later... Yeah. In that case we should write the coredump to CWD for the time being and improve that for 3.6, ok? Corinna
Re: [PATCH] Cygwin: introduce close_range
On Jan 15 13:41, Christian Franke wrote:
> Corinna Vinschen wrote:
> > On Jan 15 13:07, Corinna Vinschen wrote:
> > > Sorry Christian, but..
> > >
> > > I was just going to push this patch when I realized that we now have
> > > two lines of debug output per affected file descriptor:
> > >
> > > On Jan 15 12:19, Christian Franke wrote:
> > > > + for (unsigned int i = firstfd; i < size; i++)
> > > > +{
> > > > + cygheap_fdget cfd ((int) i, false, false);
> > > > + if (cfd < 0)
> > > > + continue;
> > > > +
> > > > + if (flags & CLOSE_RANGE_CLOEXEC)
> > > > + {
> > > > + syscall_printf ("set FD_CLOEXEC on fd %u", i);
> > > > + cfd->fcntl (F_SETFD, FD_CLOEXEC);
> > > fhandler::set_close_on_exec() already prints this:
> > >
> > >debug_printf ("set close_on_exec for %s to %d", get_name (), val);
> > >
> > > > + }
> > > > + else
> > > > + {
> > > > + syscall_printf ("closing fd %u", i);
> > > > + cfd->close_with_arch ();
> > > fhandler::close() already prints this:
> > >
> > >syscall_printf ("closing '%s' handle %p", get_name (), get_handle ());
>
> I've also seen this duplication, but the drawback of the above messages is
> that the FD itself is not printed.
In both cases, that's right. Good point, actually.
> So I decided to keep the
> syscall_printf().
I pushed the patch now with the additional syscall_printf, FWIW.
If they really annoy people, which unlikely, we can still drop them.
Thanks,
Corinna
Re: [PATCH] Cygwin: introduce close_range
On Jan 15 13:07, Corinna Vinschen wrote:
> Sorry Christian, but..
>
> I was just going to push this patch when I realized that we now have
> two lines of debug output per affected file descriptor:
>
> On Jan 15 12:19, Christian Franke wrote:
> > + for (unsigned int i = firstfd; i < size; i++)
> > +{
> > + cygheap_fdget cfd ((int) i, false, false);
> > + if (cfd < 0)
> > + continue;
> > +
> > + if (flags & CLOSE_RANGE_CLOEXEC)
> > + {
> > + syscall_printf ("set FD_CLOEXEC on fd %u", i);
> > + cfd->fcntl (F_SETFD, FD_CLOEXEC);
>
> fhandler::set_close_on_exec() already prints this:
>
> debug_printf ("set close_on_exec for %s to %d", get_name (), val);
>
> > + }
> > + else
> > + {
> > + syscall_printf ("closing fd %u", i);
> > + cfd->close_with_arch ();
>
> fhandler::close() already prints this:
>
> syscall_printf ("closing '%s' handle %p", get_name (), get_handle ());
>
> Shan't we drop the syscall calls from close_range()?
^^^
syscall_printf
Re: [PATCH] Cygwin: introduce close_range
Sorry Christian, but..
I was just going to push this patch when I realized that we now have
two lines of debug output per affected file descriptor:
On Jan 15 12:19, Christian Franke wrote:
> + for (unsigned int i = firstfd; i < size; i++)
> +{
> + cygheap_fdget cfd ((int) i, false, false);
> + if (cfd < 0)
> + continue;
> +
> + if (flags & CLOSE_RANGE_CLOEXEC)
> + {
> + syscall_printf ("set FD_CLOEXEC on fd %u", i);
> + cfd->fcntl (F_SETFD, FD_CLOEXEC);
fhandler::set_close_on_exec() already prints this:
debug_printf ("set close_on_exec for %s to %d", get_name (), val);
> + }
> + else
> + {
> + syscall_printf ("closing fd %u", i);
> + cfd->close_with_arch ();
fhandler::close() already prints this:
syscall_printf ("closing '%s' handle %p", get_name (), get_handle ());
Shan't we drop the syscall calls from close_range()?
Thanks,
Corinna
Re: [PATCH] Cygwin: introduce close_range
Hi Christian, On Jan 15 09:56, Christian Franke wrote: > Christian Franke wrote: > > Jon Turney wrote: > > > On 14/01/2024 16:07, Christian Franke wrote: > > > > Recently I learned about the existence and usefulness of close_range(): > > > > https://github.com/smartmontools/smartmontools/issues/235 > > > > > > > > https://man.freebsd.org/cgi/man.cgi?query=close_range=2 > > > > https://man7.org/linux/man-pages/man2/close_range.2.html > > > > > > > > Note that the above Linux man page is not fully correct. The > > > > include file "linux/close_range.h" exists, but provides only the > > > > defines. It is sufficient to include "unistd.h" as on FreeBSD. > > > > > > > > The attached patch adds this to Cygwin. It does not implement > > > > the Linux-specific CLOSE_RANGE_UNSHARE as I have no idea how to > > > > do this :-) > > > > > > This API should also be mentioned in the > > > "System interfaces compatible with GNU or Linux extensions" section > > > of doc/posix.xml > > > > > > > > > > Thanks for the info. I used the recent "Cygwin: introduce fallocate(2)" > > patch as a blueprint for which other files should be changed (fallocate > > is also missing in the posix.xml file). > > > > I will provide a new patch soon which also fixes an unlikely but > > possible corner case: Pass a value larger than MAX_INT as lower limit. > > > > Attached. I also decided to simply ignore CLOSE_RANGE_UNSHARE for now. After reading up on this issue, I think we should not ignore CLOSE_RANGE_UNSHARE, but quite explicitely not implement it as a valid flag. The whole idea behind CLOSE_RANGE_UNSHARE depends on the way the Linux kernel creates threads and (forked) processes and the fact that it has a wide range of ways to share parts of the execution context between parent and child process/thread. So on Linux, a process/thread can actually decide if they share or not share the descriptor table with the created process/thread. That's what the CLONE_FILES flag to clone(2) and unshare(2) manage. However, just as in FreeBSD, there's no such thing in Cygwin. Threads always share descriptors, processes never share file desriptors. The bottom line is, I think the decision of the FreeBSD developer not to expose the CLOSE_RANGE_UNSHARE flag at all, was the right decision. We should not claim that we even remotely have a way of doing this the Linux way. Does that make sense? Apart from this little thing I think the patch is nice. Thanks, Corinna > From f7704bf77a926e53e8200528ab6abc1c9fdca511 Mon Sep 17 00:00:00 2001 > From: Christian Franke > Date: Mon, 15 Jan 2024 09:52:50 +0100 > Subject: [PATCH] Cygwin: introduce close_range(2) > > This function closes or sets the close-on-exec flag for a specified > range of file descriptors. It is available on FreeBSD and Linux. > > Signed-off-by: Christian Franke > --- > newlib/libc/include/sys/unistd.h | 9 ++ > winsup/cygwin/cygwin.din | 1 + > winsup/cygwin/include/cygwin/version.h | 3 +- > winsup/cygwin/release/3.5.0| 2 ++ > winsup/cygwin/syscalls.cc | 43 ++ > winsup/doc/new-features.xml| 4 +++ > winsup/doc/posix.xml | 5 +++ > 7 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/newlib/libc/include/sys/unistd.h > b/newlib/libc/include/sys/unistd.h > index 25532251c..0a31544ed 100644 > --- a/newlib/libc/include/sys/unistd.h > +++ b/newlib/libc/include/sys/unistd.h > @@ -26,6 +26,15 @@ int chown (const char *__path, uid_t __owner, gid_t > __group); > int chroot (const char *__path); > #endif > int close (int __fildes); > +#if defined(__CYGWIN__) && (__BSD_VISIBLE || __GNU_VISIBLE) > +/* Available on FreeBSD (__BSD_VISIBLE) and Linux (__GNU_VISIBLE). */ > +int close_range (unsigned int __firstfd, unsigned int __lastfd, int > __flags); > +#if __GNU_VISIBLE > +/* Linux-specific, ignored by Cygwin. */ > +#define CLOSE_RANGE_UNSHARE (1 << 1) > +#endif > +#define CLOSE_RANGE_CLOEXEC (1 << 2) > +#endif > #if __POSIX_VISIBLE >= 199209 > size_t confstr (int __name, char *__buf, size_t __len); > #endif > diff --git a/winsup/cygwin/cygwin.din b/winsup/cygwin/cygwin.din > index 9b76ce67a..9e354acc6 100644 > --- a/winsup/cygwin/cygwin.din > +++ b/winsup/cygwin/cygwin.din > @@ -347,6 +347,7 @@ clog10l NOSIGFE > clogf NOSIGFE > clogl NOSIGFE > close SIGFE > +close_range SIGFE > closedir SIGFE > closelog SIGFE > cnd_broadcast SIGFE > diff --git a/winsup/cygwin/include/cygwin/version.h > b/winsup/cygwin/include/cygwin/version.h > index c8177c2b1..3036878c4 100644 > --- a/winsup/cygwin/include/cygwin/version.h > +++ b/winsup/cygwin/include/cygwin/version.h > @@ -484,12 +484,13 @@ details. */ >347: Add c16rtomb, c32rtomb, mbrtoc16, mbrtoc32. >348: Add c8rtomb, mbrtoc. >349: Add fallocate. > + 350: Add close_range. > >Note that we forgot to bump the api for ualarm, strtoll, strtoull, >sigaltstack,
Re: [PATCH] Cygwin: introduce close_range
On Jan 14 19:53, Christian Franke wrote: > Jon Turney wrote: > > On 14/01/2024 16:07, Christian Franke wrote: > > > Recently I learned about the existence and usefulness of close_range(): > > > https://github.com/smartmontools/smartmontools/issues/235 > > > > > > https://man.freebsd.org/cgi/man.cgi?query=close_range=2 > > > https://man7.org/linux/man-pages/man2/close_range.2.html > > > > > > Note that the above Linux man page is not fully correct. The include > > > file "linux/close_range.h" exists, but provides only the defines. It > > > is sufficient to include "unistd.h" as on FreeBSD. > > > > > > The attached patch adds this to Cygwin. It does not implement the > > > Linux-specific CLOSE_RANGE_UNSHARE as I have no idea how to do this > > > :-) > > > > This API should also be mentioned in the > > "System interfaces compatible with GNU or Linux extensions" section of > > doc/posix.xml > > > > > > Thanks for the info. I used the recent "Cygwin: introduce fallocate(2)" > patch as a blueprint for which other files should be changed (fallocate is > also missing in the posix.xml file). Oops, thanks for notifying. I'll add it in a bit... Corinna
Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 13 14:20, Jon Turney wrote: > On 12/01/2024 14:09, Jon Turney wrote: > > + > > + PWCHAR cp = dumper_command; > > + cp = wcpcpy (cp, L"\""); > > + cp = wcpcpy (cp, dll_dir); > > + cp = wcpcpy (cp, L"\\dumper.exe"); > > + cp = wcpcpy (cp, L"\" "); > > + cp = wcpcpy (cp, L"\""); > > + cp = wcpcpy (cp, global_progname); > > I wonder if this should be program_invocation_short_name, so that the > coredump is created in the cwd, rather than next to the executable. program_invocation_short_name would be nice, but does it really matter? Because... > But then, there's then no way to get similar behaviour if you decide you > want to use minidumps instead (by setting CYGWIN="error_start=minidumper"), > as the first argument to dumper/minidump is the full path to the program (to > match the 'prog procID' style of invoking gdb), but they only use it to add > an .core/.dmp extension to name the file to write. > > I guess that could by fixed by adding an option to the dumpers to strip > paths, or more control about how the JIT command is formatted. dumper/minidumper are both called with the current working directory set to the ... current working directory, right? With the full pathname as input, and the CWD already set the same as the dumped application, they can easily generate any target path for the corefile they like. Given the actual path of the corefile can be generated by the dumpers, the question is how to specify where to store the corefile. For instance - no option: CWD - some option -c/--coredir for anywhere else Under Linux versions using systemd, corefiles are by default not stored in the CWD anymore, but to /var/lib/systemd/coredump, so there is a use case for arbitrary corefile paths. Corinna
Re: [PATCH 5/5] Cygwin: Update documentation for cygwin_stackdump
On Jan 12 14:09, Jon Turney wrote: > --- > winsup/doc/misc-funcs.xml | 4 > 1 file changed, 4 insertions(+) > > diff --git a/winsup/doc/misc-funcs.xml b/winsup/doc/misc-funcs.xml > index 7463942e6..55c5cac94 100644 > --- a/winsup/doc/misc-funcs.xml > +++ b/winsup/doc/misc-funcs.xml > @@ -106,6 +106,10 @@ enum. The second is an optional pointer. > > Description > Outputs a stackdump to stderr from the called location. > + > + Note: This function is has an effect the first time it is called by a > process. ^^ This looks like a rephrasing attempt gone slightly wrong. I'm also not quite sure what you're trying to say here. Maybe a bit more detailed would help me and other readers? Thanks, Corinna
Re: [PATCH 0/5] Coredump under 'ulimit -c' control (v2)
On Jan 12 14:09, Jon Turney wrote: > Write a coredump under 'ulimit -c' control and related changes. > > The idea here is to make debugging using a coredump work as usual on a unix, > e.g.: > > $ ulimit -c unlimited > > $ ./segv-program > *** starting '"C:\cygwin64\bin\dumper.exe" > "C:\cygwin64\work\segv-program.exe" 16156' for pid 1398, tid 7136 > > $ gdb segv-program.exe segv-program.exe.core > [...] > > Jon Turney (5): > Cygwin: Make 'ulimit -c' control writing a coredump > Cygwin: Disable writing core dumps by default. > Cygwin: Define and use __WCOREFLAG > Cygwin: Treat api_fatal() similarly to a core-dumping signal > Cygwin: Update documentation for cygwin_stackdump > > winsup/cygwin/dcrt0.cc| 6 +- > winsup/cygwin/environ.cc | 1 + > winsup/cygwin/exceptions.cc | 122 ++ > winsup/cygwin/include/cygwin/wait.h | 5 +- > winsup/cygwin/local_includes/winsup.h | 2 + > winsup/cygwin/mm/cygheap.cc | 2 +- > winsup/cygwin/release/3.5.0 | 7 ++ > winsup/doc/cygwinenv.xml | 25 -- > winsup/doc/misc-funcs.xml | 4 + > winsup/doc/new-features.xml | 12 +++ > winsup/doc/utils.xml | 43 + > 11 files changed, 180 insertions(+), 49 deletions(-) > > -- > 2.43.0 This patchset looks good to me, except one typo in the last patch of the series... Thanks, Corinna
Re: [PATCH 1/2] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 12 14:09, Jon Turney wrote: > On 11/01/2024 09:42, Corinna Vinschen wrote: > > I see. It's a bit unfortunate though, if dumper tries to create > > a 2 Gigs file which is later truncated, if we're low on disk space. > > But yeah, disk space isn't much of a problem these days, I guess... > > Assuming there isn't a clear specification of which of these is supposed to > happen, I think removing is the better choice, since partial coredumps are > just useless. > > (There's still some potential lossage if the coredump is big enough to fill > the disk, but less than the (perhaps badly-chosen) ulimit. But maybe that > could be fixed by having dumper remove the file if it couldn't be written > successfully)) The Linux kernel actually writes blocks until the next write would overrun RLIMIT_CORE. It would be nice if we could get some similar behaviour, but that's something for post 3.5. Corinna
Re: [PATCH 1/2] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 10 17:38, Jon Turney wrote:
> On 10/01/2024 15:30, Corinna Vinschen wrote:
> > On Jan 10 13:57, Jon Turney wrote:
> [...]
> > >
> > > Also: Fix the (deprecated) cygwin_dumpstack() function so it will now
> > > write a .stackdump file, even when ulimit -c is zero. (Note that
> > > cygwin_dumpstack() is still idempotent, which is perhaps odd)
> >
> > Given it's deprecated and not exposed in the headers, and given
> > we only still need the symbol for backward compat, how about making
> > this function a no-op instead?
>
> We still need the function internally to write stackdumps.
Oh, right, I missed the usage in api_fatal. I was only talking
about the exported function, or rather, the fact that it's exported.
We could split it in internal and external function and...
> I know it's use has long been discouraged, but doing a GitHub code search
> does find some uses of it. What is the suggested replacement?
...doing nothing in the exported function was the idea. There appear to
be a handful of projects on github though, which call it. Not sure it's
the right thing to do, though. External code should better raise a
signal in this case.
However, if we take it as given, and if external code calls
cygwin_stackdump, do we really want it to create a stackdump, or
shouldn't the behaviour be the same as if a core-creating signal has
been raised? See below.
> (I'm also wondering if the idempotency is in the wrong place. Is it
> possible for signal_exit() get called by multiple threads? In which case it
> probably needs to do something sane in that case)
signal_exit is only called from sigpacket::process, and this method
in turn is only called from the wait_sig function, so it's only
called from the signal thread.
I just wonder if we really want to create a stackdump unconditionally
at all, after introducing corefile support and handling ulimit the
way you do it.
I.e., we have (basically) three situations:
- A core-creating signal has been raised
- api_fatal calls cygwin_stackdump
- External code calls cygwin_stackdump
Wouldn't it make sense to handle them equally, depending on
the ulimit settings?
> > Can't this be done by adding the max size as parameter to dumper?
> >
>
> Maybe. That would make forward/backwards compatibility problems when mixing
> dumper and cygwin versions.
How's that supposed to happen? dumper is part of the Cygwin package,
so, together with using the right absolute path, there's no way to
use the wrong dumper version. If so, it's certainly an SEP.
> I don't think we can control the size of the file as we write it, we'd need
> to check afterwards if it was too big, and then remove/truncate.
>
> And we need to do the same action for stackdumps, so I think it makes more
> sense to do that checking in the DLL.
I see. It's a bit unfortunate though, if dumper tries to create
a 2 Gigs file which is later truncated, if we're low on disk space.
But yeah, disk space isn't much of a problem these days, I guess...
> [...]
> > > diff --git a/winsup/cygwin/environ.cc b/winsup/cygwin/environ.cc
> > > index 008854a07..dca5c5db0 100644
> > > --- a/winsup/cygwin/environ.cc
> > > +++ b/winsup/cygwin/environ.cc
> > > @@ -832,6 +832,7 @@ environ_init (char **envp, int envc)
> > > out:
> > > findenv_func = (char * (*)(const char*, int*)) my_findenv;
> > > environ = envp;
> > > + dumper_init ();
> >
> > Sorry, but I don't quite understand why dumper_init is called so early
> > and unconditionally. Why not create the command on the fly?
>
> For the same reason we create the error_start debugger command at process
> initialisation.
>
> If I had to guess, that's because calling malloc() when we're in the middle
> of crashing may not be very reliable.
>
> (of course, we go on to ruin this attention to detail by calling
> small_printf to append the Windows PID during exec_prepared_command(), even
> though we also knew that at process startup)
>
> [...]
> > > -extern "C" void
> > > -error_start_init (const char *buf)
> > > +static void
> > > +command_prep (const char *buf, PWCHAR *command)
> > > {
> > > - if (!buf || !*buf)
> > > -return;
> > > - if (!debugger_command &&
> > > - !(debugger_command = (PWCHAR) malloc ((2 * NT_MAX_PATH + 20)
> > > - * sizeof (WCHAR
> > > + if (!*command &&
> > > + !(*command = (PWCHAR) malloc ((2 * NT_MAX_PATH + 20)
> > > + * sizeof (WCHAR
> >
> > Not your fault, but the length of this string must not exceed
Re: [PATCH] Cygwin: Fix a stray '\n' in cygcheck manpage
On Jan 10 13:57, Jon Turney wrote: > --- > winsup/doc/utils.xml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/winsup/doc/utils.xml b/winsup/doc/utils.xml > index 8261e7ebd..692dae38f 100644 > --- a/winsup/doc/utils.xml > +++ b/winsup/doc/utils.xml > @@ -210,7 +210,7 @@ At least one command option or a PROGRAM is required, as > shown above. > plain console only, not from a pty/rxvt/xterm) >-e, --search-package list all available packages matching PATTERN > PATTERN is a glob pattern with * and ? as wildcard > chars > - search selection specifiers (multiple allowed):\n\ > + search selection specifiers (multiple allowed): >--requires list packages depending on packages matching PATTERN >--build-reqs list packages depending on packages matching PATTERN > when building these packages > -- > 2.42.1 Yep, thanks!
Re: [PATCH 2/2] Cygwin: Disable writing core dumps by default.
On Jan 10 13:57, Jon Turney wrote: > Change the default core limit from unlimited to 0 (disabled) > --- > winsup/cygwin/mm/cygheap.cc | 2 +- > winsup/doc/new-features.xml | 6 ++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/winsup/cygwin/mm/cygheap.cc b/winsup/cygwin/mm/cygheap.cc > index a20ee5972..3dc0c011f 100644 > --- a/winsup/cygwin/mm/cygheap.cc > +++ b/winsup/cygwin/mm/cygheap.cc > @@ -294,7 +294,7 @@ cygheap_init () >cygheap->locale.mbtowc = __utf8_mbtowc; >/* Set umask to a sane default. */ >cygheap->umask = 022; > - cygheap->rlim_core = RLIM_INFINITY; > + cygheap->rlim_core = 0; > } >if (!cygheap->fdtab) > cygheap->fdtab.init (); > diff --git a/winsup/doc/new-features.xml b/winsup/doc/new-features.xml > index b6daadc2b..a22b78a60 100644 > --- a/winsup/doc/new-features.xml > +++ b/winsup/doc/new-features.xml > @@ -99,6 +99,12 @@ is now written on a fatal error. Otherwise, if it's > greater than zero, a text > format .stackdump file is written, as previously. > > > + > +The default RLIMIT_CORE is now 0, disabling the generation of core dump or > +stackdump files. Use e.g. ulimit -c unlimited or ulimit -c > +1024 to enable them again. > + > + > > > > -- > 2.42.1 +1
Re: [PATCH 1/2] Cygwin: Make 'ulimit -c' control writing a coredump
On Jan 10 13:57, Jon Turney wrote:
> Factor out pre-formatting a command to be executed on a fatal signal,
> and use that for both error_start (if present in the CYGWIN env var) and
> for 'dumper'.
>
> Factor out executing that command, so we can use it from try_to_debug()
> and when a fatal signal occurs.
>
> On a fatal signal, invoke dumper to write a core dump, if the core file
> size limit is greater than 1MB. Otherwise, if that limit is greater than
> 0, write a .stackdump file, as previously.
>
> Adjust and clarify the associated documentation.
>
> Also: Fix the (deprecated) cygwin_dumpstack() function so it will now
> write a .stackdump file, even when ulimit -c is zero. (Note that
> cygwin_dumpstack() is still idempotent, which is perhaps odd)
Given it's deprecated and not exposed in the headers, and given
we only still need the symbol for backward compat, how about making
this function a no-op instead?
> Also: Fix so that the error_start JIT debugger is now invoked, even when
> ulimit -c is zero.
>
> Also: Fix uses of console_printf() inside exec_debugger(). It's output
> is written via the Windows console device, so needs to use Windows-style
> line endings.
>
> Future work: Perhaps we should use the absolute path to dumper, rather
> than assuming it is in PATH, although circumstances in which cygwin1.dll
> can be loaded, but dumper isn't in the PATH are probably rare.
I'm not so sure about that. It's pretty simple to get an absolute
path from the DLL path, so I would really make sure that the right
dumper is called. Otherwise this sounds a little bit like a security
problem, if the current PATH may decide over the actual dumper.exe,
isn't it?
> Future work: truncate or remove the file written, if it exceeds the
> maximum size set by the ulimit.
Can't this be done by adding the max size as parameter to dumper?
> Future work: the language around using the words "fatal error" could
> probably be improved. These are the "certain signals whose default
> action is to cause the process to terminate and produce a core dump
> file".
> ---
> winsup/cygwin/environ.cc | 1 +
> winsup/cygwin/exceptions.cc | 100 ++
> winsup/cygwin/local_includes/winsup.h | 1 +
> winsup/doc/cygwinenv.xml | 25 +--
> winsup/doc/new-features.xml | 6 ++
> winsup/doc/utils.xml | 43 +++
> 6 files changed, 125 insertions(+), 51 deletions(-)
>
> diff --git a/winsup/cygwin/environ.cc b/winsup/cygwin/environ.cc
> index 008854a07..dca5c5db0 100644
> --- a/winsup/cygwin/environ.cc
> +++ b/winsup/cygwin/environ.cc
> @@ -832,6 +832,7 @@ environ_init (char **envp, int envc)
> out:
>findenv_func = (char * (*)(const char*, int*)) my_findenv;
>environ = envp;
> + dumper_init ();
Sorry, but I don't quite understand why dumper_init is called so early
and unconditionally. Why not create the command on the fly?
>if (envp_passed_in)
> {
> p = getenv ("CYGWIN");
> diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
> index 642afb788..a71fd4fb0 100644
> --- a/winsup/cygwin/exceptions.cc
> +++ b/winsup/cygwin/exceptions.cc
> @@ -52,6 +52,7 @@ details. */
> #define DUMPSTACK_FRAME_LIMIT32
>
> PWCHAR debugger_command;
> +PWCHAR dumper_command;
> extern uint8_t _sigbe;
> extern uint8_t _sigdelayed_end;
>
> @@ -113,18 +114,16 @@ init_console_handler (bool install_handler)
> system_printf ("SetConsoleCtrlHandler failed, %E");
> }
>
> -extern "C" void
> -error_start_init (const char *buf)
> +static void
> +command_prep (const char *buf, PWCHAR *command)
> {
> - if (!buf || !*buf)
> -return;
> - if (!debugger_command &&
> - !(debugger_command = (PWCHAR) malloc ((2 * NT_MAX_PATH + 20)
> - * sizeof (WCHAR
> + if (!*command &&
> + !(*command = (PWCHAR) malloc ((2 * NT_MAX_PATH + 20)
> + * sizeof (WCHAR
Not your fault, but the length of this string must not exceed 32767 wide
chars, incuding the trailing \0 per
https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
The only reason I can see for using these large arrays is to avoid
length checks.
We could get away with two static 64K pages for debugger_command and
dumper_command. Or even with one if we just copy the required stuff
into the single debugger_command array when required. That would also
drop the requirement for the extra allocation in initial_env().
> +extern "C" void
> +dumper_init(void)
^^^
space
> +{
> + command_prep("dumper", _command);
^^^
space
> +}
> +
> +extern "C" void
> +error_start_init (const char *buf)
> +{
> + if (!buf || !*buf)
> +return;
> +
> + command_prep(buf, _command);
^^^
space
> +}
> +
> void
>
Re: [PATCH] Cygwin: Add '--names-only' flag to cygcheck
On Nov 24 17:06, Jon Turney wrote: > Add '--names-only' flag to cygcheck, to output just the bare package > names. Push it! > --- > > Notes: > Rather than more hacky aftermarket solutions, let's make cygcheck output > something more useful for feeding into setup. > > Next step would be to adjust setup's argument parsing so 'setup -P > "$(cygcheck -n)"' works as expected. Or cygcheck could just create a comma-separated list? Either way is fine. Thanks, Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks
On Nov 22 17:31, Christian Franke wrote: > Hi Corinna, > > Corinna Vinschen wrote: > > Hi Christian, > > > > > > On second thought... > > > > I had a bad night tonight and was thinking a long time about this and > > that. It suddenly occured to me that there might be another problem > > with this approach, attaching ordinals to the label name. > > > > Assuming you have a single filesystem labled "VOLUME" which is on a > > fixed disk. So you get something like this: > > > >$ ls -l /dev/disk/by-label > > total 0 > >lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:09 VOLUME -> ../../sdb1 > >lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:10 root -> ../../sda3 > > > > Now you insert an USB Stick with a FAT32 filesystem, also labeled > > "VOLUME". Now you get something like this: > > > >$ ls -l /dev/disk/by-label > >total 0 > >lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:12 'VOLUME#0' -> ../../sdb1 > >lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:12 'VOLUME#1' -> ../../sdc1 > >lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:10 root -> ../../sda3 > > > > So the label name changes, depending on inserting or removing another > > partition. > > This is intentional. If the first duplicate appears, it is IMO better to > also replace the original name to show that a duplicate exists. > > > > > > Not saying I have a good solution myself, so I wonder if we should just > > let it slip, but I thought we should at least talk about it... > > Users should be aware that unspecific label names like VOLUME could not be > used as a persistent link if drives are changed. > > Same may apply to by-partuuid names as preformatted SD-cards and USB flash > drives may have a null MBR serial number. Makes total sense. Thanks, Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks
Hi Christian, On second thought... I had a bad night tonight and was thinking a long time about this and that. It suddenly occured to me that there might be another problem with this approach, attaching ordinals to the label name. Assuming you have a single filesystem labled "VOLUME" which is on a fixed disk. So you get something like this: $ ls -l /dev/disk/by-label total 0 lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:09 VOLUME -> ../../sdb1 lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:10 root -> ../../sda3 Now you insert an USB Stick with a FAT32 filesystem, also labeled "VOLUME". Now you get something like this: $ ls -l /dev/disk/by-label total 0 lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:12 'VOLUME#0' -> ../../sdb1 lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:12 'VOLUME#1' -> ../../sdc1 lrwxrwxrwx 1 corinna vinschen 0 Nov 22 10:10 root -> ../../sda3 So the label name changes, depending on inserting or removing another partition. Not saying I have a good solution myself, so I wonder if we should just let it slip, but I thought we should at least talk about it... Thanks, Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks
On Nov 21 19:31, Christian Franke wrote:
> Corinna Vinschen wrote:
> > Hi Christian,
> >
> > Looks good, but I just realized that I was already wondering about the
> > sanitization and forgot to talk about it:
> >
> > On Nov 21 12:24, Christian Franke wrote:
> > > diff --git a/winsup/cygwin/fhandler/dev_disk.cc
> > > b/winsup/cygwin/fhandler/dev_disk.cc
> > > index c5d72816f..d12ac52fa 100644
> > > --- a/winsup/cygwin/fhandler/dev_disk.cc
> > > +++ b/winsup/cygwin/fhandler/dev_disk.cc
> > > @@ -64,10 +64,12 @@ sanitize_label_string (WCHAR *s)
> > > /* Linux does not skip leading spaces. */
> > > return sanitize_string (s, L'\0', L' ', L'_', [] (WCHAR c) -> bool
> > > {
> > > - /* Labels may contain characters not allowed in filenames.
> > > - Linux replaces spaces with \x20 which is not an option here. */
> > > + /* Labels may contain characters not allowed in filenames. Also
> > Apart from slash and backslash, we don't have this problem in Cygwin,
> > usually. Even control characters are no problem. All chars not allowed
> > in filenames are just transposed into the Unicode private use area, as
> > per strfuncs.cc, line 20ff on the way to storage, and back when reading
> > the names from storage. This, and especially in a virtual filesystem
> > like /proc, there's no reason to avoid these characters.
>
> Thanks for clarification.
>
>
> >
> > > + replace '#' to avoid that duplicate markers introduce new
> > > + duplicates. Linux replaces spaces with \x20 which is not an
> > > + option here. */
> > > return !((0 <= c && c <= L' ') || c == L':' || c == L'/' || c ==
> > > L'\\'
> > > - || c == L'"');
> > > + || c == L'#' || c == L'"');
> > If you really want to avoid chars not allowed in DOS filenames, the
> > list seems incomplete, missing '<', '>', '?', '*', '|'.
> >
> > But as I said, there's really no reason for that. I simply reduced the
> > above expression to
> >
> >return !(c == L'/' || c == L'\\' || c == L'#');
> >
> > and created a disk label
> >
> >test"foo*bar?baz:"
> >
> > It works nicely, including stuff like
> >
> >$ ls *\"*
> >$ ls *\**
> >
> > So, I can push it as is, or we just allow everything and the kitchen sink
> > as per the reduced filter expression. What do you prefer?
>
> The latter - patch attached.
Pushed.
Thanks a lot,
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks
Hi Christian,
Looks good, but I just realized that I was already wondering about the
sanitization and forgot to talk about it:
On Nov 21 12:24, Christian Franke wrote:
> diff --git a/winsup/cygwin/fhandler/dev_disk.cc
> b/winsup/cygwin/fhandler/dev_disk.cc
> index c5d72816f..d12ac52fa 100644
> --- a/winsup/cygwin/fhandler/dev_disk.cc
> +++ b/winsup/cygwin/fhandler/dev_disk.cc
> @@ -64,10 +64,12 @@ sanitize_label_string (WCHAR *s)
>/* Linux does not skip leading spaces. */
>return sanitize_string (s, L'\0', L' ', L'_', [] (WCHAR c) -> bool
> {
> - /* Labels may contain characters not allowed in filenames.
> - Linux replaces spaces with \x20 which is not an option here. */
> + /* Labels may contain characters not allowed in filenames. Also
Apart from slash and backslash, we don't have this problem in Cygwin,
usually. Even control characters are no problem. All chars not allowed
in filenames are just transposed into the Unicode private use area, as
per strfuncs.cc, line 20ff on the way to storage, and back when reading
the names from storage. This, and especially in a virtual filesystem
like /proc, there's no reason to avoid these characters.
> + replace '#' to avoid that duplicate markers introduce new
> + duplicates. Linux replaces spaces with \x20 which is not an
> + option here. */
>return !((0 <= c && c <= L' ') || c == L':' || c == L'/' || c == L'\\'
> - || c == L'"');
> + || c == L'#' || c == L'"');
If you really want to avoid chars not allowed in DOS filenames, the
list seems incomplete, missing '<', '>', '?', '*', '|'.
But as I said, there's really no reason for that. I simply reduced the
above expression to
return !(c == L'/' || c == L'\\' || c == L'#');
and created a disk label
test"foo*bar?baz:"
It works nicely, including stuff like
$ ls *\"*
$ ls *\**
So, I can push it as is, or we just allow everything and the kitchen sink
as per the reduced filter expression. What do you prefer?
Thanks,
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks
On Nov 20 15:54, Christian Franke wrote:
> From: Christian Franke
> Date: Mon, 20 Nov 2023 15:40:42 +0100
> Subject: [PATCH] Cygwin: /dev/disk/by-uuid: Fix NTFS serial number print
> format
>
> Signed-off-by: Christian Franke
> ---
> winsup/cygwin/fhandler/dev_disk.cc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/winsup/cygwin/fhandler/dev_disk.cc
> b/winsup/cygwin/fhandler/dev_disk.cc
> index 016b4c7bc..c5d72816f 100644
> --- a/winsup/cygwin/fhandler/dev_disk.cc
> +++ b/winsup/cygwin/fhandler/dev_disk.cc
> @@ -308,7 +308,7 @@ partition_to_label_or_uuid(bool uuid, const
> UNICODE_STRING *drive_uname,
>&& nvdb->VolumeSerialNumber.QuadPart)
> {
>/* Print without any separator as on Linux. */
> - __small_sprintf (name, "%16X", nvdb->VolumeSerialNumber.QuadPart);
> + __small_sprintf (name, "%016X", nvdb->VolumeSerialNumber.QuadPart);
>NtClose(volhdl);
>return true;
> }
> --
> 2.42.1
>
Pushed.
Thanks,
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks
On Nov 20 10:40, Corinna Vinschen wrote:
> Hi Christian,
>
> This puzzles me:
>
> On Nov 17 21:25, Christian Franke wrote:
> > @@ -610,7 +607,7 @@ get_by_id_table (by_id_entry * ,
> > fhandler_dev_disk::dev_disk_location loc)
> >if (!table)
> > return (errno_set ? -1 : 0);
> >
> > - /* Sort by name and remove duplicates. */
> > + /* Sort by name and mark duplicates. */
> >qsort (table, table_size, sizeof (*table), by_id_compare_name);
> >for (unsigned i = 0; i < table_size; i++)
>
> by_id_compare_name only compars the actual names...
>
> > {
> > @@ -619,12 +616,13 @@ get_by_id_table (by_id_entry * ,
> > fhandler_dev_disk::dev_disk_location loc)
> > j++;
> >if (j == i + 1)
> > continue;
> > - /* Duplicate(s) found, remove all entries with this name. */
> > - debug_printf ("removing duplicates %d-%d: '%s'", i, j - 1,
> > table[i].name);
> > - if (j < table_size)
> > - memmove (table + i, table + j, (table_size - j) * sizeof (*table));
> > - table_size -= j - i;
> > - i--;
> > + /* Duplicate(s) found, append "#N" to all entries. This never
>
> ...but the names are identical. So the *order* within the identically
> named entries depends on qsort's reshuffling of table
> entries. Which in turn depends on outside factors like number of table
> entries and the ultimate position of the identical entries within the
> ordered table.
>
> Having said that, I don't see how adding ordinals to the names can be
> unambiguous. AFAICS, the numbers may change by just adding another
> disk (USB Stick) to the system...
Oops, that's not exactly what I was trying to say, sorry.
The problem is not adding ordinals to the name, AFAICS, the problem is
that the sorting function by_id_compare_name is not up to the task to
make sure the order is unambiguous within the entries of identical name.
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks
Hi Christian,
This puzzles me:
On Nov 17 21:25, Christian Franke wrote:
> @@ -610,7 +607,7 @@ get_by_id_table (by_id_entry * ,
> fhandler_dev_disk::dev_disk_location loc)
>if (!table)
> return (errno_set ? -1 : 0);
>
> - /* Sort by name and remove duplicates. */
> + /* Sort by name and mark duplicates. */
>qsort (table, table_size, sizeof (*table), by_id_compare_name);
>for (unsigned i = 0; i < table_size; i++)
by_id_compare_name only compars the actual names...
> {
> @@ -619,12 +616,13 @@ get_by_id_table (by_id_entry * ,
> fhandler_dev_disk::dev_disk_location loc)
> j++;
>if (j == i + 1)
> continue;
> - /* Duplicate(s) found, remove all entries with this name. */
> - debug_printf ("removing duplicates %d-%d: '%s'", i, j - 1,
> table[i].name);
> - if (j < table_size)
> - memmove (table + i, table + j, (table_size - j) * sizeof (*table));
> - table_size -= j - i;
> - i--;
> + /* Duplicate(s) found, append "#N" to all entries. This never
...but the names are identical. So the *order* within the identically
named entries depends on qsort's reshuffling of table
entries. Which in turn depends on outside factors like number of table
entries and the ultimate position of the identical entries within the
ordered table.
Having said that, I don't see how adding ordinals to the names can be
unambiguous. AFAICS, the numbers may change by just adding another
disk (USB Stick) to the system...
Thanks,
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks
On Nov 17 18:53, Christian Franke wrote: > Corinna Vinschen wrote: > > On Nov 17 17:45, Christian Franke wrote: > > > Corinna Vinschen wrote: > > > > On Nov 17 15:39, Christian Franke wrote: > > > > > The last two /dev/disk subdirectories :-) > > > > > > > > > > Note a minor difference: On Linux, empty /dev/disk subdirectories > > > > > apparently > > > > > never appear. A subdirectory is not listed in /dev/disk if it would be > > > > > empty. Not worth the effort to emulate. > > > > Agreed. This is really great. I just pushed your patch. > > > > > > > > However, there's something strange in terms of by-label: > > > > > > > > I have two partitions with labels: > > > > > > > > $ ls -l /dev/disk/by-label > > > > total 0 > > > > lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub -> ../../sda3 > > > > lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub2 -> ../../sdb2 > > > > $ > > > > > > > > Now I change the label of sdb2 to the same "blub" string as on sda3: > > > > > > > > $ ls -l /dev/disk/by-label > > > > total 0 > > > > $ > > > > > > > > I'd expected to see only one, due to the name collision, but en empty > > > > dir is a bit surprising... And it may occur more often than not, given > > > > that the default label "New_Volume" probably won't get changed very > > > > often. > > > > > > > This is intentional and inherited from the very first patch, see the loop > > > behind qsort(). If a range of identical names appear, all these entries > > > are > > > removed. If some "random" entry would be kept, it might no longer be the > > > persistent link the user expects. We could possibly add some hash like > > > done > > > for by-id or append a number in such cases later. Need some more time to > > > thing about it > > I see. Admittedly, I don't know how Linux handles this either. > > A quick test on Debian 12 with by-label suggests that the last duplicate > wins. Also not very sophisticated :-) Given this is all controlled by rather simple udev rules, see /usr/lib/udev/rules.d/60-persistent-storage.rules, that's not really surprising. > IIRC in the past I've seen in another of these directories (by-id?) that > '#N' was appended if duplicates occur. I don't see anything like that in 60-persistent-storage.rules, though. It has been removed at one point, it seems. > > > I will sent a patch for the new-features doc soon. > > Attached. Thanks, pushed. Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks
On Nov 17 17:45, Christian Franke wrote: > Corinna Vinschen wrote: > > On Nov 17 15:39, Christian Franke wrote: > > > The last two /dev/disk subdirectories :-) > > > > > > Note a minor difference: On Linux, empty /dev/disk subdirectories > > > apparently > > > never appear. A subdirectory is not listed in /dev/disk if it would be > > > empty. Not worth the effort to emulate. > > Agreed. This is really great. I just pushed your patch. > > > > However, there's something strange in terms of by-label: > > > > I have two partitions with labels: > > > >$ ls -l /dev/disk/by-label > >total 0 > >lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub -> ../../sda3 > >lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub2 -> ../../sdb2 > >$ > > > > Now I change the label of sdb2 to the same "blub" string as on sda3: > > > >$ ls -l /dev/disk/by-label > >total 0 > >$ > > > > I'd expected to see only one, due to the name collision, but en empty > > dir is a bit surprising... And it may occur more often than not, given > > that the default label "New_Volume" probably won't get changed very > > often. > > > > This is intentional and inherited from the very first patch, see the loop > behind qsort(). If a range of identical names appear, all these entries are > removed. If some "random" entry would be kept, it might no longer be the > persistent link the user expects. We could possibly add some hash like done > for by-id or append a number in such cases later. Need some more time to > thing about it I see. Admittedly, I don't know how Linux handles this either. > I will sent a patch for the new-features doc soon. Thanks, Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-label and /dev/disk/by-uuid symlinks
On Nov 17 15:39, Christian Franke wrote: > The last two /dev/disk subdirectories :-) > > Note a minor difference: On Linux, empty /dev/disk subdirectories apparently > never appear. A subdirectory is not listed in /dev/disk if it would be > empty. Not worth the effort to emulate. Agreed. This is really great. I just pushed your patch. However, there's something strange in terms of by-label: I have two partitions with labels: $ ls -l /dev/disk/by-label total 0 lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub -> ../../sda3 lrwxrwxrwx 1 corinna vinschen 0 Nov 17 17:18 blub2 -> ../../sdb2 $ Now I change the label of sdb2 to the same "blub" string as on sda3: $ ls -l /dev/disk/by-label total 0 $ I'd expected to see only one, due to the name collision, but en empty dir is a bit surprising... And it may occur more often than not, given that the default label "New_Volume" probably won't get changed very often. Thanks, Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-drive and /dev/disk/by-uuid symlinks
On Nov 16 18:02, Christian Franke wrote: > Corinna Vinschen wrote: > > On Nov 16 12:50, Christian Franke wrote: > > ... > > > > I also tried an NTFS partition and the output looks like this: > > > > > > > > 0FD4F62866CFBF09 -> ../../sdc1 > > > > > > > > This is the 64 bit volume serial number as returned by > > > > DeviceIoControl(FSCTL_GET_NTFS_VOLUME_DATA)(*). > > > > > > > > Wouldn't that be what we want to see, too? > > > Hmm.. yes. Should both information be provided in by-uuid or only the > > > serial numbers? In the latter case, should we add e.g. by-voluuid for the > > > volume GUIDs ? > > Good question... by-voluuid sounds like a nice idea. It's a Windows-only > > concept anyway, so it might make sense to present it in its own directory. > > Then by-voluuid is easy, changed patch is attached. Pushed. > I try to provide a patch > for a new by-uuid with filesystem serial numbers soon. Cool! Thanks, Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-drive and /dev/disk/by-uuid symlinks
On Nov 16 12:50, Christian Franke wrote:
> Corinna Vinschen wrote:
> > Hi Christian,
> >
> > On Nov 15 18:23, Christian Franke wrote:
> > > This is the next (and possibly last for now) extension to the /dev/disk
> > > directory. Limited to disk related entries which allowed a straightforward
> > > extension of the existing code.
> > >
> > > My original idea was to add also other drive letters and volume GUIDs. Too
> > > complex for now.
> > >
> > > Interestingly the volume GUID (by-uuid) for partitions on MBR disks is
> > > sometimes identical to the partition "GUID" (by-partuuid), sometimes
> > > (always
> > > for C:?) not. With GPT disks, both GUIDs are possibly always identical.
> > That looks great, but in terms of by-uuid, I'm not sure it's the
> > right thing to do. On Linux I have a vfat partition (/boot/efi).
> > The uuid in /dev/disk/by-uuid is the volume serial number, just
> > with an extra dash, i.e.
> >
> >057A-B3A7 -> ../../sda1
> >
> > That's what you get for FAT/FAT32/exFAT.
>
> What is the best way to retrieve a FAT* serial? There is
> GetVolumeInformation{ByHandleW}(), but this may not work with the NT Layer
> pathnames / handles used here.
It will work, because Windows and NT handles are the same thing as long
as you created the handle using an operation opening kernel objects, as,
for instance, files or volumes.
Actually, GetVolumeInformation() is the combination of multiple
NtQueryVolumeInformationFile calls. The call retrieving the
serial number is NtQueryVolumeInformationFile(..., FileFsVolumeInformation),
as is used in fs_info::update, mount.cc, line 250 ATM.
> > I also tried an NTFS partition and the output looks like this:
> >
> >0FD4F62866CFBF09 -> ../../sdc1
> >
> > This is the 64 bit volume serial number as returned by
> > DeviceIoControl(FSCTL_GET_NTFS_VOLUME_DATA)(*).
> >
> > Wouldn't that be what we want to see, too?
>
> Hmm.. yes. Should both information be provided in by-uuid or only the
> serial numbers? In the latter case, should we add e.g. by-voluuid for the
> volume GUIDs ?
Good question... by-voluuid sounds like a nice idea. It's a Windows-only
concept anyway, so it might make sense to present it in its own directory.
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-drive and /dev/disk/by-uuid symlinks
Hi Christian, On Nov 15 18:23, Christian Franke wrote: > This is the next (and possibly last for now) extension to the /dev/disk > directory. Limited to disk related entries which allowed a straightforward > extension of the existing code. > > My original idea was to add also other drive letters and volume GUIDs. Too > complex for now. > > Interestingly the volume GUID (by-uuid) for partitions on MBR disks is > sometimes identical to the partition "GUID" (by-partuuid), sometimes (always > for C:?) not. With GPT disks, both GUIDs are possibly always identical. That looks great, but in terms of by-uuid, I'm not sure it's the right thing to do. On Linux I have a vfat partition (/boot/efi). The uuid in /dev/disk/by-uuid is the volume serial number, just with an extra dash, i.e. 057A-B3A7 -> ../../sda1 That's what you get for FAT/FAT32/exFAT. I also tried an NTFS partition and the output looks like this: 0FD4F62866CFBF09 -> ../../sdc1 This is the 64 bit volume serial number as returned by DeviceIoControl(FSCTL_GET_NTFS_VOLUME_DATA)(*). Wouldn't that be what we want to see, too? Thanks, Corinna (*) Incidentally the last 8 digits represent the crippled 4 byte serial number returned by NtQueryVolumeInformationFile(..., FileFsVolumeInformation).
Re: [PATCH v2] Fix profiler error() definition and usage
On Nov 14 00:58, Mark Geisert wrote:
> Minor updates to profiler and gmondump, which share some code:
> - fix operation of error() so it actually works as intended
> - resize 4K-size auto buffer reservations to BUFSIZ (==1K)
> - remove trailing '\n' from 2nd arg on error() calls everywhere
> - provide consistent annotation of Windows error number displays
>
> Fixes: 9887fb27f6126 ("Cygwin: New tool: profiler")
> Fixes: 087a3d76d7335 ("Cygwin: New tool: gmondump")
> Signed-off-by: Mark Geisert
>
> ---
> winsup/cygwin/release/3.4.10 | 3 +++
> winsup/utils/gmondump.c | 8 ---
> winsup/utils/profiler.cc | 46 +++-
> 3 files changed, 32 insertions(+), 25 deletions(-)
Pushed.
Thanks,
Corinna
Re: [PATCH] Cygwin: Fix profiler error() definition and usage
Hi Mark,
On Nov 13 01:46, Mark Geisert wrote:
> Minor updates to profiler and gmondump, which share some code:
> - fix operation of error() so it actually works as intended
> - resize 4K-size auto buffer reservations to BUFSIZ (==1K)
> - remove trailing '\n' from 2nd arg on error() calls everywhere
> - provide consistent annotation of Windows error number displays
>
> Fixes: 9887fb27f6126 ("Cygwin: New tool: profiler")
> Fixes: 087a3d76d7335 ("Cygwin: New tool: gmondump")
> Signed-off-by: Mark Geisert
Looks good basically, but I noticed some minor problem already
in the former version of this code:
> @@ -650,7 +652,7 @@ ctrl_c (DWORD)
>static int tic = 1;
>
>if ((tic ^= 1) && !GenerateConsoleCtrlEvent (CTRL_C_EVENT, 0))
> -error (0, "couldn't send CTRL-C to child, win32 error %d\n",
> +error (0, "couldn't send CTRL-C to child, Windows error %d",
> GetLastError ());
>return TRUE;
GetLastError returns a DWORD == unsigned int. %u would be the
right format specifier. Care to fix that, too?
Thanks,
Corinna
Re: [PATCH] fix(libc): Fix handle of %E & %O modifiers at end of format string
Hi Pedro, On Nov 11 18:29, Pedro Luis Castedo Cepeda wrote: > OK. It's not a newlib problem but a GLib one as it is relaying on common but > non-standard strftime implementation details. > > I attach a short program more focused in g_date_strftime implementation so > it can be evaluated if it worths addressing this corner case. Tricky. I wonder what the GLib test is actually trying to accomplish. POSIX has this to say: RETURN VALUE If the total number of resulting bytes including the terminating null byte is not more than maxsize, these functions shall return the number of bytes placed into the array pointed to by s, not including the terminating NUL character. Otherwise, 0 shall be returned and the contents of the array are unspecified. ERRORS No errors are defined. But, and that's the big problem, POSIX does *not* provide for the error case, because it doesn't allow an error like using an incorrect format string to occur. Using an incorrect or undefined format code is just not part of the standard. And the Linux man page has an interesting extension to the above POSIX RETURN VALUE section: Note that the return value 0 does not necessarily indicate an error. For example, in many locales %p yields an empty string. An empty format string will likewise yield an empty string. and additionally in the BUGS section: If the output string would exceed max bytes, errno is not set. This makes it impossible to distinguish this error case from cases where the format string legitimately produces a zero-length output string. POSIX.1-2001 does not specify any errno settings for strftime(). So the below case tested by GLib is entirely out of scope of the standard. Corinna
Re: [PATCH] fix(libc): Fix handle of %E & %O modifiers at end of format string
On Nov 9 23:17, Brian Inglis wrote:
> On 2023-11-09 12:04, Pedro Luis Castedo Cepeda wrote:
> > - Prevent strftime to parsing format string beyond its end when
> >it finish with "%E" or "%O".
> > ---
> > newlib/libc/time/strftime.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/newlib/libc/time/strftime.c b/newlib/libc/time/strftime.c
> > index 56f227c5f..c4e9e45a9 100644
> > --- a/newlib/libc/time/strftime.c
> > +++ b/newlib/libc/time/strftime.c
> > @@ -754,6 +754,8 @@ __strftime (CHAR *s, size_t maxsize, const CHAR *format,
> > switch (*format)
> > {
> > + case CQ('\0'):
> > + break;
> > case CQ('a'):
> > _ctloc (wday[tim_p->tm_wday]);
> > for (i = 0; i < ctloclen; i++)
>
> These cases appear to already be taken care of by setting and using
> (depending on the config parameters) the "alt" variable for those modifiers,
> and the default: return 0; for the format *character* (possibly wide) not
> matching following any modifiers.
>
> Patches to newlib should go to the newlib mailing list at sourceware dot org.
Also, a simple reproducer would be nice.
Thanks,
Corinna
Re: [PATCH] Cygwin: /dev/disk/by-id: Remove leading spaces from identify fields
On Nov 8 17:40, Christian Franke wrote: > Minor improvement, avoids "/dev/disk/by-id/sata-VENDOR_MODEL___SERIAL". Pushed. Thanks, Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
On Nov 7 15:30, Christian Franke wrote: > Corinna Vinschen wrote: > > .. > > Looking forward to it. We'll just need an entry for the release text > > in winsup/cygwin/release/3.5.0 and doc/new-features.xml in the end :) > > Attached for now as implementing the remaining subdirs is not yet scheduled. > Docbook formatting not tested. Looks good, pushed. Thanks, Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
On Nov 7 11:10, Christian Franke wrote:
> Corinna Vinschen wrote:
> > Hi Christian,
> >
> > On Nov 5 16:45, Christian Franke wrote:
> > > ...
> > > Old IOCTL dropped and code simplified.
> > Great. I pushed your patch.
>
> Thanks.
>
>
> > ...
> > > > Last, but not least, do you see a chance to add any other /dev/disk
> > > > subdir? by-partuuid, perhaps?
> > > Possibly, but not very soon. I'm not yet sure which API functions could be
> > > used.
> > > Some early draft ideas:
> > >
> > > /dev/disk/by-partuid (Partition UUID -> device)
> > > GPT_PART_UUID -> ../../sdXN (GPT partition UUID)
> > > MBR_SERIAL-partN -> ../../sdYM (Fake UUID for MBR)
> > That should only require IOCTL_DISK_GET_PARTITION_INFO_EX, I think.
>
> Easier than expected: DRIVE_LAYOUT_INFORMATION_EX already contains
> PARTITION_INFORMATION_EX so existing scanning function could be enhanced.
> Patch attached.
Nice, pushed!
> > > /dev/disk/by-uuid (Windows Volume UUID -> device)
> > > Vol_UUID1 -> ../../sdXN (disk volume)
> > > Vol_UUID2 -> ../../scd0 (CD/DVD drive volume)
> > > Vol_UUID3 -> /proc/sys/GLOBAL??/Volume{UUID} (others, e.g. VeraCrypt
> > > volume)
> > Yeah, tricky. These are not the partition GUIDs but the filesystem
> > GUIDs or serial numbers. AFAICS, Windows filesystems (FAT*, NTFS)
> > don't maintain a filesystem GUID, as, e. g., ext4 or xfs, but only
> > serial numbers you can fetch via NtQueryVolumeInformationFile.
> > A Linux example of that is the serial number from a FAT32 filesytem
> > as the EFI boot partition in by-uuid:
> >
> >lrwxrwxrwx 1 root root 10 Oct 30 10:20 DC38-0407 -> ../../sda1
> >
> > On second thought, maybe that's sufficient for our by-uuid emulation.
> >
> > > /dev/disk/by-drive (Cygwin specific: drive letter -> volume)
> > > c -> ../by-uuid/UUID (if UUID available)
> > > x -> /proc/sys/DosDevices/X: (others, e.g. Network, "mounted" Volume
> > > Shadow Copy)
> > Ah, good idea. That's what my extension in /proc/partition already
> > provides, but a /dev/disk/by-drive sounds like a great idea.
>
> Left for later :-)
Looking forward to it. We'll just need an entry for the release text
in winsup/cygwin/release/3.5.0 and doc/new-features.xml in the end :)
Thanks,
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
Hi Christian,
On Nov 5 16:45, Christian Franke wrote:
> Corinna Vinschen wrote:
> > Do we really still need IOCTL_DISK_GET_DRIVE_LAYOUT w/o _EX?
>
> Possibly not. I borrowed this code from code behind /proc/partitions.
>
> > fhandler_proc still uses it, too, but the EX variation is available
> > since XP. I can't imagine that a fallback to the non-EX-version is still
> > necessary. If you like you could drop it immediately, or we can drop
> > both usages as a followup patch.
>
> Old IOCTL dropped and code simplified.
Great. I pushed your patch.
However, while I was just happily removing the non-EX ioctl's from
fhandler/proc.cc I found that this isn't feasible:
The EX calls are not supported by the floppy driver. D'uh.
So for /proc/partitions emulation we still need them.
> > Last, but not least, do you see a chance to add any other /dev/disk
> > subdir? by-partuuid, perhaps?
>
> Possibly, but not very soon. I'm not yet sure which API functions could be
> used.
> Some early draft ideas:
>
> /dev/disk/by-partuid (Partition UUID -> device)
> GPT_PART_UUID -> ../../sdXN (GPT partition UUID)
> MBR_SERIAL-partN -> ../../sdYM (Fake UUID for MBR)
That should only require IOCTL_DISK_GET_PARTITION_INFO_EX, I think.
> /dev/disk/by-uuid (Windows Volume UUID -> device)
> Vol_UUID1 -> ../../sdXN (disk volume)
> Vol_UUID2 -> ../../scd0 (CD/DVD drive volume)
> Vol_UUID3 -> /proc/sys/GLOBAL??/Volume{UUID} (others, e.g. VeraCrypt
> volume)
Yeah, tricky. These are not the partition GUIDs but the filesystem
GUIDs or serial numbers. AFAICS, Windows filesystems (FAT*, NTFS)
don't maintain a filesystem GUID, as, e. g., ext4 or xfs, but only
serial numbers you can fetch via NtQueryVolumeInformationFile.
A Linux example of that is the serial number from a FAT32 filesytem
as the EFI boot partition in by-uuid:
lrwxrwxrwx 1 root root 10 Oct 30 10:20 DC38-0407 -> ../../sda1
On second thought, maybe that's sufficient for our by-uuid emulation.
> /dev/disk/by-drive (Cygwin specific: drive letter -> volume)
> c -> ../by-uuid/UUID (if UUID available)
> x -> /proc/sys/DosDevices/X: (others, e.g. Network, "mounted" Volume
> Shadow Copy)
Ah, good idea. That's what my extension in /proc/partition already
provides, but a /dev/disk/by-drive sounds like a great idea.
Thanks,
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
Hi Christian,
patch looks pretty good to me. Just two minor points:
> + /* Traverse \Device directory ... */
> + bool errno_set = false;
> + HANDLE devhdl = INVALID_HANDLE_VALUE;
INVALID_HANDLE_VALUE is a Win32 API concept and is only returned by
CreateFile and friends. The native NT API doesn't know
INVALID_HANDLE_VALUE and, fortunately, only uses NULL. I think it's
puzzeling to use INVALID_HANDLE_VALUE in a native NT context. So, would
you mind to just use NULL (or nullptr) instead? As in...
> + if (devhdl != INVALID_HANDLE_VALUE)
if (devhdl)
Sounds easier to me :)
> + /* Fetch drive layout info to get size of all partitions on disk. */
> + DWORD part_cnt = 0;
> + PARTITION_INFORMATION_EX *pix = nullptr;
> + PARTITION_INFORMATION *pi = nullptr;
> + DWORD bytes_read;
> + if (DeviceIoControl (devhdl, IOCTL_DISK_GET_DRIVE_LAYOUT_EX, nullptr,
> 0,
> +ioctl_buf, NT_MAX_PATH, _read, nullptr))
> + {
> + DRIVE_LAYOUT_INFORMATION_EX *pdlix =
> + reinterpret_cast(ioctl_buf);
> + part_cnt = pdlix->PartitionCount;
> + pix = pdlix->PartitionEntry;
> + }
> + else if (DeviceIoControl (devhdl, IOCTL_DISK_GET_DRIVE_LAYOUT,
> nullptr,
Do we really still need IOCTL_DISK_GET_DRIVE_LAYOUT w/o _EX?
fhandler_proc still uses it, too, but the EX variation is available
since XP. I can't imagine that a fallback to the non-EX-version is still
necessary. If you like you could drop it immediately, or we can drop
both usages as a followup patch.
Last, but not least, do you see a chance to add any other /dev/disk
subdir? by-partuuid, perhaps?
Thanks,
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
On Nov 4 10:34, Corinna Vinschen wrote:
> On Nov 3 18:54, Christian Franke wrote:
> > Corinna Vinschen wrote:
> > > On Nov 3 17:27, Corinna Vinschen wrote:
> > > > On Nov 3 17:09, Christian Franke wrote:
> > > > > Unlike (S)ATA and NVMe, the serial number
> > > > > is not available for free in the device identify data block but
> > > > > requires an
> > > > > extra command (SCSI INQUIRY of VPD page 0x80). This might not be
> > > > > supported
> > > > > by the emulated controller or Windows does not use this command.
> > > > AFAICS, only the data from STORAGE_DEVICE_ID_DESCRIPTOR is available
> > > > which is equivalent to the data from VPD page 0x83. As you can see,
> > > > it's part of the STORAGE_DEVICE_UNIQUE_IDENTIFIER data. The data
> > > > returned for the VirtIo device is the identifier string "\x01\x00",
> > > > which is a bit underwhelming.
> > > >
> > > > Would be great if we would learn how to access page 0x80...
> > > Uhm...
> > >
> > > MSDN claims:
> > >
> > >If the storage device is SCSI-compliant, the port driver attempts to
> > >extract the serial number from the optional Unit Serial Number page
> > >(page 0x80) of the VPD.
> > >
> > > Now I'm puzzled.
> >
> > A quick test with a Debian 12 VM in VirtualBox with many virtual
> > controllers+drives shows the same problem:
> > Entries in /dev/disk/by-id appear only for virtual disks behind emulated
> > SATA and NVMe controllers, but not for SCSI and SAS controllers.
> > A test with "smartctl -i ..." with SCSI/SAS devices doesn't print a serial
> > number. In debug mode it prints "Vital Product Data (VPD) INQUIRY failed..."
> > and other messages that suggest limited/buggy support of optional SCSI
> > commands.
> >
> > If a Win11 PE (from install ISO) is run in same VM, the
> > STORAGE_DEVICE_DESCRIPTOR only provides the serial number for SATA (NVMe
> > drives not detected), but not for SCSI.
> >
> > Conclusion: The behavior of the current patch is compatible with Linux :-)
>
> Ok, but with the DUID we have a workaround which makes it work even
> better than on Linux, so it would begreat if we used it, unless we find
> out where the UUID in "\GLOBAL??\Disk{} comes from...
>
> Given the size of the STORAGE_DEVICE_UNIQUE_IDENTIFIER struct, we could
> even contemplate a 128 bit hash, just to be on the safe side.
Kind of like this
- strcat (name, ioctl_buf + desc->SerialNumberOffset);
+ /* Use SerialNumber in the first place, if available */
+ if (desc->SerialNumberOffset && desc_buf[desc->SerialNumberOffset])
+strcat (name, desc_buf + desc->SerialNumberOffset);
+ else /* Utilize the DUID as defined by MSDN to generate a hash */
+{
+ union {
+ unsigned __int128 all;
+ struct {
+ unsigned long high;
+ unsigned long low;
+ };
+ } hash = { 0 };
+
+ for (ULONG i = 0; i < id->Size; ++i)
+ hash.all = ioctl_buf[i] + (hash.all << 6) + (hash.all << 16) - hash.all;
+ __small_sprintf (name + strlen (name), "%X%X", hash.high, hash.low);
+}
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
On Nov 3 18:54, Christian Franke wrote:
> Corinna Vinschen wrote:
> > On Nov 3 17:27, Corinna Vinschen wrote:
> > > On Nov 3 17:09, Christian Franke wrote:
> > > > Unlike (S)ATA and NVMe, the serial number
> > > > is not available for free in the device identify data block but
> > > > requires an
> > > > extra command (SCSI INQUIRY of VPD page 0x80). This might not be
> > > > supported
> > > > by the emulated controller or Windows does not use this command.
> > > AFAICS, only the data from STORAGE_DEVICE_ID_DESCRIPTOR is available
> > > which is equivalent to the data from VPD page 0x83. As you can see,
> > > it's part of the STORAGE_DEVICE_UNIQUE_IDENTIFIER data. The data
> > > returned for the VirtIo device is the identifier string "\x01\x00",
> > > which is a bit underwhelming.
> > >
> > > Would be great if we would learn how to access page 0x80...
> > Uhm...
> >
> > MSDN claims:
> >
> >If the storage device is SCSI-compliant, the port driver attempts to
> >extract the serial number from the optional Unit Serial Number page
> >(page 0x80) of the VPD.
> >
> > Now I'm puzzled.
>
> A quick test with a Debian 12 VM in VirtualBox with many virtual
> controllers+drives shows the same problem:
> Entries in /dev/disk/by-id appear only for virtual disks behind emulated
> SATA and NVMe controllers, but not for SCSI and SAS controllers.
> A test with "smartctl -i ..." with SCSI/SAS devices doesn't print a serial
> number. In debug mode it prints "Vital Product Data (VPD) INQUIRY failed..."
> and other messages that suggest limited/buggy support of optional SCSI
> commands.
>
> If a Win11 PE (from install ISO) is run in same VM, the
> STORAGE_DEVICE_DESCRIPTOR only provides the serial number for SATA (NVMe
> drives not detected), but not for SCSI.
>
> Conclusion: The behavior of the current patch is compatible with Linux :-)
Ok, but with the DUID we have a workaround which makes it work even
better than on Linux, so it would begreat if we used it, unless we find
out where the UUID in "\GLOBAL??\Disk{} comes from...
Given the size of the STORAGE_DEVICE_UNIQUE_IDENTIFIER struct, we could
even contemplate a 128 bit hash, just to be on the safe side.
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
On Nov 3 17:27, Corinna Vinschen wrote: > On Nov 3 17:09, Christian Franke wrote: > > Unlike (S)ATA and NVMe, the serial number > > is not available for free in the device identify data block but requires an > > extra command (SCSI INQUIRY of VPD page 0x80). This might not be supported > > by the emulated controller or Windows does not use this command. > > AFAICS, only the data from STORAGE_DEVICE_ID_DESCRIPTOR is available > which is equivalent to the data from VPD page 0x83. As you can see, > it's part of the STORAGE_DEVICE_UNIQUE_IDENTIFIER data. The data > returned for the VirtIo device is the identifier string "\x01\x00", > which is a bit underwhelming. > > Would be great if we would learn how to access page 0x80... Uhm... MSDN claims: If the storage device is SCSI-compliant, the port driver attempts to extract the serial number from the optional Unit Serial Number page (page 0x80) of the VPD. Now I'm puzzled. Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
On Nov 3 17:09, Christian Franke wrote:
> Corinna Vinschen wrote:
> > I haven't found out where the UUID is coming from, yet, but based on the
> > description from
> > https://learn.microsoft.com/en-us/windows-hardware/drivers/storage/device-unique-identifiers--duids--for-storage-devices
> > I came up with this Q solution:
> >
> > === SNIP
> > diff --git a/winsup/cygwin/fhandler/dev_disk.cc
> > b/winsup/cygwin/fhandler/dev_disk.cc
> > index caca57d63216..74abfb8a3288 100644
> > --- a/winsup/cygwin/fhandler/dev_disk.cc
> > +++ b/winsup/cygwin/fhandler/dev_disk.cc
> > @@ -36,29 +36,51 @@ sanitize_id_string (char *s)
> > return i;
> > }
> > +typedef struct _STORAGE_DEVICE_UNIQUE_IDENTIFIER {
> > + ULONG Version;
> > + ULONG Size;
> > + ULONG StorageDeviceIdOffset;
> > + ULONG StorageDeviceOffset;
> > + ULONG DriveLayoutSignatureOffset;
> > +} STORAGE_DEVICE_UNIQUE_IDENTIFIER, *PSTORAGE_DEVICE_UNIQUE_IDENTIFIER;
> > +
> > +typedef struct _STORAGE_DEVICE_LAYOUT_SIGNATURE {
> > + ULONG Version;
> > + ULONG Size;
> > + BOOLEAN Mbr;
> > + union {
> > +ULONG MbrSignature;
> > +GUID GptDiskId;
> > + } DeviceSpecific;
> > +} STORAGE_DEVICE_LAYOUT_SIGNATURE, *PSTORAGE_DEVICE_LAYOUT_SIGNATURE;
> > +
>
> These are available in storduid.h
Yeah, and defining STORAGE_DEVICE_LAYOUT_SIGNATURE is useless,
but it was just for PoC.
> Thanks. Using this makes plenty of sense as a fallback if the serial number
> is unavailable. But if available, the serial number should be in the
> generated name as on Linux. This would provide a persistent name which
> reflects the actual device without a number invented by MS.
ACK
> The serial number is usually available with (S)ATA and NVMe (namespace uuid
> in the latter case). I'm not familiar with QEMU/KVM details. The fact that
> both 'vendor' and 'product' are returned on your system suggests that a
> SCSI/SAS controller is emulated.
Yes.
> Unlike (S)ATA and NVMe, the serial number
> is not available for free in the device identify data block but requires an
> extra command (SCSI INQUIRY of VPD page 0x80). This might not be supported
> by the emulated controller or Windows does not use this command.
AFAICS, only the data from STORAGE_DEVICE_ID_DESCRIPTOR is available
which is equivalent to the data from VPD page 0x83. As you can see,
it's part of the STORAGE_DEVICE_UNIQUE_IDENTIFIER data. The data
returned for the VirtIo device is the identifier string "\x01\x00",
which is a bit underwhelming.
Would be great if we would learn how to access page 0x80...
> IIRC the serial number is sometimes available via WMI even if missing in
> IOCTL_STORAGE_QUERY_PROPERTY:
>
> wmic diskdrive get manufacturer,model,serialnumber
Nope, it returns an empty string, just as the date from
STORAGE_DEVICE_DESCRIPTOR.
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
On Nov 3 12:10, Corinna Vinschen wrote:
> On Nov 3 11:09, Corinna Vinschen wrote:
> > On Nov 3 10:55, Corinna Vinschen wrote:
> > > On Oct 3 14:39, Christian Franke wrote:
> > > > According to NtQueryObject(., ObjectBasicInformation, ...), using
> > > > NtOpenFile(., MAXIMUM_ALLOWED, ...) without admin rights sets
> > > > GrantedAccess
> > > > to 0x001200a0
> > > > (FILE_EXECUTE|FILE_READ_ATTRIBUTES|READ_CONTROL|SYNCHRONIZE).
> > > > For some unknown reason, NVMe drives behind stornvme.sys additionally
> > > > require SYNCHRONIZE to use IOCTL_STORAGE_QUERY_PROPERTY. Possibly a
> > > > harmless
> > > > bug in the access check somewhere in the NVMe stack.
> > > >
> > > > The disk scanning from the first patch has been reworked based on code
> > > > borrowed from proc.cc:format_proc_partitions(). For the longer term, it
> > > > may
> > > > make sense to provide one flexible scanning function for /dev/sdXN,
> > > > /proc/partitions and /proc/disk/by-id.
> > >
> > > I applied your patch locally (patch looks pretty well, btw) but found
> > > that /dev/disk/by-id is empty, even when running with admin rights.
> > >
> > > I ran this on Windows 11 and Windows 2K19 in a QEMU/KVM VM. A
> > > \Device\Harddisk0\Partition0 symlink pointing to \Device\Harddisk0\DR0
> > > exists in both cases. I straced it, and found the following debug
> > > output:
> > >
> > > 1015 1155432 [main] ls 361 stordesc_to_id_name: Harddisk0\Partition0:
> > > 'Red_Hat' 'VirtIO' '' (ignored)
> > >
> > > Is that really desired?
>
> We could fake a serial number dependent on the path. See
> https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/mount.cc;hb=main#l253
>
> Alternatively, there's also a symlink in GLOBAL?? pointing
> to the same target as \Device\Harddisk0\Partition0, i. e.:
>
> \Device\Harddisk0\Partition0 -> \Device\Harddisk0\DR0
>
> and
>
> \GLOBAL??\Disk{4c622943-27e4-e81d-3fc7-c43fc9c7e126} ->
> \Device\Harddisk0\DR0
>
> We could use that UUID from that path, but that's quite a hassle
> to grab, because it requires to enumerate GLOBAL??.
>
> But then again, where does Windows get the UUID from? Something to
> find out...
I haven't found out where the UUID is coming from, yet, but based on the
description from
https://learn.microsoft.com/en-us/windows-hardware/drivers/storage/device-unique-identifiers--duids--for-storage-devices
I came up with this Q solution:
=== SNIP
diff --git a/winsup/cygwin/fhandler/dev_disk.cc
b/winsup/cygwin/fhandler/dev_disk.cc
index caca57d63216..74abfb8a3288 100644
--- a/winsup/cygwin/fhandler/dev_disk.cc
+++ b/winsup/cygwin/fhandler/dev_disk.cc
@@ -36,29 +36,51 @@ sanitize_id_string (char *s)
return i;
}
+typedef struct _STORAGE_DEVICE_UNIQUE_IDENTIFIER {
+ ULONG Version;
+ ULONG Size;
+ ULONG StorageDeviceIdOffset;
+ ULONG StorageDeviceOffset;
+ ULONG DriveLayoutSignatureOffset;
+} STORAGE_DEVICE_UNIQUE_IDENTIFIER, *PSTORAGE_DEVICE_UNIQUE_IDENTIFIER;
+
+typedef struct _STORAGE_DEVICE_LAYOUT_SIGNATURE {
+ ULONG Version;
+ ULONG Size;
+ BOOLEAN Mbr;
+ union {
+ULONG MbrSignature;
+GUID GptDiskId;
+ } DeviceSpecific;
+} STORAGE_DEVICE_LAYOUT_SIGNATURE, *PSTORAGE_DEVICE_LAYOUT_SIGNATURE;
+
/* Get ID string from STORAGE_DEVICE_DESCRIPTIOR. */
static bool
stordesc_to_id_name (const UNICODE_STRING *upath, char *ioctl_buf,
char (& name)[NAME_MAX + 1])
{
+ const STORAGE_DEVICE_UNIQUE_IDENTIFIER *id =
+reinterpret_cast(ioctl_buf);
+ char *desc_buf = ioctl_buf + id->StorageDeviceOffset;
const STORAGE_DEVICE_DESCRIPTOR *desc =
-reinterpret_cast(ioctl_buf);
+reinterpret_cast(desc_buf);
+
/* Ignore drive if information is missing, too short or too long. */
int vendor_len = 0, product_len = 0, serial_len = 0;
if (desc->VendorIdOffset)
-vendor_len = sanitize_id_string (ioctl_buf + desc->VendorIdOffset);
+vendor_len = sanitize_id_string (desc_buf + desc->VendorIdOffset);
if (desc->ProductIdOffset)
-product_len = sanitize_id_string (ioctl_buf + desc->ProductIdOffset);
+product_len = sanitize_id_string (desc_buf + desc->ProductIdOffset);
if (desc->SerialNumberOffset)
-serial_len = sanitize_id_string (ioctl_buf + desc->SerialNumberOffset);
+serial_len = sanitize_id_string (desc_buf + desc->SerialNumberOffset);
- bool valid = (4 <= vendor_len + product_len && 4 <= serial_len
- && (20 + vendor_len + 1 + product_len + 1 + serial_len + 10)
+ bool valid = (4 <= vendor_len + product
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
On Nov 3 12:06, Christian Franke wrote:
> Corinna Vinschen wrote:
> > > I ran this on Windows 11 and Windows 2K19 in a QEMU/KVM VM. A
> > > \Device\Harddisk0\Partition0 symlink pointing to \Device\Harddisk0\DR0
> > > exists in both cases. I straced it, and found the following debug
> > > output:
> > >
> > >1015 1155432 [main] ls 361 stordesc_to_id_name: Harddisk0\Partition0:
> > > 'Red_Hat' 'VirtIO' '' (ignored)
> > >
> > > Is that really desired?
>
> Yes - if IOCTL_STORAGE_QUERY_PROPERTY{... PropertyStandardQuery} does not
> return a serial number (''), the device is intentionally ignored.
See my other mail I just sent.
> > Thread 1 "ls" hit Breakpoint 2, stordesc_to_id_name (upath=0x7c500,
> > ioctl_buf=0x10e0720 "(", name=...)
> > at
> > /home/corinna/src/cygwin/vanilla/winsup/cygwin/fhandler/dev_disk.cc:44
> > 44const STORAGE_DEVICE_DESCRIPTOR *desc =
> > (gdb) n
> > 47int vendor_len = 0, product_len = 0, serial_len = 0;
> > (gdb)
> > 48if (desc->VendorIdOffset)
> > (gdb)
> > 49 vendor_len = sanitize_id_string (ioctl_buf +
> > desc->VendorIdOffset);
> > (gdb)
> > 50if (desc->ProductIdOffset)
> > (gdb)
> > 51 product_len = sanitize_id_string (ioctl_buf +
> > desc->ProductIdOffset);
> > (gdb)
> > 52if (desc->SerialNumberOffset)
> > (gdb)
> > 53 serial_len = sanitize_id_string (ioctl_buf +
> > desc->SerialNumberOffset);
>
> If possibly, please check whether (desc->SerialNumberOffset) is 0 or
> (ioctl_buf + desc->SerialNumberOffset) points to '\0' or a string of spaces.
> If no, there is possibly something wrong in sanitize_id_string().
desc->SerialNumberOffset is > 0, serial_len is 0, it points to an empty
string.
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
On Nov 3 11:09, Corinna Vinschen wrote:
> On Nov 3 10:55, Corinna Vinschen wrote:
> > On Oct 3 14:39, Christian Franke wrote:
> > > According to NtQueryObject(., ObjectBasicInformation, ...), using
> > > NtOpenFile(., MAXIMUM_ALLOWED, ...) without admin rights sets
> > > GrantedAccess
> > > to 0x001200a0
> > > (FILE_EXECUTE|FILE_READ_ATTRIBUTES|READ_CONTROL|SYNCHRONIZE).
> > > For some unknown reason, NVMe drives behind stornvme.sys additionally
> > > require SYNCHRONIZE to use IOCTL_STORAGE_QUERY_PROPERTY. Possibly a
> > > harmless
> > > bug in the access check somewhere in the NVMe stack.
> > >
> > > The disk scanning from the first patch has been reworked based on code
> > > borrowed from proc.cc:format_proc_partitions(). For the longer term, it
> > > may
> > > make sense to provide one flexible scanning function for /dev/sdXN,
> > > /proc/partitions and /proc/disk/by-id.
> >
> > I applied your patch locally (patch looks pretty well, btw) but found
> > that /dev/disk/by-id is empty, even when running with admin rights.
> >
> > I ran this on Windows 11 and Windows 2K19 in a QEMU/KVM VM. A
> > \Device\Harddisk0\Partition0 symlink pointing to \Device\Harddisk0\DR0
> > exists in both cases. I straced it, and found the following debug
> > output:
> >
> > 1015 1155432 [main] ls 361 stordesc_to_id_name: Harddisk0\Partition0:
> > 'Red_Hat' 'VirtIO' '' (ignored)
> >
> > Is that really desired?
We could fake a serial number dependent on the path. See
https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=winsup/cygwin/mount.cc;hb=main#l253
Alternatively, there's also a symlink in GLOBAL?? pointing
to the same target as \Device\Harddisk0\Partition0, i. e.:
\Device\Harddisk0\Partition0 -> \Device\Harddisk0\DR0
and
\GLOBAL??\Disk{4c622943-27e4-e81d-3fc7-c43fc9c7e126} -> \Device\Harddisk0\DR0
We could use that UUID from that path, but that's quite a hassle
to grab, because it requires to enumerate GLOBAL??.
But then again, where does Windows get the UUID from? Something to
find out...
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
On Nov 3 10:55, Corinna Vinschen wrote:
> Hi Christian,
>
> On Oct 3 14:39, Christian Franke wrote:
> > Christian Franke wrote:
> > > This is a first attempt to partly emulate the Linux directory
> > > /dev/disk/by-id. Useful to make sure the correct device is accessed in
> > > conjunction with dd, ddrescue, fdisk,
> >
> > Attached is the second attempt.
> >
> >
> > > The additional '*-partN' links to partitions are not yet included.
> >
> > These are now included.
> >
> >
> > > This only works properly if Win32 path '\\.\PhysicalDriveN' is always
> > > trivially mapped to NT path '\Device\HarddiskN\Partition0'.
> > > IOCTL_STORAGE_QUERY_PROPERTY with a handle from NtOpenFile(.,
> > > READ_CONTROL,...) instead of CreateFile(., 0, ...) did not work with all
> > > drivers. With stornvme.sys, it fails with permission denied. Perhaps
> > > other permission bits are required for NtOpenFile(). Thanks for any info
> > > regarding this.
> >
> > According to NtQueryObject(., ObjectBasicInformation, ...), using
> > NtOpenFile(., MAXIMUM_ALLOWED, ...) without admin rights sets GrantedAccess
> > to 0x001200a0 (FILE_EXECUTE|FILE_READ_ATTRIBUTES|READ_CONTROL|SYNCHRONIZE).
> > For some unknown reason, NVMe drives behind stornvme.sys additionally
> > require SYNCHRONIZE to use IOCTL_STORAGE_QUERY_PROPERTY. Possibly a harmless
> > bug in the access check somewhere in the NVMe stack.
> >
> > The disk scanning from the first patch has been reworked based on code
> > borrowed from proc.cc:format_proc_partitions(). For the longer term, it may
> > make sense to provide one flexible scanning function for /dev/sdXN,
> > /proc/partitions and /proc/disk/by-id.
>
> I applied your patch locally (patch looks pretty well, btw) but found
> that /dev/disk/by-id is empty, even when running with admin rights.
>
> I ran this on Windows 11 and Windows 2K19 in a QEMU/KVM VM. A
> \Device\Harddisk0\Partition0 symlink pointing to \Device\Harddisk0\DR0
> exists in both cases. I straced it, and found the following debug
> output:
>
> 1015 1155432 [main] ls 361 stordesc_to_id_name: Harddisk0\Partition0:
> 'Red_Hat' 'VirtIO' '' (ignored)
>
> Is that really desired?
Thread 1 "ls" hit Breakpoint 2, stordesc_to_id_name (upath=0x7c500,
ioctl_buf=0x10e0720 "(", name=...)
at /home/corinna/src/cygwin/vanilla/winsup/cygwin/fhandler/dev_disk.cc:44
44const STORAGE_DEVICE_DESCRIPTOR *desc =
(gdb) n
47int vendor_len = 0, product_len = 0, serial_len = 0;
(gdb)
48if (desc->VendorIdOffset)
(gdb)
49 vendor_len = sanitize_id_string (ioctl_buf + desc->VendorIdOffset);
(gdb)
50if (desc->ProductIdOffset)
(gdb)
51 product_len = sanitize_id_string (ioctl_buf +
desc->ProductIdOffset);
(gdb)
52if (desc->SerialNumberOffset)
(gdb)
53 serial_len = sanitize_id_string (ioctl_buf +
desc->SerialNumberOffset);
(gdb)
55bool valid = (4 <= vendor_len + product_len && 4 <= serial_len
(gdb) p vendor_len
$1 = 7
(gdb) p product_len
$2 = 6
(gdb) p serial_len
$3 = 0
(gdb) n
[New Thread 3944.0x1958]
56 && (20 + vendor_len + 1 + product_len + 1 + serial_len
+ 10)
(gdb) n
55bool valid = (4 <= vendor_len + product_len && 4 <= serial_len
(gdb)
55bool valid = (4 <= vendor_len + product_len && 4 <= serial_len
(gdb)
58debug_printf ("%S: '%s' '%s' '%s'%s", upath,
(gdb) p valid
$4 = false
HTH,
Corinna
Re: [PATCH] Cygwin: Add /dev/disk/by-id symlinks
Hi Christian, On Oct 3 14:39, Christian Franke wrote: > Christian Franke wrote: > > This is a first attempt to partly emulate the Linux directory > > /dev/disk/by-id. Useful to make sure the correct device is accessed in > > conjunction with dd, ddrescue, fdisk, > > Attached is the second attempt. > > > > The additional '*-partN' links to partitions are not yet included. > > These are now included. > > > > This only works properly if Win32 path '\\.\PhysicalDriveN' is always > > trivially mapped to NT path '\Device\HarddiskN\Partition0'. > > IOCTL_STORAGE_QUERY_PROPERTY with a handle from NtOpenFile(., > > READ_CONTROL,...) instead of CreateFile(., 0, ...) did not work with all > > drivers. With stornvme.sys, it fails with permission denied. Perhaps > > other permission bits are required for NtOpenFile(). Thanks for any info > > regarding this. > > According to NtQueryObject(., ObjectBasicInformation, ...), using > NtOpenFile(., MAXIMUM_ALLOWED, ...) without admin rights sets GrantedAccess > to 0x001200a0 (FILE_EXECUTE|FILE_READ_ATTRIBUTES|READ_CONTROL|SYNCHRONIZE). > For some unknown reason, NVMe drives behind stornvme.sys additionally > require SYNCHRONIZE to use IOCTL_STORAGE_QUERY_PROPERTY. Possibly a harmless > bug in the access check somewhere in the NVMe stack. > > The disk scanning from the first patch has been reworked based on code > borrowed from proc.cc:format_proc_partitions(). For the longer term, it may > make sense to provide one flexible scanning function for /dev/sdXN, > /proc/partitions and /proc/disk/by-id. I applied your patch locally (patch looks pretty well, btw) but found that /dev/disk/by-id is empty, even when running with admin rights. I ran this on Windows 11 and Windows 2K19 in a QEMU/KVM VM. A \Device\Harddisk0\Partition0 symlink pointing to \Device\Harddisk0\DR0 exists in both cases. I straced it, and found the following debug output: 1015 1155432 [main] ls 361 stordesc_to_id_name: Harddisk0\Partition0: 'Red_Hat' 'VirtIO' '' (ignored) Is that really desired? Thanks, Corinna
Re: [PATCH] Cygwin: Fix __cpuset_zero_s prototype
On Sep 7 22:36, Mark Geisert wrote: > Add a missing "void" to the prototype for __cpuset_zero_s(). > > Reported-by: Marco Mason > Addresses: https://cygwin.com/pipermail/cygwin/2023-September/254423.html > Signed-off-by: Mark Geisert > Fixes: c6cfc99648d6 (Cygwin: sys/cpuset.h: add cpuset-specific external > functions) > > --- > winsup/cygwin/include/sys/cpuset.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Pushed, including doc fix. Thanks, Corinna
Re: [PATCH] Add initial support for SOURCE_DATE_EPOCH
Jon,
you did all the latest work in terms of the build machinery.
Would you mind to review this patch, please?
Thanks,
Corinna
On Sep 5 19:01, Christian Franke wrote:
> This patch enables reproducible builds of cygwin package in conjunction with
> this cygport patch:
> https://sourceware.org/pipermail/cygwin-apps/2023-August/043108.html
>
> cygwin.cygport was enhanced for the test as described in the above post.
>
> If the same build path, SOURCE_DATE_EPOCH and toolchain are used, rebuilds
> with cygport produce identical distribution tarballs. Adding proper
> -fmacro-prefix-map gcc options (or remove all usages of __FILE__) could
> possibly make this independent from the build path.
>
> Note that 'u' (replace with newer objects only) flag needed to be removed
> from ar commands because it is incompatible with 'D' (deterministic
> archive). I don't expect any negative effect because existing .a files are
> always removed before ar is run.
>
> Not yet tested with different machines or different users accounts.
>
> Patch would be much simpler (mkvers.sh only) if binutils would support
> SOURCE_DATE_EPOCH directly.
>
> --
> Regards,
> Christian
>
> From b877330d53b95a88f1aef0fa3d14e97910d9dd2a Mon Sep 17 00:00:00 2001
> From: Christian Franke
> Date: Tue, 5 Sep 2023 18:32:49 +0200
> Subject: [PATCH] Add initial support for SOURCE_DATE_EPOCH
>
> If specified, set version timestamp to this value.
> Enable deterministic archives for ar and ranlib.
> Set cygwin1.dll PE and export table header timestamps
> to zero.
>
> Signed-off-by: Christian Franke
> ---
> winsup/cygwin/Makefile.am | 6 ++
> winsup/cygwin/scripts/mkimport | 6 +-
> winsup/cygwin/scripts/mkvers.sh | 4 ++--
> winsup/cygwin/scripts/speclib | 6 +-
> 4 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/winsup/cygwin/Makefile.am b/winsup/cygwin/Makefile.am
> index 9912b5399..64b252a22 100644
> --- a/winsup/cygwin/Makefile.am
> +++ b/winsup/cygwin/Makefile.am
> @@ -572,6 +572,10 @@ toollib_DATA = \
> libgmon_a_SOURCES = $(GMON_FILES)
> libgmon_a_LIBADD =
>
> +# Enable deterministic archives for reproducible builds.
> +ARFLAGS = cr$${SOURCE_DATE_EPOCH:+D}
> +override RANLIB := $(RANLIB)$${SOURCE_DATE_EPOCH:+ -D}
> +
> # cygserver library
> cygserver_blddir = ${target_builddir}/winsup/cygserver
> LIBSERVER = $(cygserver_blddir)/libcygserver.a
> @@ -589,12 +593,14 @@ $(LDSCRIPT): $(LDSCRIPT).in
> $(AM_V_GEN)$(CC) -E - -P < $^ -o $@
>
> # cygwin dll
> +# Set PE and export table header timestamps to zero for reproducible builds.
> $(NEW_DLL_NAME): $(LDSCRIPT) libdll.a $(VERSION_OFILES) $(LIBSERVER)\
> $(newlib_build)/libm.a $(newlib_build)/libc.a
> $(AM_V_CXXLD)$(CXX) $(CXXFLAGS) \
> -mno-use-libstdc-wrappers \
> -Wl,--gc-sections -nostdlib -Wl,-T$(LDSCRIPT) \
> -Wl,--dynamicbase -static \
> + $${SOURCE_DATE_EPOCH:+-Wl,--no-insert-timestamp} \
> -Wl,--heap=0 -Wl,--out-implib,cygdll.a -shared -o $@ \
> -e @DLL_ENTRY@ $(DEF_FILE) \
> -Wl,-whole-archive libdll.a -Wl,-no-whole-archive \
> diff --git a/winsup/cygwin/scripts/mkimport b/winsup/cygwin/scripts/mkimport
> index 7684a8f0e..9517c4e9e 100755
> --- a/winsup/cygwin/scripts/mkimport
> +++ b/winsup/cygwin/scripts/mkimport
> @@ -92,8 +92,12 @@ for my $f (keys %text) {
> }
> }
>
> +# Enable deterministic archives for reproducible builds.
> +my $opts = 'crs';
> +$opts .= 'D' if ($ENV{'SOURCE_DATE_EPOCH'} != '');
> +
> unlink $libdll;
> -system $ar, 'crus', $libdll, glob('*.o'), @ARGV;
> +system $ar, $opts, $libdll, glob('*.o'), @ARGV;
> unlink glob('*.o');
> exit 1 if $?;
>
> diff --git a/winsup/cygwin/scripts/mkvers.sh b/winsup/cygwin/scripts/mkvers.sh
> index 96af936ec..38f439cd0 100755
> --- a/winsup/cygwin/scripts/mkvers.sh
> +++ b/winsup/cygwin/scripts/mkvers.sh
> @@ -56,9 +56,9 @@ parse_preproc_flags $CC
>
>
> #
> -# Load the current date so we can work on individual fields
> +# Load the current date (or SOURCE_DATE_EPOCH) so we can work on individual
> fields
> #
> -set -$- $(date -u +"%m %d %Y %H:%M")
> +set -$- $(date ${SOURCE_DATE_EPOCH:+-d @}${SOURCE_DATE_EPOCH} -u +"%m %d %Y
> %H:%M")
> m=$1 d=$2 y=$3 hhmm=$4
> #
> # Set date into -MM-DD HH:MM:SS format
> diff --git a/winsup/cygwin/scripts/speclib b/winsup/cygwin/scripts/speclib
> index e6d4d8e94..41a3a8e13 100755
> --- a/winsup/cygwin/scripts/speclib
> +++ b/winsup/cygwin/scripts/speclib
> @@ -74,7 +74,11 @@ EOF
> close $as_fd or exit 1;
> system $objcopy, '-j', '.idata$7', $iname_o;
>
> -$res = system $ar, 'crus', $lib, sort keys %extract;
> +# Enable deterministic archives for reproducible builds.
> +my $opts = 'crs';
> +$opts .= 'D' if ($ENV{'SOURCE_DATE_EPOCH'} != '');
> +
> +$res = system $ar, $opts, $lib, sort keys %extract;
> unlink keys %extract;
> die "$0: ar creation of $lib exited with non-zero status\n" if $res;
> exit 0;
> --
> 2.39.0
>
Re: [PATCH] Cygwin: Fix __cpuset_zero_s prototype
On Sep 7 22:36, Mark Geisert wrote: > Add a missing "void" to the prototype for __cpuset_zero_s(). > > Reported-by: Marco Mason > Addresses: https://cygwin.com/pipermail/cygwin/2023-September/254423.html > Signed-off-by: Mark Geisert > Fixes: c6cfc99648d6 (Cygwin: sys/cpuset.h: add cpuset-specific external > functions) Thanks, can you also add an entry to the 3.4.10 release file (which doesn't exist yet), please? Corinna
Re: [PATCH v2] Cygwin: cpuinfo: Linux 6.5 additions
On Sep 1 11:42, Brian Inglis wrote: > add AMD 0x801f EAX 14 debug_swap SEV-ES full debug state swap > > Signed-off-by: Brian Inglis > --- > winsup/cygwin/fhandler/proc.cc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/winsup/cygwin/fhandler/proc.cc b/winsup/cygwin/fhandler/proc.cc > index cbc49a12a417..be107cb8eacc 100644 > --- a/winsup/cygwin/fhandler/proc.cc > +++ b/winsup/cygwin/fhandler/proc.cc > @@ -1652,7 +1652,7 @@ format_proc_cpuinfo (void *, char *) > /* ftcprint (features2, 11, "sev_64b");*//* SEV 64 bit host guest only */ > /* ftcprint (features2, 12, "sev_rest_inj"); *//* SEV restricted > injection */ > /* ftcprint (features2, 13, "sev_alt_inj");*//* SEV alternate > injection */ > -/* ftcprint (features2, 14, "sev_es_dbg_swap");*//* SEV-ES debug state > swap */ > + ftcprint (features2, 14, "debug_swap"); /* SEV-ES full debug state > swap */ > /* ftcprint (features2, 15, "no_host_ibs");*//* host IBS unsupported > */ > /* ftcprint (features2, 16, "vte");*//* virtual transparent > encryption */ > } > -- > 2.39.0 Pushed. Thanks, Corinna
Re: [PATCH] Cygwin: Implement sound mixer device.
On Sep 1 19:04, Takashi Yano wrote: > This patch adds implementation of OSS-based sound mixer device. This > allows applications to change the sound playing volume. Cool! Go ahead, that's a nice addition. > NOTE: Currently, the recording volume cannot be changed. I guess you're already hacking on adding that :) Thanks, Corinna
Re: [PATCH] Cygwin: cpuinfo: Linux 6.5: add AMD 0x8000001f EAX 14 debug_swap SEV-ES full debug state swap
Hi Brian, nothing against the patch as such, but your subject line is not so nice. As it becomes the commit message first line, it should be shorter. Add more descriptive text instead, please, and make sure that it tells the reader what you're really doing, i. e.: You write "add ", but your patch is actually exchanging one with another . The reader of the commit message would probably like to know why you're doing that. Partially copying the original Linux kernel commit message should be ok. Also, given that changes a string, does it qualify for a "Fixes:" tag? Thanks, Corinna On Aug 30 22:10, Brian Inglis wrote: > Signed-off-by: Brian Inglis > --- > winsup/cygwin/fhandler/proc.cc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/winsup/cygwin/fhandler/proc.cc b/winsup/cygwin/fhandler/proc.cc > index cbc49a12a417..be107cb8eacc 100644 > --- a/winsup/cygwin/fhandler/proc.cc > +++ b/winsup/cygwin/fhandler/proc.cc > @@ -1652,7 +1652,7 @@ format_proc_cpuinfo (void *, char *) > /* ftcprint (features2, 11, "sev_64b");*//* SEV 64 bit host guest only */ > /* ftcprint (features2, 12, "sev_rest_inj"); *//* SEV restricted > injection */ > /* ftcprint (features2, 13, "sev_alt_inj");*//* SEV alternate > injection */ > -/* ftcprint (features2, 14, "sev_es_dbg_swap");*//* SEV-ES debug state > swap */ > + ftcprint (features2, 14, "debug_swap"); /* SEV-ES full debug state > swap */ > /* ftcprint (features2, 15, "no_host_ibs");*//* host IBS unsupported > */ > /* ftcprint (features2, 16, "vte");*//* virtual transparent > encryption */ > } > -- > 2.39.0
[PATCH] Cygwin: execve: drop argument size limit
From: Corinna Vinschen
Before commit 44f73c5a6206 ("Cygwin: Fix segfalt when too many command
line args are specified.") we had no actual argument size limit, except
for the fact that the child process created another copy of the argv
array on the stack, which could result in a stack overflow and a
subsequent SEGV. Commit 44f73c5a6206 changed that by allocating the
additional argv array via malloc, and it introduced a new SC_ARG_MAX
limit along the lines of the typical Linux limit.
However, this new limit is artificial. Cygwin allocates all argument
and environment data on the cygheap. We only run out of ARG_MAX space
if we're out of memory resources.
Change argument size handling accordingly:
- Drop the args size check from child_info_spawn::worker.
- Return -1 from sysconf (SC_ARG_MAX), i. e., the argument size limit
is undefined.
- Change argv handling in class av, so that a failing cmalloc is not
fatal. This allows the parent process to return E2BIG if it's out
of cygheap resources.
- In the child, add a check around the new malloc call, so that it
doesn't result in a SEGV if the child process gets unexpectedly into
an ENOMEM situation at this point. In this (unlikely) case, proceed
with the original __argv array instead. Add comment to explain why.
Fixes: 44f73c5a6206 ("Cygwin: Fix segfalt when too many command line args are
specified.")
Signed-off-by: Corinna Vinschen
---
winsup/cygwin/dcrt0.cc| 18 +++---
winsup/cygwin/kernel32.cc | 7 +--
winsup/cygwin/local_includes/winf.h | 13 +
winsup/cygwin/local_includes/winsup.h | 4
winsup/cygwin/spawn.cc| 18 --
winsup/cygwin/sysconf.cc | 2 +-
6 files changed, 38 insertions(+), 24 deletions(-)
diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
index 1d8810546314..130d652aac6e 100644
--- a/winsup/cygwin/dcrt0.cc
+++ b/winsup/cygwin/dcrt0.cc
@@ -976,10 +976,22 @@ dll_crt0_1 (void *)
{
/* Create a copy of Cygwin's version of __argv so that, if the user makes
a change to an element of argv[] it does not affect Cygwin's argv.
-Changing the the contents of what argv[n] points to will still
-affect Cygwin. This is similar (but not exactly like) Linux. */
+Changing the contents of what argv[n] points to will still affect
+Cygwin. This is similar (but not exactly like) Linux.
+
+We used to allocate newargv on the stack, but all the rest of the
+argument and environment handling does not depend on the stack,
+as it does on Linux. In fact, everything is stored by the parent
+in the cygheap, so the only reason this may fail is if we're out
+of resources in a big way. If this malloc fails, we could either
+fail the entire process execution, or we could proceed with the
+original argv and potentially affect output of /proc/self/cmdline.
+We opt for the latter here because it's the lesser evil. */
char **newargv = (char **) malloc ((__argc + 1) * sizeof (char *));
- memcpy (newargv, __argv, (__argc + 1) * sizeof (char *));
+ if (newargv)
+ memcpy (newargv, __argv, (__argc + 1) * sizeof (char *));
+ else
+ newargv = __argv;
/* Handle any signals which may have arrived */
sig_dispatch_pending (false);
_my_tls.call_signal_handler ();
diff --git a/winsup/cygwin/kernel32.cc b/winsup/cygwin/kernel32.cc
index 6248aefd5183..36951f6a87be 100644
--- a/winsup/cygwin/kernel32.cc
+++ b/winsup/cygwin/kernel32.cc
@@ -424,8 +424,11 @@ ucmd ()
linebuf cmd;
path_conv real_path (__argv[0]);
av newargv (__argc, __argv);
- cmd.fromargv (newargv, real_path.get_win32 (), true);
- RtlInitUnicodeString (, cmd);
+ if (newargv.argc)
+ {
+ cmd.fromargv (newargv, real_path.get_win32 (), true);
+ RtlInitUnicodeString (, cmd);
+ }
}
return
}
diff --git a/winsup/cygwin/local_includes/winf.h
b/winsup/cygwin/local_includes/winf.h
index 651f78ba2824..b58693441095 100644
--- a/winsup/cygwin/local_includes/winf.h
+++ b/winsup/cygwin/local_includes/winf.h
@@ -23,11 +23,16 @@ class av
public:
int argc;
bool win16_exe;
- av (): argv (NULL) {}
- av (int ac_in, const char * const *av_in) : calloced (0), argc (ac_in),
win16_exe (false)
+ av () : argv (NULL), argc (0) {}
+ av (int ac_in, const char * const *av_in)
+ : calloced (0), win16_exe (false)
{
-argv = (char **) cmalloc_abort (HEAP_1_ARGV, (argc + 5) * sizeof (char *));
-memcpy (argv, av_in, (argc + 1) * sizeof (char *));
+argv = (char **) cmalloc (HEAP_1_ARGV, (ac_in + 5) * sizeof (char *));
+if (argv)
+ {
+ argc = ac_in;
+ memcpy (argv, av_in, (argc + 1) * sizeof (char *));
+ }
}
void *operator new (size_t, void *p) __attribute__ ((nothrow)) {return p;}
~av ()
diff -
Re: [PATCH v2] Cygwin: spawn: Fix segfalt when too many command line args are specified.
On Aug 28 13:09, Corinna Vinschen wrote: > On Aug 28 12:57, Corinna Vinschen wrote: > > On Aug 28 18:46, Takashi Yano wrote: > > > Previously, the number of command line args was not checked for > > > cygwin process. Due to this, segmentation fault was caused if too > > > many command line args are specified. > > > https://cygwin.com/pipermail/cygwin/2023-August/254333.html > > > > > > Since char *argv[argc + 1] is placed on the stack in dll_crt0_1(), > > > STATUS_STACK_OVERFLOW occurs if the stack does not have enough > > > space. > > > > > > With this patch, the total length of the arguments and the size of > > > argv[] is restricted to 1/4 of total stack size for the process, and > > > spawnve() returns E2BIG if the size exceeds the limit. > > > [...] > > I tried this simple patch: > > > > diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc > > index 49b7a44aeb15..961dea4ab993 100644 > > --- a/winsup/cygwin/dcrt0.cc > > +++ b/winsup/cygwin/dcrt0.cc > > @@ -978,11 +978,8 @@ dll_crt0_1 (void *) > > a change to an element of argv[] it does not affect Cygwin's argv. > > Changing the the contents of what argv[n] points to will still > > affect Cygwin. This is similar (but not exactly like) Linux. */ > > - char *newargv[__argc + 1]; > > - char **nav = newargv; > > - char **oav = __argv; > > - while ((*nav++ = *oav++) != NULL) > > - continue; > > + char **newargv = (char **) malloc ((__argc + 1) * sizeof (char **)); > > + memcpy (newargv, __argv, (__argc + 1) * sizeof (char **)); > >/* Handle any signals which may have arrived */ > >sig_dispatch_pending (false); > >_my_tls.call_signal_handler (); > > > > and the testcase `LC_ALL=C sed 's/x/y/' $(seq 100)' simply ran > > as desired. Combined with a bit of error checking... > > We may also get away with storing it in the Windows heap, but I didn't > test this . No, we don't. The Windows heap would not be inherited by a forked child and argv would be lost. Sounds not great. Corinna
Re: [PATCH v2] Cygwin: spawn: Fix segfalt when too many command line args are specified.
On Aug 28 12:57, Corinna Vinschen wrote: > On Aug 28 18:46, Takashi Yano wrote: > > Previously, the number of command line args was not checked for > > cygwin process. Due to this, segmentation fault was caused if too > > many command line args are specified. > > https://cygwin.com/pipermail/cygwin/2023-August/254333.html > > > > Since char *argv[argc + 1] is placed on the stack in dll_crt0_1(), > > STATUS_STACK_OVERFLOW occurs if the stack does not have enough > > space. > > > > With this patch, the total length of the arguments and the size of > > argv[] is restricted to 1/4 of total stack size for the process, and > > spawnve() returns E2BIG if the size exceeds the limit. > > [...] > I tried this simple patch: > > diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc > index 49b7a44aeb15..961dea4ab993 100644 > --- a/winsup/cygwin/dcrt0.cc > +++ b/winsup/cygwin/dcrt0.cc > @@ -978,11 +978,8 @@ dll_crt0_1 (void *) >a change to an element of argv[] it does not affect Cygwin's argv. >Changing the the contents of what argv[n] points to will still >affect Cygwin. This is similar (but not exactly like) Linux. */ > - char *newargv[__argc + 1]; > - char **nav = newargv; > - char **oav = __argv; > - while ((*nav++ = *oav++) != NULL) > - continue; > + char **newargv = (char **) malloc ((__argc + 1) * sizeof (char **)); > + memcpy (newargv, __argv, (__argc + 1) * sizeof (char **)); >/* Handle any signals which may have arrived */ >sig_dispatch_pending (false); >_my_tls.call_signal_handler (); > > and the testcase `LC_ALL=C sed 's/x/y/' $(seq 100)' simply ran > as desired. Combined with a bit of error checking... We may also get away with storing it in the Windows heap, but I didn't test this . > Along these lines, there's no reason to couple SC_ARG_MAX to the > size of the stack. I'd propose to return the value 2097152. It's > the default value returned by getconf ARG_MAX on LInx as well. Oh, and we can carefully check in child_info_spawn::worker that the args are not taking more space than the value returned by SC_ARG_MAX and return E2BIG if so. We do this when checking the args for non-Cygwin apps anyway. Thanks, Corinna
Re: [PATCH] Cygwin: termios: Refactor the function is_console_app().
On Aug 28 18:21, Takashi Yano wrote:
> Signed-off-by: Takashi Yano
> ---
> winsup/cygwin/fhandler/termios.cc | 18 --
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler/termios.cc
> b/winsup/cygwin/fhandler/termios.cc
> index 789ae0179..d106955dc 100644
> --- a/winsup/cygwin/fhandler/termios.cc
> +++ b/winsup/cygwin/fhandler/termios.cc
> @@ -704,22 +704,20 @@ static bool
> is_console_app (const WCHAR *filename)
> {
>HANDLE h;
> - const int id_offset = 92;
>h = CreateFileW (filename, GENERIC_READ, FILE_SHARE_READ,
> NULL, OPEN_EXISTING, 0, NULL);
>char buf[1024];
>DWORD n;
>ReadFile (h, buf, sizeof (buf), , 0);
>CloseHandle (h);
> - char *p = (char *) memmem (buf, n, "PE\0\0", 4);
> - if (p && p + id_offset < buf + n)
> -return p[id_offset] == '\003'; /* 02: GUI, 03: console */
> - else
> -{
> - wchar_t *e = wcsrchr (filename, L'.');
> - if (e && (wcscasecmp (e, L".bat") == 0 || wcscasecmp (e, L".cmd") ==
> 0))
> - return true;
> -}
> + /* The offset of Subsystem is the same for both IMAGE_NT_HEADERS32 and
> + IMAGE_NT_HEADERS64, so only IMAGE_NT_HEADERS32 is used here. */
> + IMAGE_NT_HEADERS32 *p = (IMAGE_NT_HEADERS32 *) memmem (buf, n, "PE\0\0",
> 4);
Please use PIMAGE_NT_HEADERS instead and just drop the comment.
We don't support 32 bit anyway.
Thanks,
Corinna
Re: [PATCH v2] Cygwin: spawn: Fix segfalt when too many command line args are specified.
On Aug 28 18:46, Takashi Yano wrote:
> Previously, the number of command line args was not checked for
> cygwin process. Due to this, segmentation fault was caused if too
> many command line args are specified.
> https://cygwin.com/pipermail/cygwin/2023-August/254333.html
>
> Since char *argv[argc + 1] is placed on the stack in dll_crt0_1(),
> STATUS_STACK_OVERFLOW occurs if the stack does not have enough
> space.
>
> With this patch, the total length of the arguments and the size of
> argv[] is restricted to 1/4 of total stack size for the process, and
> spawnve() returns E2BIG if the size exceeds the limit.
> [...]
> +static size_t
> +get_stack_size (const WCHAR *filename)
> +{
> + HANDLE h;
> + h = CreateFileW (filename, GENERIC_READ, FILE_SHARE_READ,
> +NULL, OPEN_EXISTING, 0, NULL);
> + char buf[1024];
> + DWORD n;
> + ReadFile (h, buf, sizeof (buf), , 0);
> + CloseHandle (h);
> + IMAGE_NT_HEADERS32 *p = (IMAGE_NT_HEADERS32 *) memmem (buf, n, "PE\0\0",
> 4);
> + if (!p)
> +return 0;
> + if ((char *) >OptionalHeader.SizeOfStackCommit > buf + n)
> +return 0; /* buf[] is not enough */
> + if (p->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC)
> +return p->OptionalHeader.SizeOfStackReserve;
> + IMAGE_NT_HEADERS64 *p64 = (IMAGE_NT_HEADERS64 *) p;
> + if ((char *) >OptionalHeader.SizeOfStackCommit > buf + n)
> +return 0; /* buf[] is not enough */
> + return p64->OptionalHeader.SizeOfStackReserve;
> +}
Sorry, but this proposal is too complicated, IMHO.
Checking the stacksize in the file header for each single execve
is quite a bit time consuming, isn't it?
The question is rather, why storing argv on the stack at all? I guess
the original idea was that argv is always a rather overseeable number.
But it doesn't have to stay on the stack.
I tried this simple patch:
diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
index 49b7a44aeb15..961dea4ab993 100644
--- a/winsup/cygwin/dcrt0.cc
+++ b/winsup/cygwin/dcrt0.cc
@@ -978,11 +978,8 @@ dll_crt0_1 (void *)
a change to an element of argv[] it does not affect Cygwin's argv.
Changing the the contents of what argv[n] points to will still
affect Cygwin. This is similar (but not exactly like) Linux. */
- char *newargv[__argc + 1];
- char **nav = newargv;
- char **oav = __argv;
- while ((*nav++ = *oav++) != NULL)
- continue;
+ char **newargv = (char **) malloc ((__argc + 1) * sizeof (char **));
+ memcpy (newargv, __argv, (__argc + 1) * sizeof (char **));
/* Handle any signals which may have arrived */
sig_dispatch_pending (false);
_my_tls.call_signal_handler ();
and the testcase `LC_ALL=C sed 's/x/y/' $(seq 100)' simply ran
as desired. Combined with a bit of error checking...
> diff --git a/winsup/cygwin/sysconf.cc b/winsup/cygwin/sysconf.cc
> index 2db92e4de..6cb2aecd0 100644
> --- a/winsup/cygwin/sysconf.cc
> +++ b/winsup/cygwin/sysconf.cc
> @@ -21,6 +21,13 @@ details. */
> #include "cpuid.h"
> #include "clock.h"
>
> +#define DEFAULT_STACKGUARD (wincap.def_guard_page_size() + wincap.page_size
> ())
> +static long
> +get_arg_max (int in)
> +{
> + return (long) (get_rlimit_stack () + DEFAULT_STACKGUARD) / 4;
> +}
> +
> static long
> get_page_size (int in)
> {
> @@ -485,7 +492,7 @@ static struct
> };
> } sca[] =
> {
> - {cons, {c:ARG_MAX}}, /* 0, _SC_ARG_MAX */
> + {func, {f:get_arg_max}}, /* 0, _SC_ARG_MAX */
>{cons, {c:CHILD_MAX}}, /* 1, _SC_CHILD_MAX */
>{cons, {c:CLOCKS_PER_SEC}},/* 2, _SC_CLK_TCK */
>{cons, {c:NGROUPS_MAX}}, /* 3, _SC_NGROUPS_MAX */
> --
> 2.39.0
Along these lines, there's no reason to couple SC_ARG_MAX to the
size of the stack. I'd propose to return the value 2097152. It's
the default value returned by getconf ARG_MAX on LInx as well.
Corinna
Re: [PATCH] Cygwin: pty: Fix failure to clear switch_to_nat_pipe flag.
On Aug 21 17:53, Takashi Yano wrote: > Hi Corinna, > > On Mon, 21 Aug 2023 10:20:30 +0200 > Corinna Vinschen wrote: > > Hi Takashi, > > > > On Aug 19 15:07, Takashi Yano wrote: > > > After the commit fbfea31dd9b9, switch_to_nat_pipe is not cleared > > > properly when non-cygwin app is terminated in the case where the > > > pseudo console is disabled. This is because get_winpid_to_hand_over() > > > sometimes returns PID of cygwin process even though it should return > > > only PID of non-cygwin process. This patch fixes the issue by adding > > > a new argument which requests only PID of non-cygwin process to > > > get_console_process_id(). > > > > How critical is that? Do we need a 3.4.9 asap, or can we wait and > > collect a few more bugfixes first? > > This problem is affected only when pseudo console is not > activated. So, most of Win10 users do not have this issue. > However, Win7/8 users may notice some small gritch after > non-cygwin app is executed. > > BTW, are you noticed that dumper.exe is missing in 3.4.8? No, I didn't, thanks for notifying me. > When you release new cygwin to fix that, I would be happy > if above patch will applied as well. Yeah, it will certainly be a 3.4.9 soon :} Corinna
Re: [PATCH] Cygwin: pty: Fix failure to clear switch_to_nat_pipe flag.
Hi Takashi, On Aug 19 15:07, Takashi Yano wrote: > After the commit fbfea31dd9b9, switch_to_nat_pipe is not cleared > properly when non-cygwin app is terminated in the case where the > pseudo console is disabled. This is because get_winpid_to_hand_over() > sometimes returns PID of cygwin process even though it should return > only PID of non-cygwin process. This patch fixes the issue by adding > a new argument which requests only PID of non-cygwin process to > get_console_process_id(). How critical is that? Do we need a 3.4.9 asap, or can we wait and collect a few more bugfixes first? Thanks, Corinna
Re: [PATCH] Cygwin: shared: Fix access permissions setting in open_shared().
On Aug 16 09:51, Corinna Vinschen wrote:
> On Aug 16 08:37, Takashi Yano wrote:
> > After the commit 93508e5bb841, the access permissions argument passed
> > to open_shared() is ignored and always replaced with (FILE_MAP_READ |
> > FILE_MAP_WRITE). This causes the weird behaviour that sshd service
> > process loses its cygwin PID. This triggers the failure in pty that
> > transfer_input() does not work properly.
> >
> > This patch resumes the access permission settings to fix that.
> >
> > Fixes: 93508e5bb841 ("Cygwin: open_shared: don't reuse shared_locations
> > parameter as output")
> > Signedd-off-by: Takashi Yano
> > ---
> > winsup/cygwin/mm/shared.cc | 6 ++
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/winsup/cygwin/mm/shared.cc b/winsup/cygwin/mm/shared.cc
> > index 40cdd4722..7977df382 100644
> > --- a/winsup/cygwin/mm/shared.cc
> > +++ b/winsup/cygwin/mm/shared.cc
> > @@ -139,8 +139,7 @@ open_shared (const WCHAR *name, int n, HANDLE&
> > shared_h, DWORD size,
> >if (name)
> > mapname = shared_name (map_buf, name, n);
> >if (m == SH_JUSTOPEN)
> > - shared_h = OpenFileMappingW (FILE_MAP_READ | FILE_MAP_WRITE, FALSE,
> > -mapname);
> > + shared_h = OpenFileMappingW (access, FALSE, mapname);
> >else
> > {
> > created = true;
> > @@ -165,8 +164,7 @@ open_shared (const WCHAR *name, int n, HANDLE&
> > shared_h, DWORD size,
> >do
> > {
> >addr = (void *) next_address;
> > - shared = MapViewOfFileEx (shared_h, FILE_MAP_READ | FILE_MAP_WRITE,
> > - 0, 0, 0, addr);
> > + shared = MapViewOfFileEx (shared_h, access, 0, 0, 0, addr);
> >next_address += wincap.allocation_granularity ();
> >if (next_address >= SHARED_REGIONS_ADDRESS_HIGH)
> > {
> > --
> > 2.39.0
>
> Oh drat, whatever was I thinking at the time? Thanks for catching
> and fixing this! Please push.
Please also add a release message to 3.4.8. I'll create the release
this week.
Thanks,
Corinna
Re: [PATCH] Cygwin: shared: Fix access permissions setting in open_shared().
On Aug 16 08:37, Takashi Yano wrote:
> After the commit 93508e5bb841, the access permissions argument passed
> to open_shared() is ignored and always replaced with (FILE_MAP_READ |
> FILE_MAP_WRITE). This causes the weird behaviour that sshd service
> process loses its cygwin PID. This triggers the failure in pty that
> transfer_input() does not work properly.
>
> This patch resumes the access permission settings to fix that.
>
> Fixes: 93508e5bb841 ("Cygwin: open_shared: don't reuse shared_locations
> parameter as output")
> Signedd-off-by: Takashi Yano
> ---
> winsup/cygwin/mm/shared.cc | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/winsup/cygwin/mm/shared.cc b/winsup/cygwin/mm/shared.cc
> index 40cdd4722..7977df382 100644
> --- a/winsup/cygwin/mm/shared.cc
> +++ b/winsup/cygwin/mm/shared.cc
> @@ -139,8 +139,7 @@ open_shared (const WCHAR *name, int n, HANDLE& shared_h,
> DWORD size,
>if (name)
> mapname = shared_name (map_buf, name, n);
>if (m == SH_JUSTOPEN)
> - shared_h = OpenFileMappingW (FILE_MAP_READ | FILE_MAP_WRITE, FALSE,
> - mapname);
> + shared_h = OpenFileMappingW (access, FALSE, mapname);
>else
> {
> created = true;
> @@ -165,8 +164,7 @@ open_shared (const WCHAR *name, int n, HANDLE& shared_h,
> DWORD size,
>do
> {
>addr = (void *) next_address;
> - shared = MapViewOfFileEx (shared_h, FILE_MAP_READ | FILE_MAP_WRITE,
> - 0, 0, 0, addr);
> + shared = MapViewOfFileEx (shared_h, access, 0, 0, 0, addr);
>next_address += wincap.allocation_granularity ();
>if (next_address >= SHARED_REGIONS_ADDRESS_HIGH)
> {
> --
> 2.39.0
Oh drat, whatever was I thinking at the time? Thanks for catching
and fixing this! Please push.
Corinna
Re: [PATCH 1/2] Detect RAM disks as a separate filesystem type
Hi Les,
your 2nd patch looks good to me, but this patch is a bit questionable.
On Aug 7 03:13, Les De Ridder wrote:
> diff --git a/winsup/cygwin/mount.cc b/winsup/cygwin/mount.cc
> index 36ab042a7..1950dadb0 100644
> --- a/winsup/cygwin/mount.cc
> +++ b/winsup/cygwin/mount.cc
> @@ -292,6 +292,17 @@ fs_info::update (PUNICODE_STRING upath, HANDLE in_vol)
>if (!NT_SUCCESS (status))
> ffdi.DeviceType = ffdi.Characteristics = 0;
>
> + if (upath->Buffer[5] == L':' && upath->Buffer[6] == L'\\')
If that's testing for a local drive path, it's probably wrong. The
NtQueryVolumeInformationFile(FileFsDeviceInformation) call returns the
FILE_REMOTE_DEVICE flag and sets is_remote_drive() in line 298. You
should use that flag.
This also means your code is called much too early in fs_info::update.
The preceeding code for file systems not providing valid serial numbers
is an exception from the rule because we need the serial number for
caching. Generically checking for another local filesystem should be
performed after the "if (is_remote_drive ())" expression, so starting
somewhere below line 495.
> + {
> + WCHAR dos[3] = {upath->Buffer[4], upath->Buffer[5], L'\0'};
> + WCHAR dev[MAX_PATH];
> + if (QueryDosDeviceW (dos, dev, MAX_PATH))
> + {
> + is_ramdisk (wcsncmp (dev, L"\\Device\\Ramdisk", 15));
> + has_buggy_reopen (is_ramdisk ());
> + }
> + }
> +
This gives me headaches. Did you check *all* the information returned
by the various NtQueryVolumeInformationFile calls? I. e., what is
returned by all these calls? What is the FS name set to? Which flags
are set in FileSystemAttributes? What is DeviceType and Characterisitics
set to?
We should really check all info already available from the
NtQueryVolumeInformationFile calls first, and please paste here the
information you get from these calls.
Also, even if all else fails, rather than calling QueryDosDeviceW we
should use NtQueryVolumeInformationFile(FileFsDriverPathInformation)
instead.
Thanks,
Corinna
Re: [PATCH 0/4] Testsuite update
On Aug 4 13:47, Jon Turney wrote: > This gets us down to no permanent failures in the testsuite in CI. > > When run locally, msgtest, semtest and shmtest fail because they need a > running cygserver using the test DLL, > and I haven't got a good idea about how to automate that. devdsp fails due > to a hang in child while exiting. > > Jon Turney (4): > Cygwin: testsuite: Add '-notimeout' option to cygrun > Cygwin: testsuite: Update README > Cygwin: testsuite: Fix cygload test > Cygwin: CI: XFAIL umask03 > > winsup/testsuite/Makefile.am | 6 ++- > winsup/testsuite/README | 82 +--- > winsup/testsuite/cygrun.c| 26 ++-- > winsup/testsuite/cygrun.sh | 2 +- > 4 files changed, 86 insertions(+), 30 deletions(-) > > -- > 2.39.0 :+1: Corinna
Re: [PATCH 5/5] Cygwin: add AT_EMPTY_PATH fix to release message
On Jul 28 17:11, Pedro Alves wrote: > On 2023-07-12 13:08, Corinna Vinschen wrote: > > +- Fix AT_EMPTY_PATH handling in fchmodat and fstatat if dirfd referres to > > referres -> refers Oops, sorry, too late :}
Re: [PATCH 0/5] Fix AT_EMPTY_PATH handling
On Jul 26 11:10, Johannes Schindelin wrote: > Hi Corinna, > > I had a look over the patches and they all make sense. I also tested the > code to make sure that the `tar xf` regression I needed to be fixed is > also addressed by this patch series. > > As I don't really know the customs of the Cygwin project, please feel free > to add any Reviewed-by:, Acked-by: or whatever footers (or, if none of > those are appropriate, I am of course totally okay with no footer at all). > > Thank you so much for fixing this! > Johannes Great, thanks for reviewing and testing! I'll push it later today. Thanks, Corinna
Re: [PATCH 0/5] Fix AT_EMPTY_PATH handling
Johannes? Ping?
On Jul 12 14:07, Corinna Vinschen wrote:
> From: Corinna Vinschen
>
> The GLIBC extension AT_EMPTY_PATH allows the functions fchownat
> and fstatat to operate on dirfd alone, if the given pathname is an
> empty string. This also allows to operate on any file type, not
> only directories.
>
> Commit fa84aa4dd2fb4 broke this. It only allows dirfd to be a
> directory in calls to these two functions.
>
> Fix that by handling AT_EMPTY_PATH right in gen_full_path_at.
> A valid dirfd and an empty pathname is now a valid combination
> and, noticably, this returns a valid path in path_ret. That
> in turn allows to remove the additional path generation code
> from the callers.
>
> Fixes: fa84aa4dd2fb ("Cygwin: fix errno values set by readlinkat")
> Reported-by: Johannes Schindelin
> Signed-off-by: Corinna Vinschen
>
> Corinna Vinschen (5):
> Cygwin: gen_full_path_at: drop never reached code
> Define _AT_NULL_PATHNAME_ALLOWED
> Cygwin: use new _AT_NULL_PATHNAME_ALLOWED flag
> Cygwin: Fix and streamline AT_EMPTY_PATH handling
> Cygwin: add AT_EMPTY_PATH fix to release message
>
> newlib/libc/include/sys/_default_fcntl.h | 11 +++--
> winsup/cygwin/release/3.4.8 | 4 ++
> winsup/cygwin/syscalls.cc| 61 ++--
> 3 files changed, 25 insertions(+), 51 deletions(-)
>
> --
> 2.40.1
Re: [PATCH] Cygwin: testsuite: Drop using DejaGnu to run tests
On Jul 21 13:29, Jon Turney wrote: > A more sophisticated (and modern) test harness would probably be useful, > but switching to Automake's built-in test harness gets us parallel test > execution, colourization of failures, simplifies matters, seems adequate > for the current testuite, and means we don't need to write any icky Tcl. > > Signed-off-by: Jon Turney > --- > .github/workflows/cygwin.yml | 2 +- > winsup/configure.ac| 2 +- > winsup/doc/faq-programming.xml | 5 +- > winsup/testsuite/Makefile.am | 27 > winsup/testsuite/README| 22 +++ > winsup/testsuite/config/default.exp| 13 > winsup/testsuite/cygrun.sh | 17 + > winsup/testsuite/winsup.api/cygload.exp| 30 - > winsup/testsuite/winsup.api/known_bugs.tcl | 4 -- > winsup/testsuite/winsup.api/winsup.exp | 74 -- > 10 files changed, 47 insertions(+), 149 deletions(-) > delete mode 100644 winsup/testsuite/config/default.exp > create mode 100755 winsup/testsuite/cygrun.sh > delete mode 100644 winsup/testsuite/winsup.api/cygload.exp > delete mode 100644 winsup/testsuite/winsup.api/known_bugs.tcl > delete mode 100644 winsup/testsuite/winsup.api/winsup.exp Good idea. Please go ahead. Thanks, Corinna
Re: [PATCH 0/2] Testsuite adjustment and relevant fix
On Jul 20 17:14, Corinna Vinschen wrote:
> On Jul 20 13:55, Jon Turney wrote:
> > On 19/07/2023 16:33, Corinna Vinschen wrote:
> > > On Jul 19 13:41, Jon Turney wrote:
> > > > [1/2] has the side effect of flipping test stat06 from working to
> > > > failing.
> > > > [2/2] fixes that
> > > >
> > > > When run with TDIRECTORY set, libltp just uses that directory and
> > > > assumes
> > > > something else will clean it up.
> > > >
> > > > When TDIRECTORY is not set, libltp creates a subdirectory under /tmp,
> > > > and when
> > > > the test is completed, removes the expected files and verifies that the
> > > > directory is empty.
> > > >
> > > > stat06 fails that check, because it creates the a file named "file"
> > > > there, and
> > > > tries stat("file", -1), testing that it returns the expected value
> > > > EFAULT.
> > > >
> > > > "file" is removed, but lingers in the STATUS_DELETE_PENDING state until
> > > > the
> > > > Windows handle which stat_worker() leaks when an exception occurs is
> > > > closed
> > > > (when the processes exits).
> > >
> > > Great find. Please push.
> >
> > So, it seems this doesn't work in an optimized build, as fh is always NULL
> > when we get around to deleting it after a fault.
> >
> > I'm thinking that I've written this wrong somehow (horses), rather than it
> > being some complex problem with how the optimizer interacts with all the
> > memory and register barriers the exception handling uses (zebras)
>
> What if you turn around the order instead?
>
> diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> index 73343ecc1f07..32ace4d38943 100644
> --- a/winsup/cygwin/syscalls.cc
> +++ b/winsup/cygwin/syscalls.cc
> @@ -1967,12 +1967,13 @@ stat_worker (path_conv , struct stat *buf)
> {
> fhandler_base *fh;
>
> - if (!(fh = build_fh_pc (pc)))
> - __leave;
> -
> debug_printf ("(%S, %p, %p), file_attributes %d",
> pc.get_nt_native_path (), buf, fh, (DWORD) *fh);
> memset (buf, 0, sizeof (*buf));
Maybe adding a MemoryBarrier() call here if all else fails...
> +
> + if (!(fh = build_fh_pc (pc)))
> + __leave;
> +
> res = fh->fstat (buf);
> if (!res)
> fh->stat_fixup (buf);
Re: [PATCH 0/2] Testsuite adjustment and relevant fix
On Jul 20 13:55, Jon Turney wrote:
> On 19/07/2023 16:33, Corinna Vinschen wrote:
> > On Jul 19 13:41, Jon Turney wrote:
> > > [1/2] has the side effect of flipping test stat06 from working to failing.
> > > [2/2] fixes that
> > >
> > > When run with TDIRECTORY set, libltp just uses that directory and assumes
> > > something else will clean it up.
> > >
> > > When TDIRECTORY is not set, libltp creates a subdirectory under /tmp, and
> > > when
> > > the test is completed, removes the expected files and verifies that the
> > > directory is empty.
> > >
> > > stat06 fails that check, because it creates the a file named "file"
> > > there, and
> > > tries stat("file", -1), testing that it returns the expected value EFAULT.
> > >
> > > "file" is removed, but lingers in the STATUS_DELETE_PENDING state until
> > > the
> > > Windows handle which stat_worker() leaks when an exception occurs is
> > > closed
> > > (when the processes exits).
> >
> > Great find. Please push.
>
> So, it seems this doesn't work in an optimized build, as fh is always NULL
> when we get around to deleting it after a fault.
>
> I'm thinking that I've written this wrong somehow (horses), rather than it
> being some complex problem with how the optimizer interacts with all the
> memory and register barriers the exception handling uses (zebras)
What if you turn around the order instead?
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 73343ecc1f07..32ace4d38943 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1967,12 +1967,13 @@ stat_worker (path_conv , struct stat *buf)
{
fhandler_base *fh;
- if (!(fh = build_fh_pc (pc)))
- __leave;
-
debug_printf ("(%S, %p, %p), file_attributes %d",
pc.get_nt_native_path (), buf, fh, (DWORD) *fh);
memset (buf, 0, sizeof (*buf));
+
+ if (!(fh = build_fh_pc (pc)))
+ __leave;
+
res = fh->fstat (buf);
if (!res)
fh->stat_fixup (buf);
> If I had infinite time, maybe I'd review the source code to see if there are
> any other instances where we fail to properly dispose of objects created in
> a __try block...
I like the idea of infinite time...
Corinna
