Re: master 3139551: Don’t attempt to modify constant strings

Re: master 3139551: Don’t attempt to modify constant strings

[Dmitry Gutov]

On 17.05.2020 08:25, Paul Eggert wrote:
>   ;; WORKAROUND: This is nominally a constant, but the text properties
>   ;; are not preserved thru dump if use defconst.  See bug#21237.
>   (defvar elisp--xref-format
> -  (let ((str "(%s %s)"))
> -    (put-text-property 1 3 'face 'font-lock-keyword-face str)
> -    (put-text-property 4 6 'face 'font-lock-function-name-face str)
> -    str))
> +  #("(%s %s)"
> +    1 3 (face font-lock-keyword-face)
> +    4 6 (face font-lock-function-name-face)))

Seems like the comment is calling for some further changes.

Re: master 3139551: Don’t attempt to modify constant strings

[Dmitry Gutov]

On 17.05.2020 08:25, Paul Eggert wrote:
>   ;;
> -;; (password-cache-add "test" "foo")
> +;; (password-cache-add "test" (copy-sequence "foo"))
>   ;;  => nil

This seems like a false positive.

Re: master 3139551: Don’t attempt to modify constant strings

[Paul Eggert]

On 5/17/20 4:22 AM, Dmitry Gutov wrote:
> On 17.05.2020 08:25, Paul Eggert wrote:
>>   ;; WORKAROUND: This is nominally a constant, but the text properties
>>   ;; are not preserved thru dump if use defconst.  See bug#21237.

> Seems like the comment is calling for some further changes.

Yes, quite true. That's on my todo list, though it may need to wait until we
start insisting on portable rather than traditional dumping.

Re: master 3139551: Don’t attempt to modify constant strings

[Paul Eggert]

On 5/17/20 4:24 AM, Dmitry Gutov wrote:
>> -;; (password-cache-add "test" "foo")
>> +;; (password-cache-add "test" (copy-sequence "foo"))
>>   ;;  => nil
> 
> This seems like a false positive.

It's just a comment. Since there are so many ways that a password cache can be
cleared and clearing means the strings holding passwords need to be mutable, I
thought it better for the comment to reflect good practice.

Re: master 3139551: Don’t attempt to modify constant strings

[Dmitry Gutov]

On 17.05.2020 20:10, Paul Eggert wrote:
> Since there are so many ways that a password cache can be
> cleared and clearing means the strings holding passwords need to be mutable,

Um, why?

A cache normally doesn't mutate the values, it only holds on to them.

Re: master 3139551: Don’t attempt to modify constant strings

[Paul Eggert]

On 5/17/20 11:02 AM, Dmitry Gutov wrote:
> On 17.05.2020 20:10, Paul Eggert wrote:
>> Since there are so many ways that a password cache can be
>> cleared and clearing means the strings holding passwords need to be mutable,
> 
> Um, why?

It's done under the assumption that it makes Emacs more "secure".

> A cache normally doesn't mutate the values, it only holds on to them.

This cache mutates its values, unfortunately.

Re: master 3139551: Don’t attempt to modify constant strings

[Dmitry Gutov]

On 18.05.2020 00:36, Paul Eggert wrote:
>> A cache normally doesn't mutate the values, it only holds on to them.
> This cache mutates its values, unfortunately.

Perhaps it should call (copy-sequence val) itself, then? That would be a 
good development practice.

Re: master 3139551: Don’t attempt to modify constant strings

[Paul Eggert]

On 5/17/20 2:54 PM, Dmitry Gutov wrote:
> On 18.05.2020 00:36, Paul Eggert wrote:
>>> A cache normally doesn't mutate the values, it only holds on to them.
>> This cache mutates its values, unfortunately.
> 
> Perhaps it should call (copy-sequence val) itself, then? That would be a good
> development practice.

That would run afoul of the whole idea of clearing the string in the first
place, which was to avoid having one's password lying around in memory for a bad
actor to grab it.

Re: master 3139551: Don’t attempt to modify constant strings

[Dmitry Gutov]

On 18.05.2020 02:16, Paul Eggert wrote:
> That would run afoul of the whole idea of clearing the string in the first
> place, which was to avoid having one's password lying around in memory for a bad
> actor to grab it.

OK, okay. I see the full design now.

Sorry for the false alarm.

Re: master 3139551: Don’t attempt to modify constant strings

[Andreas Schwab]

On Mai 17 2020, Paul Eggert wrote:

> That would run afoul of the whole idea of clearing the string in the first
> place, which was to avoid having one's password lying around in memory for a bad
> actor to grab it.

But doing it in the caller is equally bad.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: master 3139551: Don’t attempt to modify constant strings

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On 5/18/20 12:03 AM, Andreas Schwab wrote:
> On Mai 17 2020, Paul Eggert wrote:
> 
>> That would run afoul of the whole idea of clearing the string in the first
>> place, which was to avoid having one's password lying around in memory for a bad
>> actor to grab it.
> 
> But doing it in the caller is equally bad.

True, thanks, I installed the attached.

[-- Attachment #2: 0001-Improve-password-cache-add-example-in-comment.patch --]
[-- Type: text/x-patch, Size: 895 bytes --]

From 4d6e592cf5f48ca8aab9ec6569759f25cd1d1677 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 18 May 2020 15:42:18 -0700
Subject: [PATCH] Improve password-cache-add example in comment

* lisp/password-cache.el: Improve comment.  See Andreas Schwab in:
https://lists.gnu.org/r/emacs-devel/2020-05/msg02422.html
---
 lisp/password-cache.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/password-cache.el b/lisp/password-cache.el
index 86d802f283..f5007579a8 100644
--- a/lisp/password-cache.el
+++ b/lisp/password-cache.el
@@ -31,7 +31,8 @@
 ;; ;; Minibuffer prompt for password.
 ;;  => "foo"
 ;;
-;; (password-cache-add "test" (copy-sequence "foo"))
+;; (password-cache-add "test" (read-passwd "Password? "))
+;; ;; Minibuffer prompt from read-passwd, which returns "foo".
 ;;  => nil
 
 ;; (password-read "Password? " "test")
-- 
2.25.4