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.
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?
(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)
[...]
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?
Yeah, I'm just being lazy here.
I think this could only actually be security hole if the crashing
process was setuid (or otherwise had elevated capabilities), which we
don't support, but I should do this the safe way. I'll fix 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?
Maybe. That would make forward/backwards compatibility problems when
mixing dumper and cygwin versions.
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.
[...]
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 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().
Well, it's garbage anyhow because we can calculate the exact size of the
output before we do the allocation, which is presumably usually much less.
+extern "C" void
+dumper_init(void)
^^^
space
+{
+ command_prep("dumper", &dumper_command);
^^^
space
Doh!
[...]
+
+ sig |= 0x80; /* Set WCOREDUMP flag in exit code to show that we've "dumped
core" */
While at it, we could introduce `#define __WCOREFLAG 0x80' to sys/wait.h
as on linux. But that would be an extra patch.
Yeah, let's keep that separate.
