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

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

[Óscar Fuentes]

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

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

[Basil L. Contovounesios]

Óscar Fuentes <ofv@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@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

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

[Óscar Fuentes]

"Basil L. Contovounesios" <contovob@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.

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

[Eli Zaretskii]

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

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Mon, 29 Jul 2019 16:12:45 +0200
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 36834@debbugs.gnu.org
> 
> "Basil L. Contovounesios" <contovob@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?

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

[Stefan Monnier]

> 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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 10 Aug 2019 05:14:22 -0400
> Cc: 36834@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.

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

[Basil L. Contovounesios]

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

> "Basil L. Contovounesios" <contovob@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

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

[Óscar Fuentes]

Fixed in f01365f62c921407acead13bb350816a313a8c42