Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forbidden reentrant call of Tramp when creating a signed commit #5006

Open
teythoon opened this issue Sep 12, 2023 · 6 comments
Open

Forbidden reentrant call of Tramp when creating a signed commit #5006

teythoon opened this issue Sep 12, 2023 · 6 comments
Labels
area: tramp bug unconfirmed Maybe a bug, but unable to reproduce

Comments

@teythoon
Copy link

  1. Expected behavior:

When visiting a remote file over Tramp, Magit should create a commit without any issues when the remote git has commit signing turned on (commit.gpgsign = true), and the remote system has the required key material and GnuPG setup to create the signature.

  1. Observed behavior:

Magit from Debian Bookworm (2.99.0.git0957.ge8c7bd03-1) fails to create a commit at all. No error is displayed in the minibuffer or magit log buffer. The changes are still staged.

I then installed magit from git, and started it using make emacs-Q, loaded Tramp, increased Tramp's verbosity to 6, enabled trapping into the debugger using (setq debug-on-error t debug-on-signal t), and reproduced the issue. With Magit from git, the commit is created, but due to the error shown below, Magit's status buffer is not refreshed.

Magit version: Magit v3.3.0-757-g7a1d5034, Transient 0.4.3, Git 2.39.2, Emacs 28.2, gnu/linux

Stack trace:

Debugger entered--Lisp error: (remote-file-error "Forbidden reentrant call of Tramp")
  signal(remote-file-error ("Forbidden reentrant call of Tramp"))
  tramp-error(#<process *tramp/ssh europ.nat*> remote-file-error "Forbidden reentrant call of Tramp")
  tramp-send-string((tramp-file-name "ssh" nil nil "europ.nat" nil "/tmp/a-test-repo" nil) "test -d /tmp/a-test-repo 2>/dev/null; echo tramp_e...")
  tramp-send-command((tramp-file-name "ssh" nil nil "europ.nat" nil "/tmp/a-test-repo" nil) "test -d /tmp/a-test-repo 2>/dev/null; echo tramp_e...")
  tramp-send-command-and-check((tramp-file-name "ssh" nil nil "europ.nat" nil "/tmp/a-test-repo" nil) "test -d /tmp/a-test-repo")
  tramp-run-test("-d" "/ssh:europ.nat:/tmp/a-test-repo")
  tramp-sh-handle-file-directory-p("/ssh:europ.nat:/tmp/a-test-repo")
  tramp-sh-file-name-handler(file-directory-p "/ssh:europ.nat:/tmp/a-test-repo")
  apply(tramp-sh-file-name-handler file-directory-p "/ssh:europ.nat:/tmp/a-test-repo")
  tramp-file-name-handler(file-directory-p "/ssh:europ.nat:/tmp/a-test-repo")
  tramp-handle-dired-uncache("/ssh:europ.nat:/tmp/a-test-repo/")
  tramp-sh-file-name-handler(dired-uncache "/ssh:europ.nat:/tmp/a-test-repo/")
  apply(tramp-sh-file-name-handler dired-uncache "/ssh:europ.nat:/tmp/a-test-repo/")
  tramp-file-name-handler(dired-uncache "/ssh:europ.nat:/tmp/a-test-repo/")
  dired-uncache("/ssh:europ.nat:/tmp/a-test-repo/")
  (progn (dired-uncache default-dir))
  (if (fboundp 'dired-uncache) (progn (dired-uncache default-dir)))
  magit-process-finish(#<process git>)
  (progn (setq event (substring event 0 -1)) (if (string-match "^finished" event) (progn (message (concat (capitalize (process-name process)) " finished")))) (magit-process-finish process) (if (eq process magit-this-process) (progn (setq magit-this-process nil))) (if (process-get process 'inhibit-refresh) nil (let ((command-buf (process-get process 'command-buf))) (if (buffer-live-p command-buf) (save-current-buffer (set-buffer command-buf) (magit-refresh)) (let ((temp-buffer (generate-new-buffer " *temp*" t))) (save-current-buffer (set-buffer temp-buffer) (unwind-protect (progn ... ...) (and ... ...))))))))
  (if (memq (process-status process) '(exit signal)) (progn (setq event (substring event 0 -1)) (if (string-match "^finished" event) (progn (message (concat (capitalize (process-name process)) " finished")))) (magit-process-finish process) (if (eq process magit-this-process) (progn (setq magit-this-process nil))) (if (process-get process 'inhibit-refresh) nil (let ((command-buf (process-get process 'command-buf))) (if (buffer-live-p command-buf) (save-current-buffer (set-buffer command-buf) (magit-refresh)) (let ((temp-buffer ...)) (save-current-buffer (set-buffer temp-buffer) (unwind-protect ... ...))))))))
  magit-process-sentinel(#<process git> "finished\n")
  accept-process-output(#<process *tramp/ssh europ.nat*> nil nil t)
  tramp-accept-process-output(#<process *tramp/ssh europ.nat*>)
  tramp-wait-for-regexp(#<process *tramp/ssh europ.nat*> nil "\\(^\\|\0\\)[^#$\n]*///0c705e19c85cea87304133560ac40b2f...")
  tramp-wait-for-output(#<process *tramp/ssh europ.nat*>)
  tramp-send-command((tramp-file-name "ssh" nil nil "europ.nat" nil "/tmp/a-test-repo/.git/" nil) "(env QUOTING_STYLE=locale \\stat -c '((/////%N/////...")
  tramp-send-command-and-check((tramp-file-name "ssh" nil nil "europ.nat" nil "/tmp/a-test-repo/.git/" nil) "(env QUOTING_STYLE=locale \\stat -c '((/////%N/////...")
  tramp-send-command-and-read((tramp-file-name "ssh" nil nil "europ.nat" nil "/tmp/a-test-repo/.git/" nil) "(env QUOTING_STYLE=locale \\stat -c '((/////%N/////..." noerror)
  tramp-do-file-attributes-with-stat((tramp-file-name "ssh" nil nil "europ.nat" nil "/tmp/a-test-repo/.git/" nil) "/tmp/a-test-repo/.git/" string)
  tramp-sh-handle-file-attributes("/ssh:europ.nat:/tmp/a-test-repo/.git/" string)
  tramp-sh-file-name-handler(file-attributes "/ssh:europ.nat:/tmp/a-test-repo/.git/" string)
  apply(tramp-sh-file-name-handler file-attributes ("/ssh:europ.nat:/tmp/a-test-repo/.git/" string))
  tramp-file-name-handler(file-attributes "/ssh:europ.nat:/tmp/a-test-repo/.git/" string)
  tramp-check-cached-permissions((tramp-file-name "ssh" nil nil "europ.nat" nil "/tmp/a-test-repo/.git/" nil) 114)
  tramp-handle-file-readable-p("/ssh:europ.nat:/tmp/a-test-repo/.git/")
  tramp-sh-handle-file-readable-p("/ssh:europ.nat:/tmp/a-test-repo/.git/")
  tramp-sh-file-name-handler(file-readable-p "/ssh:europ.nat:/tmp/a-test-repo/.git/")
  apply(tramp-sh-file-name-handler file-readable-p "/ssh:europ.nat:/tmp/a-test-repo/.git/")
  tramp-file-name-handler(file-readable-p "/ssh:europ.nat:/tmp/a-test-repo/.git/")
  tramp-handle-file-accessible-directory-p("/ssh:europ.nat:/tmp/a-test-repo/.git/")
  tramp-sh-file-name-handler(file-accessible-directory-p "/ssh:europ.nat:/tmp/a-test-repo/.git/")
  apply(tramp-sh-file-name-handler file-accessible-directory-p "/ssh:europ.nat:/tmp/a-test-repo/.git/")
  tramp-file-name-handler(file-accessible-directory-p "/ssh:europ.nat:/tmp/a-test-repo/.git/")
  file-accessible-directory-p("/ssh:europ.nat:/tmp/a-test-repo/.git/")
  (and (file-directory-p filename) (file-accessible-directory-p filename))
  magit-file-accessible-directory-p("/ssh:europ.nat:/tmp/a-test-repo/.git/")
  (not (magit-file-accessible-directory-p dir))
  (while (not (magit-file-accessible-directory-p dir)) (setq dir (file-name-directory (directory-file-name dir))) (if (equal dir previous) (progn (throw 'unsafe-default-dir nil))) (setq previous dir))
  (let ((dir (file-name-as-directory (expand-file-name (or file default-directory)))) (previous nil)) (while (not (magit-file-accessible-directory-p dir)) (setq dir (file-name-directory (directory-file-name dir))) (if (equal dir previous) (progn (throw 'unsafe-default-dir nil))) (setq previous dir)) dir)
  (catch 'unsafe-default-dir (let ((dir (file-name-as-directory (expand-file-name (or file default-directory)))) (previous nil)) (while (not (magit-file-accessible-directory-p dir)) (setq dir (file-name-directory (directory-file-name dir))) (if (equal dir previous) (progn (throw 'unsafe-default-dir nil))) (setq previous dir)) dir))
  magit--safe-default-directory(nil)
  (and t (magit--safe-default-directory directory))
  (let* ((default-directory (and t (magit--safe-default-directory directory)))) (if default-directory (let* ((topdir (and t (magit-rev-parse-safe "--show-toplevel")))) (if topdir (let (updir) (setq topdir (magit-expand-git-file-name topdir)) (cond ((and ... ... ...) updir) ((concat ... ...)))) (let* ((gitdir (and t ...)) (gitdir (and gitdir ...))) (if gitdir (progn (if ... gitdir ...)))))) nil))
  (if magit--refresh-cache (let ((G35 (cons (or directory default-directory) 'magit-toplevel))) (let* ((G36 (and t (assoc G35 (cdr magit--refresh-cache))))) (if G36 (progn (let* ((v magit--refresh-cache)) (setcar (car v) (+ ... 1))) (cdr G36)) (progn (let* ((v magit--refresh-cache)) (setcdr (car v) (+ ... 1))) (let ((value ...)) (let* (... ...) (setcdr v ...)) value))))) (let* ((default-directory (and t (magit--safe-default-directory directory)))) (if default-directory (let* ((topdir (and t (magit-rev-parse-safe "--show-toplevel")))) (if topdir (let (updir) (setq topdir (magit-expand-git-file-name topdir)) (cond (... updir) (...))) (let* ((gitdir ...) (gitdir ...)) (if gitdir (progn ...))))) nil)))
  (or (if magit--refresh-cache (let ((G35 (cons (or directory default-directory) 'magit-toplevel))) (let* ((G36 (and t (assoc G35 ...)))) (if G36 (progn (let* (...) (setcar ... ...)) (cdr G36)) (progn (let* (...) (setcdr ... ...)) (let (...) (let* ... ...) value))))) (let* ((default-directory (and t (magit--safe-default-directory directory)))) (if default-directory (let* ((topdir (and t ...))) (if topdir (let (updir) (setq topdir ...) (cond ... ...)) (let* (... ...) (if gitdir ...)))) nil))))
  magit-toplevel()
  (or (and (boundp 'magit--default-directory) magit--default-directory) (magit-toplevel))
  magit-repository-local-repository()
  (or repository (magit-repository-local-repository))
  (assoc (or repository (magit-repository-local-repository)) magit-repository-local-cache)
  (and t (assoc (or repository (magit-repository-local-repository)) magit-repository-local-cache))
  (let* ((cache (and t (assoc (or repository (magit-repository-local-repository)) magit-repository-local-cache)))) (if cache (setq cache (assoc-delete-all key cache)) nil))
  magit-repository-local-delete(this-commit-command)
  magit-commit--reset-command()
  with-editor-finish(nil)
  funcall-interactively(with-editor-finish nil)
  command-execute(with-editor-finish)

Tramp debug log:

;; Emacs: 28.2 Tramp: 2.5.3.28.2 -*- mode: outline; coding: utf-8; -*-
10:38:13.114404 tramp-send-command (6) # test -e /tmp/a-test-repo/.git/COMMIT_EDITMSG 2>/dev/null; echo tramp_exit_status $?
10:38:13.154608 tramp-wait-for-regexp (6) # 
tramp_exit_status 0
///0c705e19c85cea87304133560ac40b2f#$
10:38:13.159099 tramp-send-command (6) # ( cd /tmp/a-test-repo/.git/ && kill -s USR1 3991181 </dev/null; echo tramp_exit_status $? )
10:38:13.199448 tramp-wait-for-regexp (6) # 
tramp_exit_status 0
///0c705e19c85cea87304133560ac40b2f#$
10:38:13.305668 tramp-send-command (6) # test -d /tmp/a-test-repo/.git 2>/dev/null; echo tramp_exit_status $?
10:38:13.343979 tramp-wait-for-regexp (6) # 
tramp_exit_status 0
///0c705e19c85cea87304133560ac40b2f#$
10:38:13.353894 tramp-do-file-attributes-with-stat (5) # file attributes with stat: /tmp/a-test-repo/.git/
10:38:13.354289 tramp-send-command (6) # (env QUOTING_STYLE=locale \stat -c '((/////%N/////) %h /////%U///// /////%G///// %X %Y %Z %s /////%A///// t %i -1)' /tmp/a-test-repo/.git/ | sed -e 's/"/\\"/g' -e 's/\/\/\/\/\//"/g') 2>/dev/null; echo tramp_exit_status $?
10:38:13.391608 tramp-send-command (6) # test -d /tmp/a-test-repo 2>/dev/null; echo tramp_exit_status $?
10:38:13.392587 tramp-send-string (1) # Remote file error: Forbidden reentrant call of Tramp
10:38:50.141255 tramp-do-file-attributes-with-ls (5) # file attributes with ls: /tmp/a-test-repo/.git/
10:38:50.141743 tramp-send-command (6) # test -e /tmp/a-test-repo/.git/ 2>/dev/null; echo tramp_exit_status $?
10:38:50.181408 tramp-wait-for-regexp (6) # 
tramp_exit_status 0
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.181858 tramp-get-ls-command (5) # Finding a suitable ‘ls’ command
10:38:50.182431 tramp-send-command (6) # while read d; do if test -x $d/ls && test -f $d/ls; then echo tramp_executable $d/ls; break; fi; done <<'a0afce8996ee05872d4b3245d153b7fe'
/bin
/usr/bin
/sbin
/usr/sbin
/usr/local/bin
/usr/local/sbin
a0afce8996ee05872d4b3245d153b7fe
10:38:50.221906 tramp-wait-for-regexp (6) # 
tramp_executable /bin/ls
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.222488 tramp-send-command (6) # /bin/ls -lnd / 2>/dev/null; echo tramp_exit_status $?
10:38:50.265095 tramp-wait-for-regexp (6) # 
drwxr-xr-x 18 0 0 4096 Jul 24 10:38 /
tramp_exit_status 0
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.265621 tramp-send-command (6) # /bin/ls --color=never -al /dev/null 2>/dev/null; echo tramp_exit_status $?
10:38:50.308125 tramp-wait-for-regexp (6) # 
crw-rw-rw- 1 root root 1, 3 Sep  4 12:25 /dev/null
tramp_exit_status 0
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.308246 tramp-get-ls-command-with (5) # Checking, whether ‘ls --quoting-style=literal --show-control-chars’ works
10:38:50.308327 tramp-send-command (6) # /bin/ls --color=never --help 2>&1 | grep -iq busybox 2>/dev/null; echo tramp_exit_status $?
10:38:50.351115 tramp-wait-for-regexp (6) # 
tramp_exit_status 1
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.351268 tramp-send-command (6) # /bin/ls --color=never --quoting-style=literal --show-control-chars -al /dev/null 2>/dev/null; echo tramp_exit_status $?
10:38:50.393507 tramp-wait-for-regexp (6) # 
crw-rw-rw- 1 root root 1, 3 Sep  4 12:25 /dev/null
tramp_exit_status 0
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.394148 tramp-send-command (6) # /bin/ls --color=never -ild --quoting-style=literal --show-control-chars /tmp/a-test-repo/.git/
10:38:50.437416 tramp-wait-for-regexp (6) # 
1160704 drwxr-xr-x 8 teythoon teythoon 280 Sep 12 10:38 /tmp/a-test-repo/.git/
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.444729 tramp-send-command (6) # ( cd /tmp/a-test-repo/.git/ && env INSIDE_EMACS\=28.2\,magit\,tramp\:2.5.3.28.2 git --no-pager --literal-pathspecs -c core.preloadindex\=true -c log.showSignature\=false -c color.ui\=false -c color.diff\=false rev-parse --show-toplevel </dev/null 2>/dev/null; echo tramp_exit_status $? )
10:38:50.490413 tramp-wait-for-regexp (6) # 
tramp_exit_status 128
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.497724 tramp-send-command (6) # ( cd /tmp/a-test-repo/.git/ && env INSIDE_EMACS\=28.2\,magit\,tramp\:2.5.3.28.2 git --no-pager --literal-pathspecs -c core.preloadindex\=true -c log.showSignature\=false -c color.ui\=false -c color.diff\=false rev-parse --git-dir </dev/null 2>/dev/null; echo tramp_exit_status $? )
10:38:50.538722 tramp-wait-for-regexp (6) # 
.
tramp_exit_status 0
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.544467 tramp-send-command (6) # test -d /tmp/a-test-repo/.git 2>/dev/null; echo tramp_exit_status $?
10:38:50.583714 tramp-wait-for-regexp (6) # 
tramp_exit_status 0
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.591003 tramp-send-command (6) # ( cd /tmp/a-test-repo/.git/ && env INSIDE_EMACS\=28.2\,magit\,tramp\:2.5.3.28.2 git --no-pager --literal-pathspecs -c core.preloadindex\=true -c log.showSignature\=false -c color.ui\=false -c color.diff\=false rev-parse --is-bare-repository </dev/null 2>/dev/null; echo tramp_exit_status $? )
10:38:50.636959 tramp-wait-for-regexp (6) # 
false
tramp_exit_status 0
///0c705e19c85cea87304133560ac40b2f#$
10:38:50.641159 tramp-send-command (6) # test -e /tmp/a-test-repo/.git/gitdir 2>/dev/null; echo tramp_exit_status $?
10:38:50.681147 tramp-wait-for-regexp (6) # 
tramp_exit_status 1
///0c705e19c85cea87304133560ac40b2f#$
  1. To reproduce:

On a remote system, create a git repository, enable commit signing, and setup GnuPG so that it has a key to sign with. Check that creating a signed commit works. Then, visit a file in that repository via Tramp, change it, and try to create a commit using Magit.

@tarsius
Copy link
Member

tarsius commented Sep 13, 2023

Hallo Justus!

I am by no means a Tramp expert, since I pretty much only use it to debug issues other people are having when using Magit over Tramp. ;)

Because I was planning to do another round of that next week anyway, your timing couldn't be much better. Turns out though, that the server I was going to use for testing, is completely borked, so that is a bit of a setback.

But actually, I think we might be able to solve this one, before I get my other box up and running again.

With Magit from git, the commit is created,

Just to make sure, the created commit is signed, right?

On a remote system, create a git repository, enable commit signing,
and setup GnuPG so that it has a key to sign with. Check that
creating a signed commit works. Then, visit a file in that repository
via Tramp, change it, and try to create a commit using Magit.

So the agent already has the decrypted key, right?

If those are two ayes, my initial guess that is has something to do with issues when the agent is trying to obtain the passphrase, obviously is out of the window. Which is probably for the better.

Debugger entered--Lisp error: (remote-file-error "Forbidden reentrant call of Tramp")
[...]
  (progn (dired-uncache default-dir))
[...]

That uncaching is not essential.

Reading up a bit on Tramp, I have learned that it cannot run more than one processes per connection at once, and since the call to dired-uncache is in a process sentinal and is going to start sub-processes, that is bound to become an issue. Looking through the history of the use of this function, I learned that many years ago we tried to protect against that, but apparently that got lost at some point.

So, please try with the call to dired-uncache removed. (Don't forget to M-x eval-defun, after making the edit.)

If that works, and I have high hopes it will, I'll change this so dired-uncache is only called if (file-remote-p default-dir) returns nil.

@tarsius tarsius added bug Something isn't working area: tramp labels Sep 13, 2023
@tarsius
Copy link
Member

tarsius commented Sep 13, 2023

Hm, and I should look into making this optional even locally. It's used a lot and is potentially horrible for performance when dealing with large directories and/or on Windows. Having to manually refresh Dired buffers on demand, doesn't seem like a big deal in comparison.

@teythoon
Copy link
Author

teythoon commented Sep 14, 2023 via email

@tarsius
Copy link
Member

tarsius commented Sep 19, 2023

The call to magit-process-buffer, which triggered the issue, wasn't actually necessary. The calling function already knows that value. I've pushed a commit that removes that call.

You still need to remove the call to dired-uncache. That function actually only does something over tramp; so disabling it over Tramp would not make sense. ;P

I still don't understand why signing a commit would trigger this issue. magit-process-finish, where the forbidden reentrant calls originate, is only called after the git commit process has exited. But I have to admit, I haven't actually experimented yet, hopefully I'll gain some insight once I do.

You could try updating Tramp from GNU ELPA, on the off chance that this is causes by a Tramp bug that has already been fixed.

@kohnish
Copy link

kohnish commented Nov 29, 2023

It seems it's still the case with latest master of emacs(tramp) and magit.
But even without magit, it's tedious to do this over ssh. It seems GPG_TTY=$(tty) is needed in case it needs to read the private key for signing and triggers pinentry which blocks that whole screen for typing the passphrase.

@hjudt
Copy link

hjudt commented Dec 9, 2023

I too get forbidden reentrant calls with a similar setup.

@kohnish using something like keychain that spawns gpg-agent and loads your keys and provides shell scripts to read in your gpg and ssh environment. there is a corresponding emacs package keychain-environment for better integration. with such a setup, i can use commit signing without any annoyances.

@tarsius tarsius added bug unconfirmed Maybe a bug, but unable to reproduce and removed bug Something isn't working labels Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tramp bug unconfirmed Maybe a bug, but unable to reproduce
Development

No branches or pull requests

4 participants