bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii <eliz@gnu.org>]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Michael Albinus <michael.albinus@gmx.de>,  ncaprisunfan@gmail.com,
>   49261@debbugs.gnu.org
> Date: Wed, 07 Jul 2021 18:01:25 +0200
> 
> +(defun auto-save--transform-file-name (filename transforms
> +                                                prefix suffix)
> +  "Transform FILENAME according to TRANSFORMS.
> +See `auto-save-file-name-transforms' for the format of
> +TRANSFORMS.  PREFIX is prepended to the non-directory portion of
> +the resulting file name, and SUFFIX is appended."
> +  (let (result uniq)
> +    ;; Apply user-specified translations
> +    ;; to the file name.
> +    (while (and transforms (not result))
> +      (if (string-match (car (car transforms)) filename)
> +	  (setq result (replace-match (cadr (car transforms)) t nil
> +				      filename)
> +		uniq (car (cddr (car transforms)))))

If this is going to be called from C, it should probably use
save-match-data, because no one will expect that just modifying a file
from some Lisp program could clobber the match data.

Also, do we ever need this during loadup?  Because before files.el is
loaded by loadup, this function will not be available, so
unconditionally calling it from C without protection, not even
safe_call or somesuch, is not safe.

> +static Lisp_Object
> +make_lock_file_name (Lisp_Object fn)
> +{
> +  return ENCODE_FILE (call1 (intern ("make-lock-file-name"), fn));
> +}

I'd prefer not to have a function return an encoded file name, it's
unusual and unexpected.  It is better to leave that to the caller.
(And if you do that, the rationale for having a separate function for
this will all but disappear, I think.)

> -  fn = Fexpand_file_name (fn, Qnil);
> +  fn = make_lock_file_name (Fexpand_file_name (fn, Qnil));

In the original code, 'fn' was an un-encoded file name, but your
changes made it encoded.  Why not keep the code more similar by having
a separate variable with the encoded file name?  E.g., this would
avoid potential trouble here:

>    if (!NILP (subject_buf)
>        && NILP (Fverify_visited_file_modtime (subject_buf))
>        && !NILP (Ffile_exists_p (fn)) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

Ffile_exists_p was passed an un-encoded file name in the original
code.  It calls file handlers, and encodes local file names by itself,
so it is better to pass it un-encoded file names.

> -      && !(lfname && current_lock_owner (NULL, lfname) == -2))
> +      && !(create_lockfiles && current_lock_owner (NULL, lfname) == -2))
>      call1 (intern ("userlock--ask-user-about-supersession-threat"), fn);
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Likewise here: Lisp function should generally be called with
un-encoded file names.

>  unlock_file_body (Lisp_Object fn)
>  {
>    char *lfname;
> -  USE_SAFE_ALLOCA;
> -
> -  Lisp_Object filename = Fexpand_file_name (fn, Qnil);
> -  fn = ENCODE_FILE (filename);
>  
> -  MAKE_LOCK_NAME (lfname, fn);
> +  Lisp_Object filename = make_lock_file_name (Fexpand_file_name (fn, Qnil));
> +  lfname = SSDATA (filename);
>  
>    int err = current_lock_owner (0, lfname);
>    if (err == -2 && unlink (lfname) != 0 && errno != ENOENT)
> @@ -736,7 +714,6 @@ unlock_file_body (Lisp_Object fn)
>    if (0 < err)
>      report_file_errno ("Unlocking file", filename, err);

Same problem here: 'filename' is now an encoded file name, so you call
report_file_errno with an encoded file name, which is a no-no.

Last, but not least: do we care that now locking a file will cons
strings, even with the default value of lock-file-name-transforms?
That sounds like we are punishing the vast majority of users for the
benefit of the few who need the opt-in behavior.  Should we perhaps
avoid consing strings, or maybe even calling Lisp altogether, under
the default value of that option?

Thanks.