Subject: bug#36834: closed (Re: bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password)

From: [email-masked] (GNU bug Tracking System)
Date: Sun, 11 Aug 2019 23:46:02 +0000

Your bug report

#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
36834: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=36834
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
From: Óscar Fuentes <[email-masked]>
To: [email-masked]
Date: Mon, 12 Aug 2019 01:45:38 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

Fixed in f01365f62c921407acead13bb350816a313a8c42

Subject: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
From: Óscar Fuentes <[email-masked]>
To: [email-masked]
Date: Mon, 29 Jul 2019 07:12:03 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

Recently observed that Gnus was creating lots of timers for
password-cache-remove, about 6 every time that it fetched new news/mail.

Upon inspection the cause was found in a change to password-cache.el:

commit d66dcde46a87ee8a9064db3d9b05da9b17036f5b
Author: Stefan Monnier <[email-masked]>
Date:   Fri Jul 28 12:27:00 2017 -0400

    * lisp/password-cache.el (password-data): Use a hash-table
    
    * lisp/auth-source.el (auth-source-magic): Remove.
    (auth-source-forget+, auth-source-forget-all-cached): Adjust to new
    format of password-data.
    (auth-source-format-cache-entry): Just use a cons.
    
    (password-cache-remove, password-cache-add, password-reset)
    (password-read-from-cache, password-in-cache-p): Adjust accordingly.
    
    Fixes: bug#26699

The points of interest of that change are:

 (defun password-in-cache-p (key)
   "Check if KEY is in the cache."
   (and password-cache
        key
-       (intern-soft key password-data)))
+       (gethash key password-data)))

and

 (defun password-cache-add (key password)
   "Add password to cache.
 The password is removed by a timer after `password-cache-expiry' seconds."
-  (when (and password-cache-expiry (null (intern-soft key password-data)))
+  (when (and password-cache-expiry (null (gethash key password-data)))

The change uses gethash instead of intern-soft, but those functions act
differently when the password (the value associated with the key) was
nil. The effect is that every call to password-cache-add with nil as
password creates a new timer, and password-in-cache-p returns nil if
there exists a (key nil) entry on password-data, when previously it
would return non-nil.

So I propose this patch:

diff --git a/lisp/password-cache.el b/lisp/password-cache.el
index 5a09ae4859..6009fb491e 100644
--- a/lisp/password-cache.el
+++ b/lisp/password-cache.el
@@ -81,7 +81,8 @@ password-in-cache-p
   "Check if KEY is in the cache."
   (and password-cache
        key
-       (gethash key password-data)))
+       (not (eq (gethash key password-data 'password-cache-no-data)
+                'password-cache-no-data))))
 
 (defun password-read (prompt &optional key)
   "Read password, for use with KEY, from user, or from cache if wanted.
@@ -125,7 +126,9 @@ password-cache-remove
 (defun password-cache-add (key password)
   "Add password to cache.
 The password is removed by a timer after `password-cache-expiry' seconds."
-  (when (and password-cache-expiry (null (gethash key password-data)))
+  (when (and password-cache-expiry
+             (eq (gethash key password-data 'password-cache-no-data)
+                 'password-cache-no-data))
     (run-at-time password-cache-expiry nil
 		 #'password-cache-remove
 		 key))

Okay to commit? To emacs-26 or master?

On another topic, before a cache entry is removed we try to overwrite
the stored password (see password-cache-remove). However, the same
change did this:

 (defun password-reset ()
   "Clear the password cache."
   (interactive)
-  (fillarray password-data 0))
+  (clrhash password-data))

I don't know if clrhash overwrites the data before releasing it.