bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable

[Federico Tedin <federicotedin@gmail.com>]

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

Ok! Here's my first try at this. I ended up skipping the check on
DEFAULT-DIRECTORY since as you mentioned, its value is used with
expand-file-name itself. In the other case, if default-directory is
picked up, then I checked the value of that variable.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 4251 bytes --]

From 0f960aeab1c4b3182d597ac0ce80526f09e97452 Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Wed, 15 Sep 2021 00:15:16 +0200
Subject: [PATCH] Add null byte checks to filenames in expand-file-name
 (bug#49723)

* src/fileio.c (expand-file-name): Check for null bytes for both NAME
and DEFAULT-DIRECTORY arguments.  Also check for null bytes in
buffer-local default-directory, assuming it is used.
* src/coding.c (encode_file_name): Use CHECK_STRING_NULL_BYTES.
* src/lisp.h (CHECK_STRING_NULL_BYTES): Add function for checking for
null bytes in Lisp strings.
* test/src/fileio-tests.el (fileio-test--expand-file-name-null-bytes):
Add test for new changes to expand-file-name.
* etc/NEWS: Announce changes.
---
 etc/NEWS                 | 6 ++++++
 src/coding.c             | 3 +--
 src/fileio.c             | 6 +++++-
 src/lisp.h               | 7 +++++++
 test/src/fileio-tests.el | 9 +++++++++
 5 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 5809716868..8d96ceff79 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -278,6 +278,12 @@ personalize the uniquified buffer name.
 ---
 ** 'remove-hook' is now an interactive command.
 
+** 'expand-file-name' now checks for null bytes in filenames.
+The function will now check for null bytes in both NAME and
+DEFAULT-DIRECTORY arguments, as well as in the 'default-directory'
+buffer-local variable, assuming its value is used.
+
+---
 ** Frames
 
 +++
diff --git a/src/coding.c b/src/coding.c
index d027c7d539..7030a53869 100644
--- a/src/coding.c
+++ b/src/coding.c
@@ -10430,8 +10430,7 @@ encode_file_name (Lisp_Object fname)
      cause subtle bugs because the system would silently use a
      different filename than expected.  Perform this check after
      encoding to not miss NUL bytes introduced through encoding.  */
-  CHECK_TYPE (memchr (SSDATA (encoded), '\0', SBYTES (encoded)) == NULL,
-              Qfilenamep, fname);
+  CHECK_STRING_NULL_BYTES (encoded);
   return encoded;
 }
 
diff --git a/src/fileio.c b/src/fileio.c
index 0db8ed793b..3c13d3fe41 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -945,6 +945,7 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
   USE_SAFE_ALLOCA;
 
   CHECK_STRING (name);
+  CHECK_STRING_NULL_BYTES (name);
 
   /* If the file name has special constructs in it,
      call the corresponding file name handler.  */
@@ -993,7 +994,10 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0,
       if (STRINGP (dir))
 	{
 	  if (file_name_absolute_no_tilde_p (dir))
-	    default_directory = dir;
+	    {
+	      CHECK_STRING_NULL_BYTES (dir);
+	      default_directory = dir;
+	    }
 	  else
 	    {
 	      Lisp_Object absdir
diff --git a/src/lisp.h b/src/lisp.h
index 7bfc69b647..9716b34bae 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1615,6 +1615,13 @@ STRING_SET_CHARS (Lisp_Object string, ptrdiff_t newsize)
   XSTRING (string)->u.s.size = newsize;
 }
 
+INLINE void
+CHECK_STRING_NULL_BYTES (Lisp_Object string)
+{
+  CHECK_TYPE (memchr (SSDATA (string), '\0', SBYTES (string)) == NULL,
+	      Qfilenamep, string);
+}
+
 /* A regular vector is just a header plus an array of Lisp_Objects.  */
 
 struct Lisp_Vector
diff --git a/test/src/fileio-tests.el b/test/src/fileio-tests.el
index f4d123b426..438ebebb76 100644
--- a/test/src/fileio-tests.el
+++ b/test/src/fileio-tests.el
@@ -136,6 +136,15 @@ fileio-tests--relative-default-directory
     (should (and (file-name-absolute-p name)
                  (not (eq (aref name 0) ?~))))))
 
+(ert-deftest fileio-test--expand-file-name-null-bytes ()
+  "Test that expand-file-name checks for null bytes in filenames."
+  (should-error (expand-file-name (concat "file" (char-to-string ?\0) ".txt"))
+                :type 'wrong-type-argument)
+  (should-error (expand-file-name "file.txt" (concat "dir" (char-to-string ?\0)))
+                :type 'wrong-type-argument)
+  (let ((default-directory (concat "dir" (char-to-string ?\0))))
+    (should-error (expand-file-name "file.txt") :type 'wrong-type-argument)))
+
 (ert-deftest fileio-tests--file-name-absolute-p ()
   "Test file-name-absolute-p."
   (dolist (suffix '("" "/" "//" "/foo" "/foo/" "/foo//" "/foo/bar"))
-- 
2.25.1


[-- Attachment #3: Type: text/plain, Size: 986 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

>> From: Federico Tedin <federicotedin@gmail.com>
>> Cc: 49723@debbugs.gnu.org,  Philipp Stephani <phst@google.com>
>> Date: Tue, 14 Sep 2021 21:01:16 +0200
>> 
>> I'm interested in looking into this one since I want to learn more about
>> the C side of the codebase. However, I wasn't able to find a call to
>> expand-file-name in encode_file_name or encode_file_name_1. I did find
>> the null byte check though (CHECK_TYPE + memchr). Maybe I am missing
>> something out.
>
> My description was inaccurate: the expand-file-name call usually
> precedes the call to ENCODE_FILE, it is not part of encode_file_name.
>
>> I assume that a similar check on expand-file-name should be applied to
>> both input arguments, NAME and DEFAULT-DIRECTORY?
>
> I don't think we need that because expand-file-name calls itself on
> DEFAULT-DIRECTORY internally.  But we may need to perform the check on
> default-directory, if we use it inside expand-file-name.