[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
From: |
Basil L. Contovounesios |
Subject: |
bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password |
Date: |
Sun, 11 Aug 2019 19:00:21 +0300 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) |
Ó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