[Tcsh] tcsh 6.22.00 does not merge history (was: Re: tcsh Deadlock with SIGHUP)

[John McCue]

Hi Brett,

I was also having issues with history and I applied your
patch when you sent this email out.  I applied it on 2
different Linux distributions (work/home) and so far no
issues.

But, I never had the SIGHUP/deadlock issues referred to,
so based on my usage I doubt I will notice any regressions
at this point.

Thanks for the patch, I was scratching my head of this.

John



On Sun, Feb 23, 2020 at 02:55:56PM -0600, Brett Frankenberger wrote:
>So I replaced my custom tcsh with the current source and confirmed that
>the fix you implemented prevents the deadlock, but in the process of
>testing, found another bug -- tcsh no longer (as of 6.21) properly
>merges history even when savehist is configured to do so.  (This is
>unrelated to the fix for the SIGHUP deadlock.)
>
>For example, if you do the following (with savehist = (100 merge lock))
>(although the lock is, I think, not important here):
>
>  (1) Launch two sessions.
>  (2) In the first session, enter the command "testcommand1".
>  (2) In the second session, enter the command "testcommand2".
>  (3) Exit the first session.
>  (4) Exit the second session.
>  (5) Launch a new session.
>
>Then, in the new session, the history will only include "testcommand2";
>it will not include "testcommmand1", because when the second sesson
>exited (step 4 above), the history file written by the first session
>(which exited in step 3 above) is not merged.
>
>This appears to have broken between 6.21.00 and 6.22.00; I believe this
>fix:
>   4. Avoid infinite loops during history loads when merging, print a better
>      error for errors during history load.
>is the cause; specifically, this code in sh.hist.c:
>   - loadhist(fname, 1);
>   + getexit(osetexit);
>   + if (setexit())
>   +     loadhist(fname, 1);
>   + resexit(osetexit);
>
>It looks to me like "if (setexit())" should be "if (!setexit())".
>(setexit() is ultimately a wrapper around setjmp() or sigsetjmp(), both
>of which return 0 on their first invocation.  So the code as written is
>skipping loadhist() the first time through, which appears to be the
>opposite of what was intended.)
>
>Replacing "setexit()" with "!setexit()" fixed the merge issue for me.
>Specifically:
>
>============
>--- tcsh-6.22.02.orig/sh.hist.c
>+++ tcsh-6.22.02/sh.hist.c
>@@ -1292,7 +1292,7 @@ rechist(Char *fname, int ref)
> #endif
>            }
>            getexit(osetexit);
>-           if (setexit())
>+           if (!setexit())
>                loadhist(fname, 1);
>            resexit(osetexit);
>        }
>============
>
>     -- Brett
>
>On Mon, Jan 20, 2020 at 11:40:49AM -0500, Christos Zoulas wrote:
>> Thanks, it should be fixed now.
>>
>> christos
>>
>> > On Jan 20, 2020, at 9:08 AM, Brett Frankenberger <rbf at rbfnet.com> wrote:
>> >
>> > (Resending what I posted last August ... let me know if there's
>> > anything I can do to get this (or a differnet fix for the same issue)
>> > into the tree.)
>> >
>> > tcsh can deadlock with itself if savehist is confgured with "merge" and
>> > "lock", and two SIGHUPs are received in rapid succession.  The
>> > mechanism of the deadlock is the first SIGHUP triggers a rechist() and
>> > while that rechist() is executing (and after it has created the lock
>> > file), the second SIGHUP triggers a another rechist() which then waits
>> > forever for the lock the the first rechist() created to be released
>> > (which will never happen).
>
>    [ . . . ]
>-- 
>Tcsh mailing list
>Tcsh at astron.com
>https://mailman.astron.com/mailman/listinfo/tcsh