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

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

On 11/01/2024 09:42, Corinna Vinschen wrote:
> 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.

I was more thinking that anything that's using it for a serious 
purposes, should probably be using libunwind or something similar, which 
maybe doesn't exist or work on Cygwin currently (or didn't back when it 
was written).

In any case, any change to cygwin_stackdump() seems out of scope for 
this work, so can we park that discussion for the moment.

> 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.

Good news.

> 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?

I don't think we want to get into second-guessing why an application is
calling cygwin_stackdump(), and changing it when it's deprecated and 
going to be removed seems silly.

Looking at the history, the current behaviour (of cygwin_stackdump() not 
dumping based on rlim_core) seems like simply an unintended side-effect 
of where the check is done.

Anyhow, rather than improving this function, there are, or at least 
should be, better user-space alternatives to use.

I hadn't noticed that api_fatal() also calls cygwin_stackdump().

Assuming generating a stackdump at that point has some value, it does 
seems to make sense to treat it the same.  (But given all the 
signal-specific things that signal_exit() does, I'm not sure the code 
can be made common).

>>> 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...

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))