Re: [PATCH 1/2] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney <jon.turney@dronecode.org.uk>]

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.