Bug#278550: tramp: Dangerous fset of substitute-in-file-name

[Toby Speight <streapadair@gmx.net>]

Package: tramp
Version: 1:2.0.45-1
Severity: normal
File: /usr/share/emacs/site-lisp/tramp/tramp-smb.el

I recently discovered problems with code that used substitute-in-file-name
to expand environment variables in file names.  It turned out that the name
in question was not having its environment variables expanded correctly.

Subsequent investigation showed that substitute-in-file-name is fbound
to 'identity.

Grepping through all my emacs-lisp for code that could change the binding
of substitute-in-file-name turned up this snippet in tramp-smb.el:

/--------
| 	;; Do `PC-do-completion' without substitution
| 	(let* (save)
| 	  (fset 'save (symbol-function 'substitute-in-file-name))
| 	  (fset 'substitute-in-file-name (symbol-function 'identity))
| 	  ad-do-it
| 	  (fset 'substitute-in-file-name (symbol-function 'save)))
\--------

which has no unwind-protect at all to ensure that substitute-in-file-name
is restored afterwards, even in the face of errors.

Also, it does not do what you think in the recursive case.  Creating a
dynamic variable with `let' does not affect function lookup using that
name:

/--------
| (let* (save)
|   (fset 'save (symbol-function 'identity))
|   (let* (save)
|     (fset 'save (symbol-function 'ignore)))
|   (save t))
| nil
\--------

/--------
| (let* ((save (symbol-function 'identity)))
|   (let* ((save (symbol-function 'ignore))))
|   (funcall save t))
| t
\--------

As well as being neater and easier to read, the version that stores
the value in a variable slot rather than a function slot actually
works correctly in more cases.

I don't know if the above is the source of my problem (I didn't
knowingly ask for SMB resources), but there is definitely an issue:

* Use unwind-protect to restore the binding
* Use the variable slot rather than the function slot to save it

Untested code:

/--------
| 	;; Do `PC-do-completion' without substitution
| 	(let (save (symbol-function 'substitute-in-file-name))
| 	  (fset 'substitute-in-file-name (symbol-function 'identity))
| 	  (unwind-protect
|               ad-do-it
|             (fset 'substitute-in-file-name save)))
\--------


-- 

-- System Information:
Debian Release: 3.1
  APT prefers testing
  APT policy: (990, 'testing'), (300, 'unstable')
Architecture: i386 (i686)
Kernel: Linux 2.6.8
Locale: LANG=en_GB, LC_CTYPE=en_GB

Versions of packages tramp depends on:
ii  emacs21 [emacsen]             21.3+1-8   The GNU Emacs editor

-- no debconf information