bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password

[Basil L. Contovounesios]

Ó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