Error on `copy-password`

[hsanjuan]

Describe the bug

Copy password does not work sometimes.

Precise recipe to reproduce the issue

Call copy-password.

Output when started from a shell

<ERROR> [14:07:44] next utility.lisp (funcall-safely fun3) -
  Error in #<FUNCTION NEXT:RETURN-INPUT>: Subprocess #<UIOP/LAUNCH-PROGRAM::PROCESS-INFO {10083E07B3}>
 with command ("/usr/bin/xclip" "-out" "-selection" "clipboard")
 exited with error code 1

So something went bad with xclip but not sure what. Is there a way I can debug this more? Calling pass manually works. Also, is this not calling pass -c directly to let pass put the things in the clipboard?

[Ambrevar]

You are the second one to report this issue (first one was in the C-v thread).

To use the clipboard we depend on the trivial-clipboard library. Maybe something fishy is happening there, but I can't figure out what at first glance.

https://github.com/snmsts/trivial-clipboard

[hsanjuan]

Perhaps it is not passing anything in?

[Ambrevar]

Good point. I'll make some tests.

[Ambrevar]

To clarify, did you choose a password entry and press RET to trigger the error?

Since March 9th, commands are called with funcall-safely when run from the binary. Are you running Next from the REPL?

[hsanjuan]

To clarify, did you choose a password entry and press RET to trigger the error?

Yes

Since March 9th, commands are called with funcall-safely when run from the binary. Are you running Next from the REPL?

No.

I think what I need is to trace what gets called for pass and what returns, but the problem is this doesn't happen consistently, but once it happens on a buffer I think it keeps happening at least for that same password. I can also add that it does not seem to be funny characters in the password, or in the usernames.

[Ambrevar]

When the issue happens, does it hang the browser or does everything keeps going as usual?

[hsanjuan]

Things keep going as usual

[Ambrevar]

To answer your question: if you start the browser from the REPL or if you set keep-alive to T in your config, you'll see more extensive stacktraces on error.

The downside is that this hangs the browser until you've restarted the condition. This should allow you to investigate.

I haven't been able to reproduce the error so far :(

[hsanjuan]

I am very stupid but I am guessing that (setq *keep-alive* t) is NOT the way to set this in my config?

[jmercouris]

You are not stupid! We are all learning all the time :-)

actually you are quite close! (setf *keep-alive* t)

it already has a default value of t though. You'll basically just want to open up a terminal and start Next so you can see the error messages in stdout rather than starting it from whatever GUI tool

[jmercouris]

I see now that it is set to nil by the startup procedure, so actually yes, setting it to t in your init file would help

[hsanjuan]

; file: /home/hector/.config/next/init.lisp                                                                                               
; in: SETF *KEEP-ALIVE*                                                                                                                   
;     (SETF NEXT-USER::*KEEP-ALIVE* T)                                                                                                    
; ==>                                                                                                                       
;   (SETQ NEXT-USER::*KEEP-ALIVE* T)                                                                                                      
;                                                                                                               
; caught WARNING:                                                                                                                         
;   undefined variable: NEXT-USER::*KEEP-ALIVE*                                                                                           
;                                                                                                               
; compilation unit finished                                                                                                               
;   Undefined variable:                                                                                                                   
;     *KEEP-ALIVE*   

doesn't really let me setf either. also errors when (in-package :next).

[Ambrevar]

a few corrections:

(setq keep-alive t)

is perfectly valid. in fact, setf expands to setq when given a variable. For consistency, we prefer to use setf all the time.

héctor: set it to nil if you want to see a stack trace in the shell.

(setf next::*keep-alive* nil)

We would need to change the name of the variable because it's currently confusing.

It could also be set to NIL with a --debug command line option.

Thoughts?

[jmercouris]

Pierre is correct about setq, unlike in elisp it should however be avoided

I also think that keep-alive is confusing, I read the doc string and still got it backwards... Maybe we can have a config that will set logging , stacktrace etc? This config could be set by a command line argument, I’m thinking something like how ENVY looks but using command line flags instead of environment variables?

[hsanjuan]

Ok, with keep-alive set to nil:

<ERROR> [12:19:42] next utility.lisp (funcall-safely fun3) -
  In #<FUNCTION (LAMBDA (NEXT::PASSWORD-NAME)
                  :IN
                  NEXT:COPY-PASSWORD) {527B9F5B}>: Subprocess #<UIOP/LAUNCH-PROGRAM:FE0883}>
 with command ("/usr/bin/xclip" "-out" "-selection" "clipboard")
 exited with error code 1

It does not give much more information. One thing I realized is taht we are not trying to copy something into the clipboard, but rather to read it out (-out). It's that what we want?

The library you linked (if I'm reading that lisp well), apparently uses the xclip -out command when the data passed is NIL.

This would somehow mean that we failed to read the password from pass. Can we capture stderr from pass? Are we catching if pass throws an error? I wonder if we should just call pass -c0 and forget about clipboard management and line splitting.

[Ambrevar]

Hmmm, I can't reproduce your issue from the REPL :(

The culprit function is clip-password-string in libraries/password-manager/password.lisp.

Here it is for your convenience:

(defun clip-password-string (pass)
  (let ((original-clipboard (trivial-clipboard:text)))
    (trivial-clipboard:text pass)
    (bt:make-thread
     (lambda ()
       (sleep *sleep-timer*)
       (when (string= (trivial-clipboard:text) pass)
         (trivial-clipboard:text original-clipboard))))))

The offending call is (trivial-clipboard:text) (the first one).

I don't understand how this could fail though.

@hsanjuan: I suppose the consequence is that you can't copy the password into your clipboard, right? What if you wrap the offending call in (ignore-errors ...)? Like so:

(defun clip-password-string (pass)
  (let ((original-clipboard (ignore-errors (trivial-clipboard:text))))
    (trivial-clipboard:text pass)
    (bt:make-thread
     (lambda ()
       (sleep *sleep-timer*)
       (when (string= (ignore-errors (trivial-clipboard:text)) pass)
         (trivial-clipboard:text original-clipboard))))))

Does it solve your issue?

[hsanjuan]

I did not have time to try but I think you changed something?

Anyways, I caught an xclip error by running the xclip command directly right after the error in next:

[10:43:12] ~ $ xclip -out -selection clipboard
Error: target STRING not available⏎ 

This returned with an error status. Per https://github.com/astrand/xclip/issues/38#issuecomment-466625564 this happens when the clipboard contains something non-text. I am not sure how the clipboard ends up containing something non-text though. In fact, if I add urandom things to the clipboard it still works.

Could it be next somehow mangling things when it restores the "original" clipboard so that xclip becomes unusable on the next occasion?

[Ambrevar]

Thanks, this makes sense.

The clipboard can indeed contain pictures for instance (I also learned that recently :p).

The non-text detect of xclip might be based on the frequency of unprintable characters in some encoding. If it uses an 8-bit encoding, then only some 30-ish/256 characters are unprintable, which makes for a low probability to detect non-strings. I'm just making a wild guess here.

I'll push a fix just now. Thanks a lot for investigating.

[Ambrevar]

Fixed in 3020c4d6495d4e72c9c79cd33835f0df78e1efb9.

[hsanjuan]

@Ambrevar on a side note, should it just clear the clipboard (rather than try to restore?).

Normally, if I put something in the clipboard, I would expect to be overwriting the previous thing. Restoring it strikes me as a bit unnatural. If fact sometimes leads me to paste wrong things because I am expecting to paste the password in a password field, but it expired and now I am pasting something else that was in the clipboard before (maybe another password), and now I am sending the wrong thing to some server, which is quite scary...

[jmercouris]

I believe that we should not use the clipboard for this and should directly output into the text area, I’ll provide an alternate command for this

[Ambrevar]

@Ambrevar on a side note, should it just clear the clipboard (rather than try to restore?).

Normally, if I put something in the clipboard, I would expect to be overwriting the previous thing. Restoring it strikes me as a bit unnatural. If fact sometimes leads me to paste wrong things because I am expecting to paste the password in a password field, but it expired and now I am pasting something else that was in the clipboard before (maybe another password), and now I am sending the wrong thing to some server, which is quite scary...

Good point. Fixed in 65d0974b9455c9a6da4a5dfca8aea83acbeff8c0.