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

Previous Next

Full log

View this message in rfc822 format

From: help-debbugs <at> gnu.org (GNU bug Tracking System)
To: Óscar Fuentes <ofv <at> wanadoo.es>
Subject: bug#36834: closed (Re: bug#36834: 27.0.50; [PATCH]
 password-cache.el: confuses key absence with nil password)
Date: Sun, 11 Aug 2019 23:46:02 +0000

[Message part 1 (text/plain, inline)]

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 36834 <at> debbugs.gnu.org.

-- 
36834: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=36834
GNU Bug Tracking System
Contact help-debbugs <at> gnu.org with problems

[Message part 2 (message/rfc822, inline)]

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

Fixed in f01365f62c921407acead13bb350816a313a8c42

[Message part 3 (message/rfc822, inline)]

From: Óscar Fuentes <ofv <at> wanadoo.es>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil
 password
Date: Mon, 29 Jul 2019 07:12:03 +0200

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 <monnier <at> iro.umontreal.ca>
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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.