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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 36834 in the body.
You can then email your comments to 36834 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

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.

Message #8 received at 36834 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Óscar Fuentes <ofv <at> wanadoo.es>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 36834 <at> debbugs.gnu.org
Subject: Re: bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key
 absence with nil password
Date: Mon, 29 Jul 2019 09:36:17 +0100

Óscar Fuentes <ofv <at> wanadoo.es> writes:

> 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:

(CCing the author.)

> 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.

Is it valid for the password to be nil?  The logic in password-read
suggests otherwise.

> The effect is that every call to password-cache-add with nil as
> password creates a new timer,

Where is password-cache-add being passed a nil password?

> and password-in-cache-p returns nil if
> there exists a (key nil) entry on password-data, when previously it
> would return non-nil.

I think a nil key is also not expected.

> 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))))

Note that password-in-cache-p is currently identical to
password-read-from-cache.  One can probably be written in terms of the
other.

>  (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))

Even if these "memhash" checks are TRT, I suggest either reusing or
copying the hash table method of map-contains-key, rather than comparing
against an interned symbol.

> 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.

I don't either.

Thanks,

-- 
Basil

Message #11 received at 36834 <at> debbugs.gnu.org (full text, mbox):

From: Óscar Fuentes <ofv <at> wanadoo.es>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 36834 <at> debbugs.gnu.org
Subject: Re: bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key
 absence with nil password
Date: Mon, 29 Jul 2019 16:12:45 +0200

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

>> The change uses gethash instead of intern-soft, but those functions act
>> differently when the password (the value associated with the key) was
>> nil.
>
> Is it valid for the password to be nil?  The logic in password-read
> suggests otherwise.

Callers are sending nil. If it is not valid, there is a problem
elsewhere, but my understanding is that a nil password means "no
password" and it is cached in the memoization sense.

>> The effect is that every call to password-cache-add with nil as
>> password creates a new timer,
>
> Where is password-cache-add being passed a nil password?

The caller is auth-source-remember, IIRC, which itself is called from
auth-source-search.

>> and password-in-cache-p returns nil if
>> there exists a (key nil) entry on password-data, when previously it
>> would return non-nil.
>
> I think a nil key is also not expected.

(key nil) means a hash table entry with `key' as key and nil as value,
not that key is nil.

> Note that password-in-cache-p is currently identical to
> password-read-from-cache.  One can probably be written in terms of the
> other.

Yes, right now they are identical, which causes a problem, because
checking for key existence shall not be the same as retrieving the value
when value can be nil.

> Even if these "memhash" checks are TRT, I suggest either reusing or
> copying the hash table method of map-contains-key, rather than comparing
> against an interned symbol.

Is map-contains-key available by default? I'm wary of introducing new
dependencies for saving just a few characters.

Message #14 received at 36834 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Óscar Fuentes <ofv <at> wanadoo.es>
Cc: contovob <at> tcd.ie, monnier <at> iro.umontreal.ca, 36834 <at> debbugs.gnu.org
Subject: Re: bug#36834: 27.0.50;
 [PATCH] password-cache.el: confuses key absence with nil password
Date: Sat, 10 Aug 2019 11:19:01 +0300

Ping!  Any followups?  Stefan?  Should we just push this?

> From: Óscar Fuentes <ofv <at> wanadoo.es>
> Date: Mon, 29 Jul 2019 16:12:45 +0200
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 36834 <at> debbugs.gnu.org
> 
> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
> 
> >> The change uses gethash instead of intern-soft, but those functions act
> >> differently when the password (the value associated with the key) was
> >> nil.
> >
> > Is it valid for the password to be nil?  The logic in password-read
> > suggests otherwise.
> 
> Callers are sending nil. If it is not valid, there is a problem
> elsewhere, but my understanding is that a nil password means "no
> password" and it is cached in the memoization sense.
> 
> >> The effect is that every call to password-cache-add with nil as
> >> password creates a new timer,
> >
> > Where is password-cache-add being passed a nil password?
> 
> The caller is auth-source-remember, IIRC, which itself is called from
> auth-source-search.

Can you show a reproducer?

Message #17 received at 36834 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Óscar Fuentes <ofv <at> wanadoo.es>
Cc: 36834 <at> debbugs.gnu.org
Subject: Re: bug#36834: 27.0.50;
 [PATCH] password-cache.el: confuses key absence with nil password
Date: Sat, 10 Aug 2019 05:14:22 -0400

> 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))

Looks good to me, thanks.

> 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))

Obarrays are just arrays of symbols (i.e. arrays of pointers to symbol
objects), so (fillarray password-data 0) does not overwrite the
passwords stored in the symbols, it just overwrites the pointers to
those symbols.
(clrhash password-data) has basically the same effect.


        Stefan

Message #20 received at 36834 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: ofv <at> wanadoo.es, 36834 <at> debbugs.gnu.org
Subject: Re: bug#36834: 27.0.50;
 [PATCH] password-cache.el: confuses key absence with nil password
Date: Sat, 10 Aug 2019 13:01:18 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Date: Sat, 10 Aug 2019 05:14:22 -0400
> Cc: 36834 <at> debbugs.gnu.org
> 
> > 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))
> 
> Looks good to me, thanks.

Thanks.

Óscar, please push to the emacs-26 branch, and thanks.

Message #23 received at 36834 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Óscar Fuentes <ofv <at> wanadoo.es>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 36834 <at> debbugs.gnu.org
Subject: Re: bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key
 absence with nil password
Date: Sun, 11 Aug 2019 19:00:21 +0300

Óscar Fuentes <ofv <at> wanadoo.es> writes:

> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>
>>> The change uses gethash instead of intern-soft, but those functions act
>>> differently when the password (the value associated with the key) was
>>> nil.
>>
>> Is it valid for the password to be nil?  The logic in password-read
>> suggests otherwise.
>
> Callers are sending nil. If it is not valid, there is a problem
> elsewhere, but my understanding is that a nil password means "no
> password" and it is cached in the memoization sense.

If clients of password-cache.el already rely on this, IWBNI it were
mentioned somewhere.

>>> The effect is that every call to password-cache-add with nil as
>>> password creates a new timer,
>>
>> Where is password-cache-add being passed a nil password?
>
> The caller is auth-source-remember, IIRC, which itself is called from
> auth-source-search.

Indeed, that's what it looks like:

  ;; note we remember the lack of result too, if it's applicable
  (when auth-source-do-cache
    (auth-source-remember spec found)))

>>> and password-in-cache-p returns nil if
>>> there exists a (key nil) entry on password-data, when previously it
>>> would return non-nil.
>>
>> I think a nil key is also not expected.
>
> (key nil) means a hash table entry with `key' as key and nil as value,
> not that key is nil.

Ah, sorry.

>> Note that password-in-cache-p is currently identical to
>> password-read-from-cache.  One can probably be written in terms of the
>> other.
>
> Yes, right now they are identical, which causes a problem, because
> checking for key existence shall not be the same as retrieving the value
> when value can be nil.
>
>> Even if these "memhash" checks are TRT, I suggest either reusing or
>> copying the hash table method of map-contains-key, rather than comparing
>> against an interned symbol.
>
> Is map-contains-key available by default? I'm wary of introducing new
> dependencies for saving just a few characters.

That's why I said "either reusing or _copying_" what map-contains-key
does, namely not using an interned symbol:

  (let ((v '(nil)))
    (not (eq v (gethash key map v))))

But as long as passwords are strings or nil it doesn't really matter.

Thanks,

-- 
Basil

Message #28 received at 36834-done <at> debbugs.gnu.org (full text, mbox):

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

Previous Next

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