[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with
Basil L. Contovounesios
bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password
Sun, 11 Aug 2019 19:00:21 +0300
Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux)
Óscar Fuentes <address@hidden> writes:
> "Basil L. Contovounesios" <address@hidden> writes:
>>> The change uses gethash instead of intern-soft, but those functions act
>>> differently when the password (the value associated with the key) was
>> 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
>>> 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
Indeed, that's what it looks like:
;; note we remember the lack of result too, if it's applicable
(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.
>> Note that password-in-cache-p is currently identical to
>> password-read-from-cache. One can probably be written in terms of the
> 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.