unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
@ 2021-07-24 17:39 Eli Zaretskii
  2021-09-14 19:01 ` Federico Tedin
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-07-24 17:39 UTC (permalink / raw)
  To: 49723; +Cc: Philipp Stephani

From: eliz@HOME-C4E4A596F7.i-did-not-set--mail-host-address--so-tickle-me
--text follows this line--
Emacs currently tries to catch null bytes in file names in
encode_file_name, after encoding the file name.  But that is not
reliable enough, because it assumes that the call to expand-file-name
at the beginning of encode_file_name leaves those null bytes intact.
That is not guaranteed, and in fact doesn't happen at least on
MS-Windows: the file name gets chopped after the first null byte, and
doesn't trigger the test in encode_file_name.

So for a more reliable test we need to include such a check inside
expand-file-name (and then we could probably drop the test in
encode_file_name, because Emacs always runs file names through
expand-file-name before using them).

In GNU Emacs 28.0.50 (build 1573, i686-pc-mingw32)
 of 2021-07-24 built on HOME-C4E4A596F7
Repository revision: 7c83e605ab84e8b62254c55f347abc8aa9c6057b
Repository branch: master
Windowing system distributor 'Microsoft Corp.', version 5.1.2600
System Description: Microsoft Windows XP Service Pack 3 (v5.1.0.2600)

Configured using:
 'configure -C --prefix=/d/usr --with-wide-int --with-modules
 --enable-checking=yes,glyphs 'CFLAGS=-O0 -gdwarf-4 -g3''

Configured features:
ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NOTIFY
W32NOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF TOOLKIT_SCROLL_BARS XPM
ZLIB

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp1255

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
iso-transl tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel dos-w32 ls-lisp disp-table term/w32-win w32-win
w32-vars term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads w32notify w32 lcms2 multi-tty
make-network-process emacs)

Memory information:
((conses 16 56888 7103)
 (symbols 48 7785 1)
 (strings 16 21560 2884)
 (string-bytes 1 630636)
 (vectors 16 13320)
 (vector-slots 8 173301 9720)
 (floats 8 23 47)
 (intervals 40 265 116)
 (buffers 888 10))





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-07-24 17:39 bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable Eli Zaretskii
@ 2021-09-14 19:01 ` Federico Tedin
  2021-09-14 19:24   ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Federico Tedin @ 2021-09-14 19:01 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Philipp Stephani, 49723

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.

I assume that a similar check on expand-file-name should be applied to
both input arguments, NAME and DEFAULT-DIRECTORY?

Thanks!

Eli Zaretskii <eliz@gnu.org> writes:

> From: eliz@HOME-C4E4A596F7.i-did-not-set--mail-host-address--so-tickle-me
> --text follows this line--
> Emacs currently tries to catch null bytes in file names in
> encode_file_name, after encoding the file name.  But that is not
> reliable enough, because it assumes that the call to expand-file-name
> at the beginning of encode_file_name leaves those null bytes intact.
> That is not guaranteed, and in fact doesn't happen at least on
> MS-Windows: the file name gets chopped after the first null byte, and
> doesn't trigger the test in encode_file_name.
>
> So for a more reliable test we need to include such a check inside
> expand-file-name (and then we could probably drop the test in
> encode_file_name, because Emacs always runs file names through
> expand-file-name before using them).
>
> In GNU Emacs 28.0.50 (build 1573, i686-pc-mingw32)
>  of 2021-07-24 built on HOME-C4E4A596F7
> Repository revision: 7c83e605ab84e8b62254c55f347abc8aa9c6057b
> Repository branch: master
> Windowing system distributor 'Microsoft Corp.', version 5.1.2600
> System Description: Microsoft Windows XP Service Pack 3 (v5.1.0.2600)
>
> Configured using:
>  'configure -C --prefix=/d/usr --with-wide-int --with-modules
>  --enable-checking=yes,glyphs 'CFLAGS=-O0 -gdwarf-4 -g3''
>
> Configured features:
> ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NOTIFY
> W32NOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF TOOLKIT_SCROLL_BARS XPM
> ZLIB
>
> Important settings:
>   value of $LANG: ENU
>   locale-coding-system: cp1255
>
> Major mode: Lisp Interaction
>
> Minor modes in effect:
>   tooltip-mode: t
>   global-eldoc-mode: t
>   eldoc-mode: t
>   electric-indent-mode: t
>   mouse-wheel-mode: t
>   tool-bar-mode: t
>   menu-bar-mode: t
>   file-name-shadow-mode: t
>   global-font-lock-mode: t
>   font-lock-mode: t
>   blink-cursor-mode: t
>   auto-composition-mode: t
>   auto-encryption-mode: t
>   auto-compression-mode: t
>   line-number-mode: t
>   indent-tabs-mode: t
>   transient-mark-mode: t
>
> Load-path shadows:
> None found.
>
> Features:
> (shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
> rfc822 mml mml-sec epa derived epg epg-config gnus-util rmail
> rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
> eieio-loaddefs password-cache json map text-property-search time-date
> subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
> mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
> cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
> iso-transl tooltip eldoc electric uniquify ediff-hook vc-hooks
> lisp-float-type mwheel dos-w32 ls-lisp disp-table term/w32-win w32-win
> w32-vars term/common-win tool-bar dnd fontset image regexp-opt fringe
> tabulated-list replace newcomment text-mode elisp-mode lisp-mode
> prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
> timer select scroll-bar mouse jit-lock font-lock syntax font-core
> term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
> misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
> cp51932 hebrew greek romanian slovak czech european ethiopic indian
> cyrillic chinese composite charscript charprop case-table epa-hook
> jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
> loaddefs faces cus-face macroexp files window text-properties overlay
> sha1 md5 base64 format env code-pages mule custom widget
> hashtable-print-readable backquote threads w32notify w32 lcms2 multi-tty
> make-network-process emacs)
>
> Memory information:
> ((conses 16 56888 7103)
>  (symbols 48 7785 1)
>  (strings 16 21560 2884)
>  (string-bytes 1 630636)
>  (vectors 16 13320)
>  (vector-slots 8 173301 9720)
>  (floats 8 23 47)
>  (intervals 40 265 116)
>  (buffers 888 10))





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-14 19:01 ` Federico Tedin
@ 2021-09-14 19:24   ` Eli Zaretskii
  2021-09-14 22:17     ` Federico Tedin
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-09-14 19:24 UTC (permalink / raw)
  To: Federico Tedin; +Cc: phst, 49723

> 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.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-14 19:24   ` Eli Zaretskii
@ 2021-09-14 22:17     ` Federico Tedin
  2021-09-16 12:25       ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Federico Tedin @ 2021-09-14 22:17 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: phst, 49723

[-- 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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-14 22:17     ` Federico Tedin
@ 2021-09-16 12:25       ` Eli Zaretskii
  2021-09-16 16:58         ` Federico Tedin
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-09-16 12:25 UTC (permalink / raw)
  To: Federico Tedin; +Cc: phst, 49723

> From: Federico Tedin <federicotedin@gmail.com>
> Cc: phst@google.com,  49723@debbugs.gnu.org
> Date: Wed, 15 Sep 2021 00:17:33 +0200
> 
> 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.

Thanks, this LGTM.

> +** '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.

This should say that if null bytes are found, expand-file-name will
signal an error.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-16 12:25       ` Eli Zaretskii
@ 2021-09-16 16:58         ` Federico Tedin
  2021-09-16 18:12           ` Michael Albinus
  2021-09-17  7:49           ` Eli Zaretskii
  0 siblings, 2 replies; 18+ messages in thread
From: Federico Tedin @ 2021-09-16 16:58 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: phst, 49723

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

Eli Zaretskii <eliz@gnu.org> writes:

>> +** '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.
>
> This should say that if null bytes are found, expand-file-name will
> signal an error.

Makes sense! I'm attaching a revised version.


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

From 6faba0f1107290d44a83d396a61747dea67f0f39 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                 | 7 +++++++
 src/coding.c             | 3 +--
 src/fileio.c             | 6 +++++-
 src/lisp.h               | 7 +++++++
 test/src/fileio-tests.el | 9 +++++++++
 5 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 17c42ce104..3484ef3f3f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -281,6 +281,13 @@ 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.  If null bytes are
+found, 'expand-file-name' will signal an error.
+
+---
 ** 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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-16 16:58         ` Federico Tedin
@ 2021-09-16 18:12           ` Michael Albinus
  2021-09-16 18:30             ` Eli Zaretskii
  2021-09-16 18:38             ` Federico Tedin
  2021-09-17  7:49           ` Eli Zaretskii
  1 sibling, 2 replies; 18+ messages in thread
From: Michael Albinus @ 2021-09-16 18:12 UTC (permalink / raw)
  To: Federico Tedin; +Cc: phst, 49723

Federico Tedin <federicotedin@gmail.com> writes:

Hi,

Sorry for being late, I have seen this just now only.

> +** '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.  If null bytes are
> +found, 'expand-file-name' will signal an error.

Should this be implemented also in remote file names?

Best regards, Michael.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-16 18:12           ` Michael Albinus
@ 2021-09-16 18:30             ` Eli Zaretskii
  2021-09-16 18:44               ` Michael Albinus
  2021-09-16 18:38             ` Federico Tedin
  1 sibling, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-09-16 18:30 UTC (permalink / raw)
  To: Michael Albinus; +Cc: phst, 49723, federicotedin

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  phst@google.com,  49723@debbugs.gnu.org
> Date: Thu, 16 Sep 2021 20:12:09 +0200
> 
> > +** '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.  If null bytes are
> > +found, 'expand-file-name' will signal an error.
> 
> Should this be implemented also in remote file names?

Are we sure remote file names cannot include null bytes?





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-16 18:12           ` Michael Albinus
  2021-09-16 18:30             ` Eli Zaretskii
@ 2021-09-16 18:38             ` Federico Tedin
  2021-09-16 18:51               ` Michael Albinus
  1 sibling, 1 reply; 18+ messages in thread
From: Federico Tedin @ 2021-09-16 18:38 UTC (permalink / raw)
  To: Michael Albinus; +Cc: phst, 49723

Michael Albinus <michael.albinus@gmx.de> writes:

> Federico Tedin <federicotedin@gmail.com> writes:
>
> Hi,
>
> Sorry for being late, I have seen this just now only.
>
>> +** '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.  If null bytes are
>> +found, 'expand-file-name' will signal an error.
>
> Should this be implemented also in remote file names?
>
> Best regards, Michael.

Would this imply only doing it in `tramp-handle-expand-file-name`, or
other functions as well?

I had thought of doing it for at least `tramp-handle-expand-file-name`,
but then I noticed that the function eventually calls `expand-file-name`
with a combination of name + dir, or only with name. So in that case I
figured that the null bytes check would be performed by
`expand-file-name` anyways.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-16 18:30             ` Eli Zaretskii
@ 2021-09-16 18:44               ` Michael Albinus
  2021-09-16 19:07                 ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Michael Albinus @ 2021-09-16 18:44 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: phst, 49723, federicotedin

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> > +** '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.  If null bytes are
>> > +found, 'expand-file-name' will signal an error.
>>
>> Should this be implemented also in remote file names?
>
> Are we sure remote file names cannot include null bytes?

Likely not. I have added "foo\0bar" as file name in tramp-test.el, and
then I get

--8<---------------cut here---------------start------------->8---
Test tramp-test41-special-characters condition:
    (ert-test-failed
     ((should
       (file-exists-p file1))
      :form
      (file-exists-p "/mock:gandalf:/tmp/tramp-testkLeKOx/foo\0bar")
      :value nil))
   FAILED  1/1  tramp-test41-special-characters (0.484141 sec)

--8<---------------cut here---------------end--------------->8---

Well, perhaps Tramp can be improved to handle null bytes on (remote)
shell level, but do we need this?

Best regards, Michael.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-16 18:38             ` Federico Tedin
@ 2021-09-16 18:51               ` Michael Albinus
  2021-09-16 19:13                 ` Federico Tedin
  0 siblings, 1 reply; 18+ messages in thread
From: Michael Albinus @ 2021-09-16 18:51 UTC (permalink / raw)
  To: Federico Tedin; +Cc: phst, 49723

Federico Tedin <federicotedin@gmail.com> writes:

Hi Federico,

> Would this imply only doing it in `tramp-handle-expand-file-name`, or
> other functions as well?
>
> I had thought of doing it for at least `tramp-handle-expand-file-name`,
> but then I noticed that the function eventually calls `expand-file-name`
> with a combination of name + dir, or only with name. So in that case I
> figured that the null bytes check would be performed by
> `expand-file-name` anyways.

I haven't checked yet what's needed. However, there are several
incarnations of `expand-file-name' implementation in Tramp, namely

--8<---------------cut here---------------start------------->8---
grep --color=auto -nH --null -E 'defun.+handle-expand-file-name' *.el
tramp.el3393:(defun tramp-handle-expand-file-name (name &optional dir)
tramp-gvfs.el1137:(defun tramp-gvfs-handle-expand-file-name (name &optional dir)
tramp-sh.el2683:(defun tramp-sh-handle-expand-file-name (name &optional dir)
tramp-smb.el736:(defun tramp-smb-handle-expand-file-name (name &optional dir)
tramp-sudoedit.el346:(defun tramp-sudoedit-handle-expand-file-name (name &optional dir)
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-16 18:44               ` Michael Albinus
@ 2021-09-16 19:07                 ` Eli Zaretskii
  0 siblings, 0 replies; 18+ messages in thread
From: Eli Zaretskii @ 2021-09-16 19:07 UTC (permalink / raw)
  To: Michael Albinus; +Cc: phst, 49723, federicotedin

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: federicotedin@gmail.com,  phst@google.com,  49723@debbugs.gnu.org
> Date: Thu, 16 Sep 2021 20:44:32 +0200
> 
> >> Should this be implemented also in remote file names?
> >
> > Are we sure remote file names cannot include null bytes?
> 
> Likely not. I have added "foo\0bar" as file name in tramp-test.el, and
> then I get
> 
> --8<---------------cut here---------------start------------->8---
> Test tramp-test41-special-characters condition:
>     (ert-test-failed
>      ((should
>        (file-exists-p file1))
>       :form
>       (file-exists-p "/mock:gandalf:/tmp/tramp-testkLeKOx/foo\0bar")
>       :value nil))
>    FAILED  1/1  tramp-test41-special-characters (0.484141 sec)
> 
> --8<---------------cut here---------------end--------------->8---
> 
> Well, perhaps Tramp can be improved to handle null bytes on (remote)
> shell level, but do we need this?

I'm not sure.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-16 18:51               ` Michael Albinus
@ 2021-09-16 19:13                 ` Federico Tedin
  2021-09-20 11:59                   ` Michael Albinus
  0 siblings, 1 reply; 18+ messages in thread
From: Federico Tedin @ 2021-09-16 19:13 UTC (permalink / raw)
  To: Michael Albinus; +Cc: phst, 49723

Michael Albinus <michael.albinus@gmx.de> writes:

> I haven't checked yet what's needed. However, there are several
> incarnations of `expand-file-name' implementation in Tramp, namely
>
> grep --color=auto -nH --null -E 'defun.+handle-expand-file-name' *.el
> tramp.el3393:(defun tramp-handle-expand-file-name (name &optional dir)
> tramp-gvfs.el1137:(defun tramp-gvfs-handle-expand-file-name (name &optional dir)
> tramp-sh.el2683:(defun tramp-sh-handle-expand-file-name (name &optional dir)
> tramp-smb.el736:(defun tramp-smb-handle-expand-file-name (name &optional dir)
> tramp-sudoedit.el346:(defun tramp-sudoedit-handle-expand-file-name (name &optional dir)
>
> Best regards, Michael.

Ouch, hadn't noticed those. I gave a look at the four I hadn't seen (gvfs,
sh, smb and sudoedit) and it seems like all of them return expressions
in the shape of:

  (expand-file-name xyz)

or:

  (tramp-run-real-handler #'expand-file-name xyz)

with xyz being a value derived from NAME or NAME and DIR. So in that
case it seems like we would be covered by the changes in `expand-file-mode`.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-16 16:58         ` Federico Tedin
  2021-09-16 18:12           ` Michael Albinus
@ 2021-09-17  7:49           ` Eli Zaretskii
  2021-09-17 19:00             ` Federico Tedin
  1 sibling, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-09-17  7:49 UTC (permalink / raw)
  To: Federico Tedin; +Cc: phst, 49723

> From: Federico Tedin <federicotedin@gmail.com>
> Cc: phst@google.com,  49723@debbugs.gnu.org
> Date: Thu, 16 Sep 2021 18:58:02 +0200
> 
> >> +** '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.
> >
> > This should say that if null bytes are found, expand-file-name will
> > signal an error.
> 
> Makes sense! I'm attaching a revised version.

Thanks.  Did you run the test suite after applying the changes, and
did you see no regressions?  If you didn't yet run the test suite,
please be sure to run all of it, as the use of expand-file-name is
universal.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-17  7:49           ` Eli Zaretskii
@ 2021-09-17 19:00             ` Federico Tedin
  2021-09-18  6:51               ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Federico Tedin @ 2021-09-17 19:00 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: phst, 49723

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

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks.  Did you run the test suite after applying the changes, and
> did you see no regressions?  If you didn't yet run the test suite,
> please be sure to run all of it, as the use of expand-file-name is
> universal.

I hadn't, so I checked out master aa59d38c59 and applied my patch on top
of it. I then ran "make check" and waited for a bit. There appears to be
only two tests failing in test/lisp/time-stamp-tests.el
('time-stamp-format-day-of-week' and
'time-stamp-format-string-width'). Both seem to be unrelated to my
change; maybe it's my system's strange combination of
Spanish/English/German locale-related configurations (I'm attaching the
log just in case). All other test files were run without problems.


[-- Attachment #2: log --]
[-- Type: text/plain, Size: 10534 bytes --]

Running 135 tests (2021-09-17 20:46:15+0200, selector `(not (or (tag :expensive-test) (tag :unstable) (tag :nativecomp)))')
   passed    1/135  formatz-%-10z-hhmm (0.000985 sec)
   passed    2/135  formatz-%-10z-seconds (0.001173 sec)
   passed    3/135  formatz-%-10z-threedigit (0.000526 sec)
   passed    4/135  formatz-%-12z-hhmm (0.000991 sec)
   passed    5/135  formatz-%-12z-seconds (0.001199 sec)
   passed    6/135  formatz-%-12z-threedigit (0.000558 sec)
   passed    7/135  formatz-%-3z-hhmm (0.000924 sec)
   passed    8/135  formatz-%-3z-seconds (0.001093 sec)
   passed    9/135  formatz-%-3z-threedigit (0.000506 sec)
   passed   10/135  formatz-%-z-hhmm (0.000854 sec)
   passed   11/135  formatz-%-z-seconds (0.001047 sec)
   passed   12/135  formatz-%-z-threedigit (0.000491 sec)
   passed   13/135  formatz-%012:::z-hhmm (0.001046 sec)
   passed   14/135  formatz-%012:::z-seconds (0.001133 sec)
   passed   15/135  formatz-%012:::z-threedigit (0.000583 sec)
   passed   16/135  formatz-%012::z-hhmm (0.001066 sec)
   passed   17/135  formatz-%012::z-seconds (0.001096 sec)
   passed   18/135  formatz-%012::z-threedigit (0.000584 sec)
   passed   19/135  formatz-%012:z-hhmm (0.001047 sec)
   passed   20/135  formatz-%012:z-seconds (0.001066 sec)
   passed   21/135  formatz-%012:z-threedigit (0.000558 sec)
   passed   22/135  formatz-%012z-hhmm (0.001053 sec)
   passed   23/135  formatz-%012z-seconds (0.001166 sec)
   passed   24/135  formatz-%012z-threedigit (0.095949 sec)
   passed   25/135  formatz-%03:::z-hhmm (0.000898 sec)
   passed   26/135  formatz-%03:::z-seconds (0.000912 sec)
   passed   27/135  formatz-%03:::z-threedigit (0.000488 sec)
   passed   28/135  formatz-%05z-hhmm (0.000870 sec)
   passed   29/135  formatz-%05z-seconds (0.001055 sec)
   passed   30/135  formatz-%05z-threedigit (0.000480 sec)
   passed   31/135  formatz-%06:::z-hhmm (0.000852 sec)
   passed   32/135  formatz-%06:::z-seconds (0.000949 sec)
   passed   33/135  formatz-%06:::z-threedigit (0.000495 sec)
   passed   34/135  formatz-%06:z-hhmm (0.000839 sec)
   passed   35/135  formatz-%06:z-seconds (0.000926 sec)
   passed   36/135  formatz-%06:z-threedigit (0.000489 sec)
   passed   37/135  formatz-%06z-hhmm (0.000954 sec)
   passed   38/135  formatz-%06z-seconds (0.001083 sec)
   passed   39/135  formatz-%06z-threedigit (0.000492 sec)
   passed   40/135  formatz-%07:::z-hhmm (0.000957 sec)
   passed   41/135  formatz-%07:::z-seconds (0.001001 sec)
   passed   42/135  formatz-%07:::z-threedigit (0.000532 sec)
   passed   43/135  formatz-%07:z-hhmm (0.000938 sec)
   passed   44/135  formatz-%07:z-seconds (0.000979 sec)
   passed   45/135  formatz-%07:z-threedigit (0.000506 sec)
   passed   46/135  formatz-%09::z-hhmm (0.000963 sec)
   passed   47/135  formatz-%09::z-seconds (0.000970 sec)
   passed   48/135  formatz-%09::z-threedigit (0.000526 sec)
   passed   49/135  formatz-%0:::z-hhmm (0.000857 sec)
   passed   50/135  formatz-%0:::z-seconds (0.000980 sec)
   passed   51/135  formatz-%0:::z-threedigit (0.000534 sec)
   passed   52/135  formatz-%0::z-hhmm (0.000964 sec)
   passed   53/135  formatz-%0::z-seconds (0.001002 sec)
   passed   54/135  formatz-%0::z-threedigit (0.000543 sec)
   passed   55/135  formatz-%0:z-hhmm (0.000887 sec)
   passed   56/135  formatz-%0:z-seconds (0.000970 sec)
   passed   57/135  formatz-%0:z-threedigit (0.094870 sec)
   passed   58/135  formatz-%0z-hhmm (0.000828 sec)
   passed   59/135  formatz-%0z-seconds (0.000983 sec)
   passed   60/135  formatz-%0z-threedigit (0.000470 sec)
   passed   61/135  formatz-%10:::z-hhmm (0.001001 sec)
   passed   62/135  formatz-%10:::z-seconds (0.001000 sec)
   passed   63/135  formatz-%10:::z-threedigit (0.000493 sec)
   passed   64/135  formatz-%12:::z-hhmm (0.000864 sec)
   passed   65/135  formatz-%12:::z-seconds (0.001001 sec)
   passed   66/135  formatz-%12:::z-threedigit (0.000515 sec)
   passed   67/135  formatz-%12::z-hhmm (0.000959 sec)
   passed   68/135  formatz-%12::z-seconds (0.000998 sec)
   passed   69/135  formatz-%12::z-threedigit (0.000530 sec)
   passed   70/135  formatz-%12:z-hhmm (0.000921 sec)
   passed   71/135  formatz-%12:z-seconds (0.001011 sec)
   passed   72/135  formatz-%12:z-threedigit (0.000559 sec)
   passed   73/135  formatz-%12z-hhmm (0.000956 sec)
   passed   74/135  formatz-%12z-seconds (0.001112 sec)
   passed   75/135  formatz-%12z-threedigit (0.000514 sec)
   passed   76/135  formatz-%3:::z-hhmm (0.000850 sec)
   passed   77/135  formatz-%3:::z-seconds (0.000958 sec)
   passed   78/135  formatz-%3:::z-threedigit (0.000514 sec)
   passed   79/135  formatz-%5z-hhmm (0.000912 sec)
   passed   80/135  formatz-%5z-seconds (0.001076 sec)
   passed   81/135  formatz-%5z-threedigit (0.000507 sec)
   passed   82/135  formatz-%6:z-hhmm (0.000887 sec)
   passed   83/135  formatz-%6:z-seconds (0.000965 sec)
   passed   84/135  formatz-%6:z-threedigit (0.000530 sec)
   passed   85/135  formatz-%9::z-hhmm (0.000967 sec)
   passed   86/135  formatz-%9::z-seconds (0.001021 sec)
   passed   87/135  formatz-%9::z-threedigit (0.000549 sec)
   passed   88/135  formatz-%:::z-hhmm (0.096159 sec)
   passed   89/135  formatz-%:::z-seconds (0.000838 sec)
   passed   90/135  formatz-%:::z-threedigit (0.000442 sec)
   passed   91/135  formatz-%::z-hhmm (0.000822 sec)
   passed   92/135  formatz-%::z-seconds (0.000866 sec)
   passed   93/135  formatz-%::z-threedigit (0.000466 sec)
   passed   94/135  formatz-%:z-hhmm (0.000757 sec)
   passed   95/135  formatz-%:z-seconds (0.000848 sec)
   passed   96/135  formatz-%:z-threedigit (0.000466 sec)
   passed   97/135  formatz-%_12z-hhmm (0.001074 sec)
   passed   98/135  formatz-%_12z-seconds (0.001125 sec)
   passed   99/135  formatz-%_12z-threedigit (0.000527 sec)
   passed  100/135  formatz-%_7z-hhmm (0.000929 sec)
   passed  101/135  formatz-%_7z-seconds (0.001055 sec)
   passed  102/135  formatz-%_7z-threedigit (0.000501 sec)
   passed  103/135  formatz-%_z-hhmm (0.000903 sec)
   passed  104/135  formatz-%_z-seconds (0.001031 sec)
   passed  105/135  formatz-%_z-threedigit (0.000492 sec)
   passed  106/135  formatz-%z-spotcheck (0.000326 sec)
   passed  107/135  formatz-illegal-options (0.000445 sec)
   passed  108/135  time-stamp-custom-count (0.000538 sec)
   passed  109/135  time-stamp-custom-end (0.000311 sec)
   passed  110/135  time-stamp-custom-format-tabs-expand (0.000654 sec)
   passed  111/135  time-stamp-custom-inserts-lines (0.000468 sec)
   passed  112/135  time-stamp-custom-pattern (0.005087 sec)
   passed  113/135  time-stamp-custom-time-zone (0.000375 sec)
   passed  114/135  time-stamp-format-am-pm (0.000284 sec)
   passed  115/135  time-stamp-format-day-number-in-week (0.000231 sec)
   passed  116/135  time-stamp-format-day-of-month (0.000470 sec)
Test time-stamp-format-day-of-week backtrace:
  signal(ert-test-failed (((should (equal (time-stamp-string "%3a" ref
  ert-fail(((should (equal (time-stamp-string "%3a" ref-time1) Mon)) :
  #f(compiled-function () #<bytecode 0xbda32adea3e6e3b>)()
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name time-stamp-format-day-of-week :docume
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/time-stamp-tests" "-
  command-line()
  normal-top-level()
Test time-stamp-format-day-of-week condition:
    (ert-test-failed
     ((should
       (equal
	(time-stamp-string "%3a" ref-time1)
	Mon))
      :form
      (equal " Mo" "Mo")
      :value nil :explanation
      (arrays-of-different-length 3 2 " Mo" "Mo" first-mismatch-at 0)))
   FAILED  117/135  time-stamp-format-day-of-week (0.000415 sec)
   passed  118/135  time-stamp-format-hours-12 (0.000579 sec)
   passed  119/135  time-stamp-format-hours-24 (0.000581 sec)
   passed  120/135  time-stamp-format-ignored-modifiers (0.000306 sec)
   passed  121/135  time-stamp-format-minute (0.000419 sec)
   passed  122/135  time-stamp-format-month-name (0.000326 sec)
   passed  123/135  time-stamp-format-month-number (0.000418 sec)
   passed  124/135  time-stamp-format-multiple-conversions (0.000698 sec)
   passed  125/135  time-stamp-format-non-conversions (0.000174 sec)
   passed  126/135  time-stamp-format-non-date-conversions (0.000432 sec)
   passed  127/135  time-stamp-format-second (0.000424 sec)
Test time-stamp-format-string-width backtrace:
  signal(ert-test-failed (((should (equal (time-stamp-string "%#3a" re
  ert-fail(((should (equal (time-stamp-string "%#3a" ref-time3) SUN)) 
  #f(compiled-function () #<bytecode -0xd4dc00a92f30942>)()
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name time-stamp-format-string-width :docum
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/time-stamp-tests" "-
  command-line()
  normal-top-level()
Test time-stamp-format-string-width condition:
    (ert-test-failed
     ((should
       (equal
	(time-stamp-string "%#3a" ref-time3)
	SUN))
      :form
      (equal " SO" "SO")
      :value nil :explanation
      (arrays-of-different-length 3 2 " SO" "SO" first-mismatch-at 0)))
   FAILED  128/135  time-stamp-format-string-width (0.000539 sec)
   passed  129/135  time-stamp-format-time-zone-name (0.000231 sec)
   passed  130/135  time-stamp-format-time-zone-offset (0.000485 sec)
   passed  131/135  time-stamp-format-year-2digit (0.000393 sec)
   passed  132/135  time-stamp-format-year-4digit (0.000225 sec)
   passed  133/135  time-stamp-helper-safe-locals (0.000230 sec)
   passed  134/135  time-stamp-helper-string-defaults (0.000352 sec)
   passed  135/135  time-stamp-helper-zone-type-p (0.000208 sec)

Ran 135 tests, 133 results as expected, 2 unexpected (2021-09-17 20:46:21+0200, 5.091627 sec)

2 unexpected results:
   FAILED  time-stamp-format-day-of-week
   FAILED  time-stamp-format-string-width


^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-17 19:00             ` Federico Tedin
@ 2021-09-18  6:51               ` Eli Zaretskii
  2021-09-18 17:57                 ` Federico Tedin
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2021-09-18  6:51 UTC (permalink / raw)
  To: Federico Tedin; +Cc: phst, 49723-done

> From: Federico Tedin <federicotedin@gmail.com>
> Cc: phst@google.com,  49723@debbugs.gnu.org
> Date: Fri, 17 Sep 2021 21:00:08 +0200
> 
> > Thanks.  Did you run the test suite after applying the changes, and
> > did you see no regressions?  If you didn't yet run the test suite,
> > please be sure to run all of it, as the use of expand-file-name is
> > universal.
> 
> I hadn't, so I checked out master aa59d38c59 and applied my patch on top
> of it. I then ran "make check" and waited for a bit. There appears to be
> only two tests failing in test/lisp/time-stamp-tests.el
> ('time-stamp-format-day-of-week' and
> 'time-stamp-format-string-width'). Both seem to be unrelated to my
> change; maybe it's my system's strange combination of
> Spanish/English/German locale-related configurations (I'm attaching the
> log just in case). All other test files were run without problems.

Thanks.  I installed your changes, and I'm therefore closing this bug.

A few minor stylistic comments, for the future

 . the lines in the commit log message are too wide, they should be at
   most 66 characters, because we produce ChangeLog files from Girt
   logs, and ChangeLog files have the fill-column set to 74, which
   includes 9-column TAB (perhaps this means the fill-column setting
   in .dire-locals.el should be amended?)
 . please quote symbols in commit log messages rather than leaving
   them unquoted (this doesn't apply to symbols in parentheses that
   state the functions which were changed)
 . please try to establish whether the changes need to be described in
   the manual(s), and mark the NEWS entries accordingly (if you decide
   there's a need to describe in the manual, please also include a
   suitable change for that)

Thanks again for working on this.





^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-18  6:51               ` Eli Zaretskii
@ 2021-09-18 17:57                 ` Federico Tedin
  0 siblings, 0 replies; 18+ messages in thread
From: Federico Tedin @ 2021-09-18 17:57 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: phst, 49723-done

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

Noted! Thank you.

Eli Zaretskii <eliz@gnu.org> schrieb am Sa. 18. Sept. 2021 um 08:51:

> > From: Federico Tedin <federicotedin@gmail.com>
> > Cc: phst@google.com,  49723@debbugs.gnu.org
> > Date: Fri, 17 Sep 2021 21:00:08 +0200
> >
> > > Thanks.  Did you run the test suite after applying the changes, and
> > > did you see no regressions?  If you didn't yet run the test suite,
> > > please be sure to run all of it, as the use of expand-file-name is
> > > universal.
> >
> > I hadn't, so I checked out master aa59d38c59 and applied my patch on top
> > of it. I then ran "make check" and waited for a bit. There appears to be
> > only two tests failing in test/lisp/time-stamp-tests.el
> > ('time-stamp-format-day-of-week' and
> > 'time-stamp-format-string-width'). Both seem to be unrelated to my
> > change; maybe it's my system's strange combination of
> > Spanish/English/German locale-related configurations (I'm attaching the
> > log just in case). All other test files were run without problems.
>
> Thanks.  I installed your changes, and I'm therefore closing this bug.
>
> A few minor stylistic comments, for the future
>
>  . the lines in the commit log message are too wide, they should be at
>    most 66 characters, because we produce ChangeLog files from Girt
>    logs, and ChangeLog files have the fill-column set to 74, which
>    includes 9-column TAB (perhaps this means the fill-column setting
>    in .dire-locals.el should be amended?)
>  . please quote symbols in commit log messages rather than leaving
>    them unquoted (this doesn't apply to symbols in parentheses that
>    state the functions which were changed)
>  . please try to establish whether the changes need to be described in
>    the manual(s), and mark the NEWS entries accordingly (if you decide
>    there's a need to describe in the manual, please also include a
>    suitable change for that)
>
> Thanks again for working on this.
>

[-- Attachment #2: Type: text/html, Size: 2694 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable
  2021-09-16 19:13                 ` Federico Tedin
@ 2021-09-20 11:59                   ` Michael Albinus
  0 siblings, 0 replies; 18+ messages in thread
From: Michael Albinus @ 2021-09-20 11:59 UTC (permalink / raw)
  To: Federico Tedin; +Cc: phst, 49723

Federico Tedin <federicotedin@gmail.com> writes:

Hi Federico,

> Ouch, hadn't noticed those. I gave a look at the four I hadn't seen (gvfs,
> sh, smb and sudoedit) and it seems like all of them return expressions
> in the shape of:
>
>   (expand-file-name xyz)
>
> or:
>
>   (tramp-run-real-handler #'expand-file-name xyz)
>
> with xyz being a value derived from NAME or NAME and DIR. So in that
> case it seems like we would be covered by the changes in `expand-file-mode`.

Indeed. So there's no need for Tramp to do anything special.

Best regards, Michael.





^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-09-20 11:59 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-24 17:39 bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable Eli Zaretskii
2021-09-14 19:01 ` Federico Tedin
2021-09-14 19:24   ` Eli Zaretskii
2021-09-14 22:17     ` Federico Tedin
2021-09-16 12:25       ` Eli Zaretskii
2021-09-16 16:58         ` Federico Tedin
2021-09-16 18:12           ` Michael Albinus
2021-09-16 18:30             ` Eli Zaretskii
2021-09-16 18:44               ` Michael Albinus
2021-09-16 19:07                 ` Eli Zaretskii
2021-09-16 18:38             ` Federico Tedin
2021-09-16 18:51               ` Michael Albinus
2021-09-16 19:13                 ` Federico Tedin
2021-09-20 11:59                   ` Michael Albinus
2021-09-17  7:49           ` Eli Zaretskii
2021-09-17 19:00             ` Federico Tedin
2021-09-18  6:51               ` Eli Zaretskii
2021-09-18 17:57                 ` Federico Tedin

Code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).