bug#25391: 24.5; ffap-guesser "stack overflow in regexp matcher" error may crash emacs.

[npostavs]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>>
>> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>>
>>> However, this handle the problem with "Stack overflow in regexp matcher"
>>> generally, but perhaps regexps such as `ffap-gopher-regexp` could be
>>> avoided when possible in emacs source code.
>>
>> Ah, so by "crash" you meant throw an error.
>
> I had the both, a crash on emacs-26, emacs taking all memory and finally
> crashing and on emacs-24.5 "Stack overflow in regexp matcher".

Hmm, well I've meanwhile merged the fix for the regex stack limit, if
you can still crash emacs this way, open a new bug.

>
>
>> Right, and ffap-gopher-at-point is also using `set' on local variables,
>> icky.  Here's a patch:
>
> I don't know `let-alist`, but looks good, however keeping
> `ffap-gopher-regexp` instead of hardcoding it and returning nil when the
> regexp is nil would allow users to disable the feature.

Makes sense, here's an update:

>From 93875e1d9dba4509ddd18ade9b09bf6b258d2174 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 8 Jan 2017 18:19:32 -0500
Subject: [PATCH v2] Improve ffap-gopher-at-point handling of long lines

* lisp/ffap.el (ffap-gopher-regexp): Only match the KEY part.
(ffap--gopher-var-on-line): New function.
(ffap-gopher-at-point): Use it instead of the old ffap-gopher-regexp
which could overflow the regexp stack on long lines (Bug#25391).  Use
`let-alist' instead of calling `set' on local variables.
* test/lisp/ffap-tests.el (ffap-gopher-at-point): New test.
---
 lisp/ffap.el            | 61 +++++++++++++++++++++++++------------------------
 test/lisp/ffap-tests.el | 15 ++++++++++++
 2 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/lisp/ffap.el b/lisp/ffap.el
index 8144d41..bcc8911 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -1194,9 +1194,18 @@ ffap-url-at-point
           val))))
 
 (defvar ffap-gopher-regexp
-  "^.*\\<\\(Type\\|Name\\|Path\\|Host\\|Port\\) *= *\\(.*\\) *$"
-  "Regexp matching a line in a gopher bookmark (maybe indented).
-The two subexpressions are the KEY and VALUE.")
+  "\\<\\(Type\\|Name\\|Path\\|Host\\|Port\\) *= *"
+  "Regexp matching a key in a gopher bookmark.")
+
+(defun ffap--gopher-var-on-line ()
+  "Return (KEY . VALUE) of gopher bookmark on current line."
+  (save-excursion
+    (let ((eol (progn (end-of-line) (skip-chars-backward " ") (point)))
+          (bol (progn (beginning-of-line) (point))))
+     (when (re-search-forward ffap-gopher-regexp eol t)
+       (let ((key (match-string 1))
+             (val (buffer-substring-no-properties (match-end 0) eol)))
+         (cons (intern (downcase key)) val))))))
 
 (defun ffap-gopher-at-point ()
   "If point is inside a gopher bookmark block, return its URL.
@@ -1204,33 +1213,25 @@ ffap-gopher-at-point
 Sets the variable `ffap-string-at-point-region' to the bounds of URL, if any."
   ;; `gopher-parse-bookmark' from gopher.el is not so robust
   (save-excursion
-    (beginning-of-line)
-    (if (looking-at ffap-gopher-regexp)
-       (progn
-         (while (and (looking-at ffap-gopher-regexp) (not (bobp)))
-           (forward-line -1))
-         (or (looking-at ffap-gopher-regexp) (forward-line 1))
-          (setq ffap-string-at-point-region (list (point) (point)))
-         (let ((type "1") path host (port "70"))
-           (while (looking-at ffap-gopher-regexp)
-             (let ((var (intern
-                         (downcase
-                          (buffer-substring (match-beginning 1)
-                                            (match-end 1)))))
-                   (val (buffer-substring (match-beginning 2)
-                                          (match-end 2))))
-               (set var val)
-               (forward-line 1)))
-            (setcdr ffap-string-at-point-region (list (point)))
-           (if (and path (string-match "^ftp:.*@" path))
-               (concat "ftp://";
-                       (substring path 4 (1- (match-end 0)))
-                       (substring path (match-end 0)))
-             (and (= (length type) 1)
-                  host;; (ffap-machine-p host)
-                  (concat "gopher://"; host
-                          (if (equal port "70") "" (concat ":" port))
-                          "/" type path))))))))
+    (let* ((beg (progn (beginning-of-line)
+                       (while (and (not (bobp)) (ffap--gopher-var-on-line))
+                         (forward-line -1))
+                       (point)))
+           (bookmark (cl-loop for keyval = (ffap--gopher-var-on-line)
+                              while keyval collect keyval
+                              do (forward-line 1))))
+      (when bookmark
+        (setq ffap-string-at-point-region (list beg (point)))
+        (let-alist (nconc bookmark '((type . "1") (port . "70")))
+          (if (and .path (string-match "\\`ftp:.*@" .path))
+              (concat "ftp://";
+                      (substring .path 4 (1- (match-end 0)))
+                      (substring .path (match-end 0)))
+            (and (= (length .type) 1)
+                 .host ;; (ffap-machine-p host)
+                 (concat "gopher://"; .host
+                         (if (equal .port "70") "" (concat ":" .port))
+                         "/" .type .path))))))))
 
 (defvar ffap-ftp-sans-slash-regexp
   (and
diff --git a/test/lisp/ffap-tests.el b/test/lisp/ffap-tests.el
index 1ba5f86..f3414ac 100644
--- a/test/lisp/ffap-tests.el
+++ b/test/lisp/ffap-tests.el
@@ -49,6 +49,21 @@
               (should (equal '(1 1) ffap-string-at-point-region)))))
       (and (file-exists-p file) (delete-file file)))))
 
+(ert-deftest ffap-gopher-at-point ()
+  (with-temp-buffer
+    (insert "\
+Type = 1
+Name = foo
+Path = /the/path
+Port = 7070
+Host = example.com\n")
+    (should-not (ffap-gopher-at-point))
+    (goto-char (point-min))
+    (should (equal (ffap-gopher-at-point)
+                   "gopher://example.com:7070/1/the/path";))
+    (should (equal ffap-string-at-point-region
+                   (list (point-min) (point-max))))))
+
 (provide 'ffap-tests)
 
 ;;; ffap-tests.el ends here
-- 
2.9.3