Akib Azmain Turja <akib@disroot.org> writes:

> Why the closure doesn't capture "s"?  For me, the following code
> captures "s" (obviously with lexical binding): (just let-wrapped version
> of your code)
>
> (let ((e '(:secret "topsecret")))
>   (when-let* ((s (plist-get e :secret)) ; s not captured by closure
>               (v (auth-source--obfuscate s)))
>     (setf (plist-get e :secret)
>           (lambda () (auth-source--deobfuscate v))))
>   e)
> ;; => (:secret
> ;;     (closure
> ;;         ((p #1)
> ;;          (v . "XIcHKKIKtavKgK8J6zXP1w==-N/XAaAOqAtGcCzKGKX71og==")
> ;;          (s . "topsecret") ;; LEAKED!!!
> ;;          (e :secret #1)
> ;;          t)
> ;;         nil
> ;;       (auth-source--deobfuscate v)))
>

Looks like you don't have:

  commit 1b1ffe07897ebe06cf96ab423fad3cde9fd6c981
  Author: Stefan Monnier <monnier@iro.umontreal.ca>
  Date:   Mon Oct 17 17:11:40 2022 -0400
  
      (Ffunction): Make interpreted closures safe for space
    
It's easiest to just make a habit of applying patches on the latest
HEAD. Once you do, you'll find that the output of your example changes.
If ELPA's Compat ever takes an interest, I suppose a backported version
could just `byte-compile' the lambda.

>> +        (push e out)))))
>
> [...]
>
>> +(defun auth-source-pass--retrieve-parsed (seen path port-number-p)
>> +  (when-let ((m (string-match auth-source-pass--match-regexp path)))
>
> Why do you let-bound "m"?

Because I am slow and blind, I guess.

>  I can't find any use of it in the body.

Go figure. (Thanks.)

>> +(defun auth-source-pass--find-match-many (hosts users ports require max)
>> +  "Return plists for valid combinations of HOSTS, USERS, PORTS.
>> +Each plist contains, at the very least, a host and a secret."
>> +  (let ((seen (make-hash-table :test #'equal))
>> +        (entries (auth-source-pass-entries))
>> +        out)
>> +    (catch 'done
>> +      (dolist (host hosts out)
>> +        (pcase-let ((`(,_ ,u ,p) (auth-source-pass--disambiguate host)))
>> +          (unless (or (not (equal "443" p)) (string-prefix-p "https://" host))
>> +            (setq p nil))
>> +          (dolist (user (or users (list u)))
>> +            (dolist (port (or ports (list p)))
>> +              (dolist (e entries)
>> +                (when-let*
>> +                    ((m (or (gethash e seen) (auth-source-pass--retrieve-parsed
>> +                                              seen e (integerp port))))
>> +                     ((equal host (plist-get m :host)))
>> +                     ((auth-source-pass--match-parts m :port port require))
>> +                     ((auth-source-pass--match-parts m :user user require))
>> +                     (parsed (auth-source-pass-parse-entry e))
>> +                     ;; For now, ignore body-content pairs, if any,
>> +                     ;; from `auth-source-pass--parse-data'.
>> +                     (secret (or (auth-source-pass--get-attr 'secret parsed)
>> +                                 (not (memq :secret require)))))
>> +                  (push
>> +                   `( :host ,host ; prefer user-provided :host over h
>> +                      ,@(and-let* ((u (plist-get m :user))) (list :user u))
>> +                      ,@(and-let* ((p (plist-get m :port))) (list :port p))
>> +                      ,@(and secret (not (eq secret t)) (list :secret secret)))
>> +                   out)
>> +                  (when (or (zerop (cl-decf max))
>> +                            (null (setq entries (remove e entries))))
>
> Remove will create a lot of garbage, e.g. (let ((x '(1 2 3 4 5)))
> (eq (remove 6 x) x)) and (let ((x '(1 2 3 4 5))) (eq (remove 1 x)
> (cdr x))) both returns nil.

Since you're clearly aware that, for lists, `remove' just calls `delete'
on a shallow copy, how could (remove thing x) ever be eq to some nthcdr
of x so long as both are non-nil?

> If you think delete is OK, go ahead and use it.  If you think remove is
> better, keep it.  Do whatever you think right.

As I tried to explain in

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=58985#64

I think `delete' is safe in this situation, assuming of course that, for
ancient, core functions, the implementation can be construed as the de
facto interface. Based on your comments, you seem to agree with this
assumption, which seems only sane. I have thus reverted the change.

>
>> +                    (throw 'done out)))))))))))
>> +
>
> [...]

While I certainly welcome the assiduous scrutinizing of Emacs lisp
mechanics and technique (truly), I was mainly hoping that, as an avid
pass user, you would also help flesh out the precise effects of the
behavior introduced by these changes and hopefully share some insights
into how they might impact day-to-day usage for the typical pass user.
Granted, that necessarily involves applying these patches atop your
daily driver and living with them for a spell and, ideally, investing
some thought into imagining common usage patterns beyond your own (plus
any potentially problematic edge cases). If you have the energy to
devote to (perhaps just some of) these areas, it would really help move
this bug report forward. Thanks.