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