Re: fix invalid uses of siglongjmp

[Jose E. Marchesi]

Hi Bruno.

    In pk-repl.c there are three cases of invalid siglongjmp invocations
    (assuming a single-threaded poke).

Thanks for the patch and the explanation.
OK for master.


    To reproduce one of them, press Ctrl-D and then very quickly
    Ctrl-C. I see this:
    
      Type ".exit" to leave the program.
      (poke) 
      ^C
      (poke) .load gmo.pk
      /media/develdata/devel/inst-x86_64-64/share/poke/gmo.pk: Datei oder 
Verzeichnis nicht gefunden
      (poke) 
      *** stack smashing detected ***: ./poke/poke terminated
      Abgebrochen (Speicherabzug geschrieben)
    
    The three cases are:
      (1) If the user presses Ctrl-C between the moment the signal handler
          is installed and the moment the first sigsetjmp is terminated.
      (2) If the user presses Ctrl-C between the moment a round of the REP loop
          is terminated and the next round of the REP loop is started.
      (3) If the user presses Ctrl-C after the function pk_repl has terminated.
          (This is the scenario above.)
    
    In case (1) and (3), it is better for the program to behave like if no
    handler was installed, that is, terminate the program.
    
    In case (2), the user does not want to terminate poke; instead he wants
    to (reliably!) get at the poke prompt again.
    
    The attached proposed patch implements this. With it, I see this:
    
      Type ".exit" to leave the program.
      (poke) 
      ^C
    
    A remark regarding the use of 'volatile': 'volatile' is needed for every
    global variable that is accessed from a signal handler. (Think about the
    main thread and the signal handler being executed on different CPUs.
    Without 'volatile' and no memory barrier, the CPU that executes the main
    thread might never propagate changes to the variable to the other CPUs.)
    Unfortunately, I can't mark a sigjmpbuf variable as 'volatile' easily,
    without introducing compilation warnings...
    
    Bruno
    
    >From 5174f470b65a61496f66ff3227b8f2dea749fdc2 Mon Sep 17 00:00:00 2001
    From: Bruno Haible <bruno@clisp.org>
    Date: Sat, 27 Jun 2020 17:04:22 +0200
    Subject: [PATCH] Fix invalid uses of siglongjmp.
    
    * poke/pk-repl.c (ctrlc_buf_valid): New variable.
    (poke_sigint_handler): If ctrlc_buf_valid is false, terminate the process.
    (pk_repl): Set ctrlc_buf_valid to false before installing the signal handler
    and to true while ctrlc_buf is valid. Move the sigsetjmp invocation out of 
the
    loop.
    ---
     poke/pk-repl.c | 42 ++++++++++++++++++++++++++++++++----------
     1 file changed, 32 insertions(+), 10 deletions(-)
    
    diff --git a/poke/pk-repl.c b/poke/pk-repl.c
    index 08505cb..38a53a5 100644
    --- a/poke/pk-repl.c
    +++ b/poke/pk-repl.c
    @@ -37,7 +37,10 @@
     #include "pk-utils.h"
     #include "pk-map.h"
     
    -static sigjmp_buf ctrlc_buf;
    +/* The non-local entry point for reentering the REPL.  */
    +static sigjmp_buf /*volatile*/ ctrlc_buf;
    +/* When nonzero, ctrlc_buf contains a valid value.  */
    +static int volatile ctrlc_buf_valid;
     
     /* This function is called repeatedly by the readline library, when
        generating potential command line completions.
    @@ -202,13 +205,28 @@ banner (void)
     }
     
     static void
    -poke_sigint_handler (int status)
    +poke_sigint_handler (int sig)
     {
    -  rl_free_line_state ();
    -  rl_cleanup_after_signal ();
    -  rl_line_buffer[rl_point = rl_end = rl_mark = 0] = 0;
    -  printf ("\n");
    -  siglongjmp (ctrlc_buf, 1);
    +  if (ctrlc_buf_valid)
    +    {
    +      /* Abort the current command, and return to the REPL.  */
    +      rl_free_line_state ();
    +      rl_cleanup_after_signal ();
    +      rl_line_buffer[rl_point = rl_end = rl_mark = 0] = 0;
    +      printf ("\n");
    +      siglongjmp (ctrlc_buf, 1);
    +    }
    +  else
    +    {
    +      /* The default behaviour for SIGINT is to terminate the process.
    +         Let's do that here.  */
    +      struct sigaction sa;
    +      sa.sa_handler = SIG_DFL;
    +      sa.sa_flags = 0;
    +      sigemptyset (&sa.sa_mask);
    +      sigaction (SIGINT, &sa, NULL);
    +      raise (SIGINT);
    +    }
     }
     
     /* Return a copy of TEXT, with every instance of the space character
    @@ -272,11 +290,12 @@ pk_repl (void)
       banner ();
     
       /* Arrange for the current line to be cancelled on SIGINT.  */
    +  ctrlc_buf_valid = 0;
       struct sigaction sa;
       sa.sa_handler = poke_sigint_handler;
       sa.sa_flags = 0;
       sigemptyset (&sa.sa_mask);
    -  sigaction (SIGINT, &sa, 0);
    +  sigaction (SIGINT, &sa, NULL);
     
     #if defined HAVE_READLINE_HISTORY_H
       char *poke_history = NULL;
    @@ -300,13 +319,14 @@ pk_repl (void)
       rl_filename_quote_characters = " ";
       rl_filename_quoting_function = escape_metacharacters;
     
    +  sigsetjmp (ctrlc_buf, 1);
    +  ctrlc_buf_valid = 1;
    +
       while (!poke_exit_p)
         {
           char *prompt;
           char *line;
     
    -      while (sigsetjmp (ctrlc_buf, 1) != 0);
    -
           pk_term_flush ();
           rl_completion_entry_function = poke_completion_function;
           prompt = pk_prompt ();
    @@ -345,6 +365,8 @@ pk_repl (void)
         free (poke_history);
       }
     #endif
    +
    +  ctrlc_buf_valid = 0;
     }
     
     static int saved_point;