From: "Basil L. Contovounesios" <contovob@tcd.ie>
To: "Óscar Fuentes" <ofv@wanadoo.es>
Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 36834@debbugs.gnu.org
Subject: bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
Date: Mon, 29 Jul 2019 09:36:17 +0100 [thread overview]
Message-ID: <87v9vlck5a.fsf@tcd.ie> (raw)
In-Reply-To: <875znltof0.fsf@telefonica.net> ("Óscar Fuentes"'s message of "Mon, 29 Jul 2019 07:12:03 +0200")
Ó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
next prev parent reply other threads:[~2019-07-29 8:36 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-29 5:12 bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password Óscar Fuentes
2019-07-29 8:36 ` Basil L. Contovounesios [this message]
2019-07-29 14:12 ` Óscar Fuentes
2019-08-10 8:19 ` Eli Zaretskii
2019-08-11 16:00 ` Basil L. Contovounesios
2019-08-10 9:14 ` Stefan Monnier
2019-08-10 10:01 ` Eli Zaretskii
2019-08-11 23:45 ` Óscar Fuentes
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87v9vlck5a.fsf@tcd.ie \
--to=contovob@tcd.ie \
--cc=36834@debbugs.gnu.org \
--cc=monnier@iro.umontreal.ca \
--cc=ofv@wanadoo.es \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.