bug#40407: [PATCH] slow ENCODE_FILE and DECODE_FILE

["Mattias Engdegård" <mattiase@acm.org>]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

5 apr. 2020 kl. 17.56 skrev Eli Zaretskii <eliz@gnu.org>:

> Once we do set up default-file-name-coding-system, these macros will
> never return their argument (unless someone forcefully sets the
> encoding to nil, in which case they deserve what they get).  Do you
> agree?

Thank you, and yes, I do agree partly: ENCODE_FILE is the identity for all unibyte strings no matter the coding system in use.

However, my point (which I didn't do a very good job explaining) was that if either ENCODE_FILE or DECODE_FILE are called with the assumption that they return a new string, that is at least a latent bug.

Thus I went through them all once again, and found a few questionable calls that I'd like to fix. They rely on Fexpand_file_name returning a new string, which may or may not be true now but we would be better without such assumptions. (I also stumbled on a potential GC-related bug.) Patch attached!

With these fixed, nothing prevents those two functions from using no-copy semantics. I agree this approach is better and safer than going straight for code_convert_string_norecord in one pass.


[-- Attachment #2: 0001-Don-t-rely-on-copying-in-EN-DE-CODE_FILE.patch --]
[-- Type: application/octet-stream, Size: 2496 bytes --]

From ff62a3874890810823f79dac1273ebdd214ba529 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 6 Apr 2020 15:20:08 +0200
Subject: [PATCH] Don't rely on copying in {EN,DE}CODE_FILE

Callers of ENCODE_FILE and DECODE_FILE should not assume that these
functions always return a new string (bug#40407).

* src/w32fns.c (Fw32_shell_execute):
* src/w32proc.c (Fw32_application_type):
Sink taking the address of a Lisp string past GC points.
Copy values returned from ENCODE_FILE before mutating them.
---
 src/w32fns.c  | 4 ++--
 src/w32proc.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/w32fns.c b/src/w32fns.c
index 9bb4e27b01..8d714f0b8d 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -8258,7 +8258,6 @@ parameters (e.g., \"printto\" requires the printer address).  Otherwise,
   /* Encode filename, current directory and parameters.  */
   current_dir = GUI_ENCODE_FILE (current_dir);
   document = GUI_ENCODE_FILE (document);
-  doc_w = GUI_SDATA (document);
   if (STRINGP (parameters))
     {
       parameters = GUI_ENCODE_SYSTEM (parameters);
@@ -8269,6 +8268,7 @@ parameters (e.g., \"printto\" requires the printer address).  Otherwise,
       operation = GUI_ENCODE_SYSTEM (operation);
       ops_w = GUI_SDATA (operation);
     }
+  doc_w = GUI_SDATA (document);
   result = (intptr_t) ShellExecuteW (NULL, ops_w, doc_w, params_w,
 				     GUI_SDATA (current_dir),
 				     (FIXNUMP (show_flag)
@@ -8353,7 +8353,7 @@ parameters (e.g., \"printto\" requires the printer address).  Otherwise,
   handler = Ffind_file_name_handler (absdoc, Qfile_exists_p);
   if (NILP (handler))
     {
-      Lisp_Object absdoc_encoded = ENCODE_FILE (absdoc);
+      Lisp_Object absdoc_encoded = Fcopy_sequence (ENCODE_FILE (absdoc));
 
       if (faccessat (AT_FDCWD, SSDATA (absdoc_encoded), F_OK, AT_EACCESS) == 0)
 	{
diff --git a/src/w32proc.c b/src/w32proc.c
index de33726905..16e32e4c58 100644
--- a/src/w32proc.c
+++ b/src/w32proc.c
@@ -3231,7 +3231,7 @@ DEFUN ("w32-application-type", Fw32_application_type,
   char *progname, progname_a[MAX_PATH];
 
   program = Fexpand_file_name (program, Qnil);
-  encoded_progname = ENCODE_FILE (program);
+  encoded_progname = Fcopy_sequence (ENCODE_FILE (program));
   progname = SSDATA (encoded_progname);
   unixtodos_filename (progname);
   filename_to_ansi (progname, progname_a);
-- 
2.21.1 (Apple Git-122.3)