* bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
@ 2019-07-29 5:12 Óscar Fuentes
2019-07-29 8:36 ` Basil L. Contovounesios
2019-08-10 9:14 ` Stefan Monnier
0 siblings, 2 replies; 8+ messages in thread
From: Óscar Fuentes @ 2019-07-29 5:12 UTC (permalink / raw)
To: 36834
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.
^ permalink raw reply related [flat|nested] 8+ messages in thread* bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
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
2019-07-29 14:12 ` Óscar Fuentes
2019-08-10 9:14 ` Stefan Monnier
1 sibling, 1 reply; 8+ messages in thread
From: Basil L. Contovounesios @ 2019-07-29 8:36 UTC (permalink / raw)
To: Óscar Fuentes; +Cc: Stefan Monnier, 36834
Ó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
^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
2019-07-29 8:36 ` Basil L. Contovounesios
@ 2019-07-29 14:12 ` Óscar Fuentes
2019-08-10 8:19 ` Eli Zaretskii
2019-08-11 16:00 ` Basil L. Contovounesios
0 siblings, 2 replies; 8+ messages in thread
From: Óscar Fuentes @ 2019-07-29 14:12 UTC (permalink / raw)
To: Basil L. Contovounesios; +Cc: Stefan Monnier, 36834
"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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
2019-07-29 14:12 ` Óscar Fuentes
@ 2019-08-10 8:19 ` Eli Zaretskii
2019-08-11 16:00 ` Basil L. Contovounesios
1 sibling, 0 replies; 8+ messages in thread
From: Eli Zaretskii @ 2019-08-10 8:19 UTC (permalink / raw)
To: Óscar Fuentes; +Cc: contovob, monnier, 36834
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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
2019-07-29 14:12 ` Óscar Fuentes
2019-08-10 8:19 ` Eli Zaretskii
@ 2019-08-11 16:00 ` Basil L. Contovounesios
1 sibling, 0 replies; 8+ messages in thread
From: Basil L. Contovounesios @ 2019-08-11 16:00 UTC (permalink / raw)
To: Óscar Fuentes; +Cc: Stefan Monnier, 36834
Ó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
^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
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
@ 2019-08-10 9:14 ` Stefan Monnier
2019-08-10 10:01 ` Eli Zaretskii
1 sibling, 1 reply; 8+ messages in thread
From: Stefan Monnier @ 2019-08-10 9:14 UTC (permalink / raw)
To: Óscar Fuentes; +Cc: 36834
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread* bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
2019-08-10 9:14 ` Stefan Monnier
@ 2019-08-10 10:01 ` Eli Zaretskii
2019-08-11 23:45 ` Óscar Fuentes
0 siblings, 1 reply; 8+ messages in thread
From: Eli Zaretskii @ 2019-08-10 10:01 UTC (permalink / raw)
To: Stefan Monnier; +Cc: ofv, 36834
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-08-11 23:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).