Fix km_error_key() infinite loop and unget buffer pollution.

'bind pager \Ch help' produces an infinite loop when an unbound key is
pressed in the pager.  The reason is because km_error_key() tries to
verify that the key sequence is really bound to the OP_HELP operation.
It does this by using km_expand_key(), tokenize_unget_string() on the
resulting buffer, then checking if the next km_dokey() returns OP_HELP.

The problem is that km_expand_key() does not always produce a string
that is properly reparsed by tokenize_unget_string().  Control-h
sequences are expanded to ^H.  tokenize_unget_string() recognizes this
as two characters '^' and 'H'.  km_error_key() checks the OP returned,
which is OP_PAGER_TOP for the '^'.  This is not OP_HELP, so it prints
a generic error and returns.  This leaves the 'H' in the input buffer!
Since 'H' (by default) is unbound in the pager, it retriggers
km_error_key(), resulting in an infinite loop.

The same issues can occur without control sequences:
  bind generic ? noop
  bind generic dq help
In the index, hitting an unbound key will end up leaving 'q' in the unget
buffer, because 'd' is bound in the index menu and will be read by km_dokey().

A simple approach to fix this would be to just use the same code as in
mutt_make_help(), which has no double-check.  This would be no worse
than the help menu, but can generate an inaccurate error message (e.g
if '?' were bound to noop)

This patch instead uses OP_END_COND as a barrier in the unget buffer.
It directly inserts the keys in the OP_HELP keymap, instead of using
km_expand_key() + tokenize_unget_string().  After calling km_dokey()
it flushes the unget buffer to the OP_END_COND barrier.

Thanks to Walter Alejandro Iglesias for reporting the bug.

diff --git a/curs_lib.c b/curs_lib.c
index c4c42b425fc0e066d801bc87edb4d91ed030f776..9d108755724fbc777ffc8e460d228f2d85065d2f 100644 (file)
--- a/curs_lib.c
+++ b/curs_lib.c
@@ -826,6 +826,18 @@ void mutt_flush_macro_to_endcond (void)
   }
 }
 
+/* Normally, OP_END_COND should only be in the MacroEvent buffer.
+ * km_error_key() (ab)uses OP_END_COND as a barrier in the unget
+ * buffer, and calls this function to flush. */
+void mutt_flush_unget_to_endcond (void)
+{
+  while (UngetCount > 0)
+  {
+    if (UngetKeyEvents[--UngetCount].op == OP_END_COND)
+      return;
+  }
+}
+
 void mutt_flushinp (void)
 {
   UngetCount = 0;

diff --git a/keymap.c b/keymap.c
index 07147bd218e765309083342499be2a99de29d36a..1e8246baff862b648d75a40824d4b5cde8e70340 100644 (file)
--- a/keymap.c
+++ b/keymap.c
@@ -837,24 +837,53 @@ void km_error_key (int menu)
 {
   char buf[SHORT_STRING];
   struct keymap_t *key;
+  int p, op;
 
-  if(!(key = km_find_func(menu, OP_HELP)))
-    key = km_find_func(MENU_GENERIC, OP_HELP);
-  
-  if(!(km_expand_key(buf, sizeof(buf), key)))
+  key = km_find_func (menu, OP_HELP);
+  if (!key && (menu != MENU_EDITOR) && (menu != MENU_PAGER))
+    key = km_find_func (MENU_GENERIC, OP_HELP);
+  if (!key)
   {
     mutt_error _("Key is not bound.");
     return;
   }
 
-  /* make sure the key is really the help key in this menu */
-  tokenize_unget_string (buf);
-  if (km_dokey (menu) != OP_HELP)
+  /* Make sure the key is really the help key in this menu.
+   *
+   * OP_END_COND is used as a barrier to ensure nothing extra
+   * is left in the unget buffer.
+   *
+   * Note that km_expand_key() + tokenize_unget_string() should
+   * not be used here: control sequences are expanded to a form
+   * (e.g. "^H") not recognized by km_dokey(). */
+  mutt_unget_event (0, OP_END_COND);
+  p = key->len;
+  while (p--)
+    mutt_unget_event (key->keys[p], 0);
+
+  /* Note, e.g. for the index menu:
+   *   bind generic ?   noop
+   *   bind generic ,a  help
+   *   bind index   ,ab quit
+   * The index keybinding shadows the generic binding.
+   * OP_END_COND will be read and returned as the op.
+   *
+   *   bind generic ?   noop
+   *   bind generic dq  help
+   *   bind index   d   delete-message
+   * OP_DELETE will be returned as the op, leaving "q" + OP_END_COND
+   * in the unget buffer.
+   */
+  op = km_dokey (menu);
+  if (op != OP_END_COND)
+    mutt_flush_unget_to_endcond ();
+  if (op != OP_HELP)
   {
     mutt_error _("Key is not bound.");
     return;
   }
 
+  km_expand_key (buf, sizeof(buf), key);
   mutt_error (_("Key is not bound.  Press '%s' for help."), buf);
   return;
 }

diff --git a/mutt_curses.h b/mutt_curses.h
index 62004491fcb44f0cec4574dd8c881f5492989303..84e60984675317a9aff791fc83a09de8062e7b90 100644 (file)
--- a/mutt_curses.h
+++ b/mutt_curses.h
@@ -96,6 +96,7 @@ void mutt_unget_event (int, int);
 void mutt_unget_string (char *);
 void mutt_push_macro_event (int, int);
 void mutt_flush_macro_to_endcond (void);
+void mutt_flush_unget_to_endcond (void);
 void mutt_need_hard_redraw (void);
 
 /* ----------------------------------------------------------------------------