From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: "Basil L. Contovounesios" Newsgroups: gmane.emacs.bugs Subject: bug#36834: 27.0.50; [PATCH] password-cache.el: confuses key absence with nil password Date: Mon, 29 Jul 2019 09:36:17 +0100 Message-ID: <87v9vlck5a.fsf@tcd.ie> References: <875znltof0.fsf@telefonica.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="170460"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) Cc: Stefan Monnier , 36834@debbugs.gnu.org To: =?UTF-8?Q?=C3=93scar?= Fuentes Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Jul 29 10:37:23 2019 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1hs19m-000iB3-B1 for geb-bug-gnu-emacs@m.gmane.org; Mon, 29 Jul 2019 10:37:22 +0200 Original-Received: from localhost ([::1]:50494 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hs19l-0000ZW-B6 for geb-bug-gnu-emacs@m.gmane.org; Mon, 29 Jul 2019 04:37:21 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:48706) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hs19U-0000X4-F1 for bug-gnu-emacs@gnu.org; Mon, 29 Jul 2019 04:37:05 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hs19T-0006Ko-AL for bug-gnu-emacs@gnu.org; Mon, 29 Jul 2019 04:37:04 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:38827) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hs19S-0006JM-EU for bug-gnu-emacs@gnu.org; Mon, 29 Jul 2019 04:37:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1hs19S-0005Xo-9J for bug-gnu-emacs@gnu.org; Mon, 29 Jul 2019 04:37:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: "Basil L. Contovounesios" Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 29 Jul 2019 08:37:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 36834 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 36834-submit@debbugs.gnu.org id=B36834.156438939121264 (code B ref 36834); Mon, 29 Jul 2019 08:37:02 +0000 Original-Received: (at 36834) by debbugs.gnu.org; 29 Jul 2019 08:36:31 +0000 Original-Received: from localhost ([127.0.0.1]:47648 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hs18w-0005Wu-LK for submit@debbugs.gnu.org; Mon, 29 Jul 2019 04:36:31 -0400 Original-Received: from mail-wm1-f45.google.com ([209.85.128.45]:38870) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1hs18t-0005Wd-CE for 36834@debbugs.gnu.org; Mon, 29 Jul 2019 04:36:28 -0400 Original-Received: by mail-wm1-f45.google.com with SMTP id s15so31282727wmj.3 for <36834@debbugs.gnu.org>; Mon, 29 Jul 2019 01:36:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tcd-ie.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version:content-transfer-encoding; bh=2br9Q1+m8BzhCbhD9d7xqNxxaLYP7gTiT04i2aZvYSc=; b=do2WvIHRk62YIV/tH/DYlrtZpmRjkURDnMXxzYjwrRWXR3D51eYtu8oKQHKZ7mqQsX UutedPPOdjw43wOFgAOi7ibKJ1dpzc1Dmfz9BAaFSI5cgD/sHg0oFv20i25tVe0sHjjy ec+k5oxtD2dRhRIsdelY9raw2okYT/HGJyiOj8at0XLEhf7TVhJP3fRBZTOOC0nJ1GfJ 5vgeGrSFoHe3p1F5s5a6w1FKuDiQPG7b4nQRf/1Qc1tfmv0RUxglcjViNgdqqY+CRKKz EucRTjYW1OER3bDUiDlsHniIxbWR60geyyRxJAEDoetdMBPopvEpxiSqf7uq1VG7n+p4 YTyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=2br9Q1+m8BzhCbhD9d7xqNxxaLYP7gTiT04i2aZvYSc=; b=EjP7IB69IEKgagadWafygJ2KyJdHmrQmPx99AmLvQIugOaCX0nWm/00DzdufG2K8aj tbWisLp01Cq5lv4pzNdGBCCHN3Ad6BCHrrxYnjRGZWz/oaqRF2zqCjXVq0aaDjE7wuIB lFoI7huGzngNprLEyIl7MVmjF0fk/R+PNn6+LsvHmOIz9rIkMU90BDEoVzslHp378ucG 703E/jeQPpnxRVlDwUg6QCy4t4oX5khIG+4JdB/wJFFf5/r+TosP6xRKMzz5hk+kMTz7 DYn8F/llEXgkw7qXLBm0I9udOjb0KkIC9rZ565CK2a4txXeIxCrx8tbGDyMLcKkO4UIm 86EQ== X-Gm-Message-State: APjAAAW4k4oJTuCQ1MPIwQmui5hD19gsbw88/d2iizVD3Zi6j0dcy/qf nc+EmedYfa5PLG+fxNvgoT5TJg== X-Google-Smtp-Source: APXvYqznIp279Xu72azxL1CS7uc78D51+KlngvGWN0EA+BjVip+oIF+pR4eHVhIoGsykxd7651pMPw== X-Received: by 2002:a05:600c:c4:: with SMTP id u4mr98367594wmm.96.1564389380845; Mon, 29 Jul 2019 01:36:20 -0700 (PDT) Original-Received: from localhost ([88.128.80.170]) by smtp.gmail.com with ESMTPSA id c30sm115148739wrb.15.2019.07.29.01.36.19 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Mon, 29 Jul 2019 01:36:20 -0700 (PDT) In-Reply-To: <875znltof0.fsf@telefonica.net> ("=?UTF-8?Q?=C3=93scar?= Fuentes"'s message of "Mon, 29 Jul 2019 07:12:03 +0200") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.51.188.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:164010 Archived-At: =C3=93scar Fuentes 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 > Date: Fri Jul 28 12:27:00 2017 -0400 > > * lisp/password-cache.el (password-data): Use a hash-table >=20=20=20=20=20 > * 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. >=20=20=20=20=20 > (password-cache-remove, password-cache-add, password-reset) > (password-read-from-cache, password-in-cache-p): Adjust accordingly. >=20=20=20=20=20 > 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, --=20 Basil