bug#59976: ERC 5.4.1: erc-networks--id gets clobbered in erc server buffer on /query name conflict

["J.P." <jp@neverwas.me>]

[-- Attachment #1: Type: text/plain, Size: 5627 bytes --]

Mike Kazantsev <mk.fraggod@gmail.com> writes:

> Hi.

Hi Mike,

Thanks for the detailed bug report. I mostly just have questions for now
but am committed to finding a solution, so please bear with me.

> I've needed to use case-sensitive channel names (with matrix2051 ad-hoc
> ircd), wanted to test updated ERC client for that, and stumbled upon
> what looks like a bug there:
>
>   When IRC server is named "slack", and then it sends you a message from
>   user named "slack", ERC tries to open query-buffer "slack", ends up
>   running erc--open-target("slack") which does kill-all-local-variables()
>   when enabling erc-mode in the buffer.
>
>   That removes erc-networks--id value there and from then on ERC keeps
>   spamming "error in process filter" about erc-networks--id being nil.
>
>
> - What I'm trying to do:
>
>   - (erc-tls ...)  ;; cleates "slack" server buffer and some channels.
>   - Open "slack" buffer

You mean opened manually, like by issuing a /query slack, right? So this
is in addition to the observation above where "it sends you a message
from user named 'slack'"?

>   - Send "/ping myuser" IRC-command there
>

You also mention "case-sensitive channel names." It'd be nice to know
what those look like when they first arrive off the wire. (BTW, if
you're expecting ERC to interoperate with a case-sensitive network, like
one for which "#FOO" and "#foo" are distinct channels, then that may
need a separate bug report.)

> - Expected result:
>
>   Command actually results in PRIVMSG from "slack" user on this
>   specific ircd, so I expect to see that open a separate query-buffer
>   where that message is displayed.
>
>   (message in question - though probably doesn't matter:
>   "msg: ^APING 1670781447.080567^A could not be sent channel_not_found")
>
>
> - Actual result:
>
>   - Emacs slows down while printing following errors to MiniBuffer and *Messages*:
>
>       error in process filter: or: Wrong type argument: erc-networks--id, nil
>       error in process filter: Wrong type argument: erc-networks--id, nil
>
>   - As far as I can tell that IRC connection becomes unusable, and
>     errors like above get signaled from then on randomly, likely on
>     some commands sent from ircd.
>
>
> - Workaround: changing announced name of the server to something else
>   helps - "slack" query-buffer gets created and is separate from server buffer.

You mean by changing `erc-server-announced-name'?

> Was able to understand what seem to be the issue here by enabling
> logging for erc-networks--id changes to *Messages* like this:
>
>   (defun debug-watch-log (sym newval op where)
>     (message "Variable update: %s = %S -> %S [%S %S]"
>       sym (symbol-value sym) newval op where)
>     (backtrace))
>   (add-variable-watcher 'erc-networks--id #'debug-watch-log)
>
> (run M-x cancel-debug-on-variable-change afterwards to disable this)

Thanks for the detective work. In this case, I'm pretty sure the root
cause is not directly related to that variable, despite all appearances.

BTW, the most helpful insights are usually to be found in the contents
of the *erc-protocol* buffer, which can be generated by calling
`erc-toggle-debug-irc-protocol' before connecting for the first time in
an Emacs session. If you're not comfortable sharing such info on the
tracker, please consider doing so out of band.

> I think some kind of disambiguation or conflict-detection for
> query-channel names might be either missing or not working atm,
> but is needed to avoid this happening unintentionally, or maybe
> even on purpose (e.g. to annoy someone by cutting their IRC connection).

Agreed.

Upon detecting the collision, perhaps the query buffer needs to become
"slack!~slack@example.com" (or "slack@slack/me") and/or the server
buffer "slack/me". Additionally, we could have an option that says to
always use full n!u@h's when naming query buffers instead of adapting
dynamically.

That said, at the moment, we only support unambiguous "network IDs" via
the ":id" keyword parameter of `erc' and `erc-tls'. IOW, calling these
with args like

  :server "127.0.0.1"
  :port 2051
  ...
  :id 'Slack

is supposed to always work. But, I can definitely see how that doesn't
in some cases (e.g., a lowercase `:id'). As you say, you may have no
control over who sends you a query, which is still pertinent in the case
of an explicit `:id' so long as it consists entirely of valid nick
characters.

Unfortunately, the best we can do for ERC 5.5 (Emacs 29) is to mention
somewhere, like in (info "(erc) Network Identifier"), that users really
worried about this issue should choose an `:id' containing characters
disallowed in nicks by their network (or just something improbable and
unlikely to be guessed). But, hopefully, we can address this in a more
DWIM-like fashion in an upcoming ELPA release, such as ERC 5.6.

> I'm running ERC 5.4.1 from current 0e5d059 git master on Emacs 28.2
> (replacing "erc" dir in /usr/share/emacs), so it is also possible that
> maybe some change in Emacs 29+ prevents this behavior, but it seems unlikely.

Unlikely, yes. But I sometimes worry about ERC's loaddefs when people
manually shadow/replace instead of building alongside Emacs or
installing from devel:

  (push '("devel" . "https://elpa.gnu.org/devel/") package-archives)

(Not saying you need to do either, of course.) Thanks again for the very
nice bug report.

J.P.

P.S. I've attached some patches for Emacs 29 that address issues
discovered while briefly looking into this bug; the first is totally
unrelated and the latter two only tangentially so (in case anyone wants
to take a gander).



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Be-carefuller-updating-browse-url-var-in-erc-compat.patch --]
[-- Type: text/x-patch, Size: 1476 bytes --]

From d7887fb9059ecd36d7209a98af2f4ff649e8642b Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 11 Dec 2022 19:16:07 -0800
Subject: [PATCH 1/3] ; Be carefuller updating browse-url var in erc-compat

* lisp/erc/erc-compat.el: Be more cautious about modifying
`browse-url-default-handlers' when loading erc-compat on Emacs 28.  A
user may have already added an entry for irc:// URLs before loading
`erc-compat'.
---
 lisp/erc/erc-compat.el | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lisp/erc/erc-compat.el b/lisp/erc/erc-compat.el
index bd93254758..884abaf824 100644
--- a/lisp/erc/erc-compat.el
+++ b/lisp/erc/erc-compat.el
@@ -389,10 +389,17 @@ erc-compat--29-browse-url-irc
             url-irc-function)))
     (url-irc url)))
 
+(declare-function cl-adjoin "cl-lib" (arg1 arg2 &rest rest))
+
 (cond ((fboundp 'browse-url-irc)) ; 29
       ((boundp 'browse-url-default-handlers) ; 28
+       (require 'cl-lib)
        (cl-pushnew '("\\`irc6?s?://" . erc-compat--29-browse-url-irc)
-                   browse-url-default-handlers))
+                   browse-url-default-handlers
+                   :key #'car
+                   :test (lambda (_ b)
+                           (and (stringp b)
+                                (string-match-p b "irc://localhost")))))
       ((boundp 'browse-url-browser-function) ; 27
        (require 'browse-url)
        (let ((existing browse-url-browser-function))
-- 
2.38.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Actually-accept-non-symbols-as-IDs-in-erc-open.patch --]
[-- Type: text/x-patch, Size: 3351 bytes --]

From c6423e8834e7ed7101e800931bc461a960083aa9 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 11 Dec 2022 19:41:43 -0800
Subject: [PATCH 2/3] Actually accept non-symbols as IDs in erc-open

* lisp/erc/erc.el (erc-generate-new-buffer-name): Despite what it says
in the documentation, only symbols were being accepted as valid :id
entry point arguments.  This ensures that the `princ' form is used
instead.
* test/lisp/erc/erc-scenarios-base-netid-samenet.el
(erc-scenarios-common--base-network-id-same-network): Randomly specify
a string for the ID param instead of a non-nil symbol when opening a
new connection.
* test/lisp/erc/resources/erc-scenarios-common.el
(erc-scenarios-common-assert-initial-buf-name): Adjust helper to allow
for non-symbol IDs.
---
 lisp/erc/erc.el                                   | 9 +++++----
 test/lisp/erc/erc-scenarios-base-netid-samenet.el | 3 +++
 test/lisp/erc/resources/erc-scenarios-common.el   | 2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 6bb2e013c3..a5ba45d9b3 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1607,11 +1607,12 @@ erc-generate-new-buffer-name
     (if (and (with-suppressed-warnings ((obsolete erc-reuse-buffers))
                erc-reuse-buffers)
              id)
-        (progn
-          (when-let* ((buf (get-buffer (symbol-name id)))
+        (let ((string (symbol-name (erc-networks--id-symbol
+                                    (erc-networks--id-create id)))))
+          (when-let* ((buf (get-buffer string))
                       ((erc-server-process-alive buf)))
-            (user-error  "Session with ID %S already exists" id))
-          (symbol-name id))
+            (user-error  "Session with ID %S already exists" string))
+          string)
       (generate-new-buffer-name (if (and server port)
                                     (if (with-suppressed-warnings
                                             ((obsolete erc-reuse-buffers))
diff --git a/test/lisp/erc/erc-scenarios-base-netid-samenet.el b/test/lisp/erc/erc-scenarios-base-netid-samenet.el
index 3cd8b7dfa1..1436712251 100644
--- a/test/lisp/erc/erc-scenarios-base-netid-samenet.el
+++ b/test/lisp/erc/erc-scenarios-base-netid-samenet.el
@@ -40,6 +40,9 @@ erc-scenarios-common--base-network-id-same-network
        (erc-server-flood-margin 30)
        erc-serv-buf-a erc-serv-buf-b)
 
+    (when (and id-a (zerop (random 2))) (setq id-a (symbol-name id-a)))
+    (when (and id-b (zerop (random 2))) (setq id-b (symbol-name id-b)))
+
     (ert-info ("Connect to foonet with nick tester")
       (with-current-buffer
           (setq erc-serv-buf-a (erc :server "127.0.0.1"
diff --git a/test/lisp/erc/resources/erc-scenarios-common.el b/test/lisp/erc/resources/erc-scenarios-common.el
index d77b32984b..57d4658e75 100644
--- a/test/lisp/erc/resources/erc-scenarios-common.el
+++ b/test/lisp/erc/resources/erc-scenarios-common.el
@@ -185,7 +185,7 @@ erc-scenarios-common-with-cleanup
 (defun erc-scenarios-common-assert-initial-buf-name (id port)
   ;; Assert no limbo period when explicit ID given
   (should (string= (if id
-                       (symbol-name id)
+                       (format "%s" id)
                      (format "127.0.0.1:%d" port))
                    (buffer-name))))
 
-- 
2.38.1


[-- Attachment #4: 0003-Limit-casemapping-to-appropriate-ranges-in-ERC.patch --]
[-- Type: text/x-patch, Size: 5004 bytes --]

From 28eb435ca3756ecde6791162e7ef8de1018113b2 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 11 Dec 2022 19:41:43 -0800
Subject: [PATCH 3/3] Limit casemapping to appropriate ranges in ERC

* lisp/erc/erc-common.el (erc-downcase): Use case table for
`erc-downcase' so that case conversions are limited to the ASCII
interval.
* lisp/erc/erc.el (erc-casemapping--rfc1459-strict,
erc--casemapping-rfc1459): Make these case tables instead of
translation tables.  The functions in case-table.el modify the
standard syntax table, but that doesn't seem to make sense here,
right?
* test/lisp/erc/erc-tests.el (erc-downcase): Add cases showing
mappings outside of the ASCII range.
---
 lisp/erc/erc-common.el     | 16 +++++-----------
 lisp/erc/erc.el            | 28 ++++++++++++++++++++--------
 test/lisp/erc/erc-tests.el |  3 +++
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index a4046ba9b3..e662c06daa 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -301,17 +301,11 @@ erc-log
 (defun erc-downcase (string)
   "Return a downcased copy of STRING with properties.
 Use the CASEMAPPING ISUPPORT parameter to determine the style."
-  (let* ((mapping (erc--get-isupport-entry 'CASEMAPPING 'single))
-         (inhibit-read-only t))
-    (if (equal mapping "ascii")
-        (downcase string)
-      (with-temp-buffer
-        (insert string)
-        (translate-region (point-min) (point-max)
-                          (if (equal mapping "rfc1459-strict")
-                              erc--casemapping-rfc1459-strict
-                            erc--casemapping-rfc1459))
-        (buffer-string)))))
+  (with-case-table (pcase (erc--get-isupport-entry 'CASEMAPPING 'single)
+                     ("ascii" ascii-case-table)
+                     ("rfc1459-strict" erc--casemapping-rfc1459-strict)
+                     (_ erc--casemapping-rfc1459))
+    (downcase string)))
 
 (define-inline erc-get-channel-user (nick)
   "Find NICK in the current buffer's `erc-channel-users' hash table."
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index a5ba45d9b3..195fb4c730 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -407,15 +407,27 @@ erc-server-users
   "Hash table of users on the current server.
 It associates nicknames with `erc-server-user' struct instances.")
 
-(defconst erc--casemapping-rfc1459
-  (make-translation-table
-   '((?\[ . ?\{) (?\] . ?\}) (?\\ . ?\|) (?~  . ?^))
-   (mapcar (lambda (c) (cons c (+ c 32))) "ABCDEFGHIJKLMNOPQRSTUVWXYZ")))
-
 (defconst erc--casemapping-rfc1459-strict
-  (make-translation-table
-   '((?\[ . ?\{) (?\] . ?\}) (?\\ . ?\|))
-   (mapcar (lambda (c) (cons c (+ c 32))) "ABCDEFGHIJKLMNOPQRSTUVWXYZ")))
+  (let ((tbl (copy-sequence ascii-case-table))
+        (cup (copy-sequence (char-table-extra-slot ascii-case-table 0))))
+    (set-char-table-extra-slot tbl 0 cup)
+    (set-char-table-extra-slot tbl 1 nil)
+    (set-char-table-extra-slot tbl 2 nil)
+    (pcase-dolist (`(,uc . ,lc) '((?\[ . ?\{) (?\] . ?\}) (?\\ . ?\|)))
+      (aset tbl uc lc)
+      (aset tbl lc lc)
+      (aset cup uc uc))
+    tbl))
+
+(defconst erc--casemapping-rfc1459
+  (let ((tbl (copy-sequence erc--casemapping-rfc1459-strict))
+        (cup (copy-sequence (char-table-extra-slot
+                             erc--casemapping-rfc1459-strict 0))))
+    (set-char-table-extra-slot tbl 0 cup)
+    (aset tbl ?~ ?^)
+    (aset tbl ?^ ?^)
+    (aset cup ?~ ?~)
+    tbl))
 
 (defun erc-add-server-user (nick user)
   "This function is for internal use only.
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 4d0d69cd7b..e30452b1cf 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -428,18 +428,21 @@ erc-downcase
 
     (ert-info ("ascii")
       (puthash 'CASEMAPPING  '("ascii") erc--isupport-params)
+      (should (equal (erc-downcase "ABC 123 ΣΛΣΝΟΣ") "abc 123 ΣΛΣΝΟΣ"))
       (should (equal (erc-downcase "Bob[m]`") "bob[m]`"))
       (should (equal (erc-downcase "Tilde~") "tilde~" ))
       (should (equal (erc-downcase "\\O/") "\\o/" )))
 
     (ert-info ("rfc1459")
       (puthash 'CASEMAPPING  '("rfc1459") erc--isupport-params)
+      (should (equal (erc-downcase "ABC 123 ΣΛΣΝΟΣ") "abc 123 ΣΛΣΝΟΣ"))
       (should (equal (erc-downcase "Bob[m]`") "bob{m}`" ))
       (should (equal (erc-downcase "Tilde~") "tilde^" ))
       (should (equal (erc-downcase "\\O/") "|o/" )))
 
     (ert-info ("rfc1459-strict")
       (puthash 'CASEMAPPING  '("rfc1459-strict") erc--isupport-params)
+      (should (equal (erc-downcase "ABC 123 ΣΛΣΝΟΣ") "abc 123 ΣΛΣΝΟΣ"))
       (should (equal (erc-downcase "Bob[m]`") "bob{m}`"))
       (should (equal (erc-downcase "Tilde~") "tilde~" ))
       (should (equal (erc-downcase "\\O/") "|o/" )))))
-- 
2.38.1