[Top][All Lists]

[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 <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
>>> 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]