bug#56469: 29.0.50; Unibyte dir in directory_files_internal

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Package: Emacs
Version: 29.0.50


If you have a directory named "/tmp/\303a" with a file named "fée"
inside, then (directory-files "/tmp/\303a" 'full) is likely to return
a funny string which is multibyte but contains an invalid
utf-8 sequence (its bytes spell "/tmp/\303a/f\303\251e").
That strings seems to be printed as "/tmp/¡/fée" which corresponds
to "/tmp/\303\241/f\303\251e".

Such a string with an invalid UTF-8 sequence is handled quite graciously
by Emacs, so I wasn't able to get an actual crash out of it, but it's
still something we should avoid.

I suggest the patch below.  In a comment I suggest we don't try to use
unibyte strings when a multibyte string would work as well.  This is
because for those ASCII-only strings, it's cheaper to test bytes==chars
to (re)discover that they are ASCII-only (when they're multibyte) than
having to loop through the bytes (when they're unibyte).


        Stefan


diff --git a/src/dired.c b/src/dired.c
index 6bb8c2fcb9f..33ddfafd8e7 100644
--- a/src/dired.c
+++ b/src/dired.c
@@ -219,6 +219,13 @@ directory_files_internal (Lisp_Object directory, Lisp_Object full,
     }
 #endif
 
+  if (!NILP (full) && !STRING_MULTIBYTE (directory))
+    { /* We will be concatenating 'directory' with local file name.
+         We always decode local file names, so in order to safely concatenate
+         them we need 'directory' to be multibyte.  */
+      directory = Fstring_to_multibyte (directory);
+    }
+
   ptrdiff_t directory_nbytes = SBYTES (directory);
   re_match_object = Qt;
 
@@ -263,9 +270,10 @@ directory_files_internal (Lisp_Object directory, Lisp_Object full,
 	  ptrdiff_t name_nbytes = SBYTES (name);
 	  ptrdiff_t nbytes = directory_nbytes + needsep + name_nbytes;
 	  ptrdiff_t nchars = SCHARS (directory) + needsep + SCHARS (name);
-	  finalname = make_uninit_multibyte_string (nchars, nbytes);
-	  if (nchars == nbytes)
-	    STRING_SET_UNIBYTE (finalname);
+	  /* FIXME: Why not make them all multibyte?  */
+	  finalname = (nchars == nbytes)
+	              ? make_uninit_string (nchars, nbytes)
+	              : make_uninit_multibyte_string (nchars, nbytes);
 	  memcpy (SDATA (finalname), SDATA (directory), directory_nbytes);
 	  if (needsep)
 	    SSET (finalname, directory_nbytes, DIRECTORY_SEP);

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Eli Zaretskii]

> Date: Sat, 09 Jul 2022 13:44:52 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> If you have a directory named "/tmp/\303a" with a file named "fée"
> inside, then (directory-files "/tmp/\303a" 'full) is likely to return
> a funny string which is multibyte but contains an invalid
> utf-8 sequence (its bytes spell "/tmp/\303a/f\303\251e").
> That strings seems to be printed as "/tmp/¡/fée" which corresponds
> to "/tmp/\303\241/f\303\251e".
> 
> Such a string with an invalid UTF-8 sequence is handled quite graciously
> by Emacs, so I wasn't able to get an actual crash out of it, but it's
> still something we should avoid.
> 
> I suggest the patch below.  In a comment I suggest we don't try to use
> unibyte strings when a multibyte string would work as well.  This is
> because for those ASCII-only strings, it's cheaper to test bytes==chars
> to (re)discover that they are ASCII-only (when they're multibyte) than
> having to loop through the bytes (when they're unibyte).

Please bootstrap Emacs in a directory with such a name, and if that
works, I'm okay with installing this change.

Thanks.

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I suggest the patch below.  In a comment I suggest we don't try to use
>> unibyte strings when a multibyte string would work as well.  This is
>> because for those ASCII-only strings, it's cheaper to test bytes==chars
>> to (re)discover that they are ASCII-only (when they're multibyte) than
>> having to loop through the bytes (when they're unibyte).
>
> Please bootstrap Emacs in a directory with such a name, and if that
> works, I'm okay with installing this change.

Just to clarify: by "this change" you refer to the change in the patch
or the change suggested in the comment?


        Stefan

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 56469@debbugs.gnu.org
> Date: Sat, 09 Jul 2022 14:20:37 -0400
> 
> >> I suggest the patch below.  In a comment I suggest we don't try to use
> >> unibyte strings when a multibyte string would work as well.  This is
> >> because for those ASCII-only strings, it's cheaper to test bytes==chars
> >> to (re)discover that they are ASCII-only (when they're multibyte) than
> >> having to loop through the bytes (when they're unibyte).
> >
> > Please bootstrap Emacs in a directory with such a name, and if that
> > works, I'm okay with installing this change.
> 
> Just to clarify: by "this change" you refer to the change in the patch
> or the change suggested in the comment?

I meant the patch.  The comment I didn't understand at all.  It seemed
to be unrelated to the code and the change you were proposing.

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Please bootstrap Emacs in a directory with such a name, and if that
> works, I'm okay with installing this change.

Pushed, thanks.

W.r.t to the comment, it's indeed unrelated to the patch (other than
the fact that it touches the same code).  The question is when we do:

	  finalname = (nchars == nbytes)
	              ? make_uninit_string (nbytes)
	              : make_uninit_multibyte_string (nchars, nbytes);

the actual bytes are "decoded" (i.e. in our internal UTF-8 encoding), so
(nchars == nbytes) checks whether its "pure ASCII" or not and if it's
pure ASCII we return a unibyte string.

Our file-name manipulation routines always consider unibyte-ASCII and
multibyte-ASCII as "equivalent", and indeed DECODE_FILE and ENCODE_FILE
take advantage of that so as to return their argument as-is when it's
all-ASCII so as to avoid allocating a string unnecessarily.

So in the above code snippet, when the string is all-ASCII, we actually
have a choice, and both a unibyte string and a multibyte string should
work.  Currently in that case we return a unibyte string, but I think in
such cases we're better off returning a multibyte string because the
subsequent "all-ASCII" test (that DE/ENCODE_FILE will perform when we
pass that filename to some further operation) will be more efficient
(it's a constant-time (nchars == nbytes) test whereas when the string is
unibyte it requires looking at each and every byte).

IOW, while it makes sense to return a "decoded unibyte" string from
DECODE_FILE in order to avoid an allocation, I don't think it makes
sense to return such a "decoded unibyte" string when we have to allocate
a new string anyway.


        Stefan

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 56469@debbugs.gnu.org
> Date: Sun, 10 Jul 2022 10:23:28 -0400
> 
> W.r.t to the comment, it's indeed unrelated to the patch (other than
> the fact that it touches the same code).  The question is when we do:
> 
> 	  finalname = (nchars == nbytes)
> 	              ? make_uninit_string (nbytes)
> 	              : make_uninit_multibyte_string (nchars, nbytes);
> 
> the actual bytes are "decoded" (i.e. in our internal UTF-8 encoding), so
> (nchars == nbytes) checks whether its "pure ASCII" or not and if it's
> pure ASCII we return a unibyte string.

I don't think this is true, because early during startup we don't yet
have the coding-systems set up, and so the file names are unibyte and
undecoded.  So that place in dired.c doesn't only handle ASCII when it
sees that ncahrs == nbytes.

> So in the above code snippet, when the string is all-ASCII, we actually
> have a choice, and both a unibyte string and a multibyte string should
> work.  Currently in that case we return a unibyte string, but I think in
> such cases we're better off returning a multibyte string because the
> subsequent "all-ASCII" test (that DE/ENCODE_FILE will perform when we
> pass that filename to some further operation) will be more efficient
> (it's a constant-time (nchars == nbytes) test whereas when the string is
> unibyte it requires looking at each and every byte).
> 
> IOW, while it makes sense to return a "decoded unibyte" string from
> DECODE_FILE in order to avoid an allocation, I don't think it makes
> sense to return such a "decoded unibyte" string when we have to allocate
> a new string anyway.

I'm not necessarily opposed to decide that ASCII strings should be
multibyte, but doing so for file names will need careful auditing of
the sources with the startup process in mind.

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii [2022-07-10 17:32:17] wrote:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: 56469@debbugs.gnu.org
>> Date: Sun, 10 Jul 2022 10:23:28 -0400
>> 
>> W.r.t to the comment, it's indeed unrelated to the patch (other than
>> the fact that it touches the same code).  The question is when we do:
>> 
>> 	  finalname = (nchars == nbytes)
>> 	              ? make_uninit_string (nbytes)
>> 	              : make_uninit_multibyte_string (nchars, nbytes);
>> 
>> the actual bytes are "decoded" (i.e. in our internal UTF-8 encoding), so
>> (nchars == nbytes) checks whether its "pure ASCII" or not and if it's
>> pure ASCII we return a unibyte string.
>
> I don't think this is true, because early during startup we don't yet
> have the coding-systems set up, and so the file names are unibyte and
> undecoded.  So that place in dired.c doesn't only handle ASCII when it
> sees that ncahrs == nbytes.

Hmm... the early startup is actually not a worry here (according to my
tests `directory_files_internal` is first called when we get to
native-compile the macroexp/bytecomp, at which point all our coding
systems have been setup).

But indeed, if the file name coding system is something like `binary`,
DECODE_FILE will always return a unibyte string, so we may have non-ASCII
bytes when (nchars == nbytes).
Thanks, I'll update the comment accordingly.


        Stefan

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 56469@debbugs.gnu.org
> Date: Sun, 10 Jul 2022 10:58:30 -0400
> 
> Eli Zaretskii [2022-07-10 17:32:17] wrote:
> 
> > I don't think this is true, because early during startup we don't yet
> > have the coding-systems set up, and so the file names are unibyte and
> > undecoded.  So that place in dired.c doesn't only handle ASCII when it
> > sees that ncahrs == nbytes.
> 
> Hmm... the early startup is actually not a worry here (according to my
> tests `directory_files_internal` is first called when we get to
> native-compile the macroexp/bytecomp, at which point all our coding
> systems have been setup).

That could be the situation _today_, but that's just sheer luck (or
lack thereof).  In general, all the file-handling code we have in
fileio.c and dired.c should be equally prepared to handle unibyte
non-ASCII file names and multibyte file names, because we may need
that any time.  When we make changes in Emacs, we shouldn't be worried
whether those changes could cause some dired.c code be called early on
during Emacs startup.

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> That could be the situation _today_, but that's just sheer luck (or
> lack thereof).  In general, all the file-handling code we have in
> fileio.c and dired.c should be equally prepared to handle unibyte
> non-ASCII file names and multibyte file names, because we may need
> that any time.  When we make changes in Emacs, we shouldn't be worried
> whether those changes could cause some dired.c code be called early on
> during Emacs startup.

Agreed.  In the updated comment I noted that we have a bug when we do

    (let ((file-name-coding-system 'binary))
      (directory-files "/tmp/été/" 'full)

because we'll be concatenating the multibyte string "/tmp/été/" with
the undecoded unibyte strings of the names of files in that directory.


        Stefan

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 56469@debbugs.gnu.org
> Date: Sun, 10 Jul 2022 11:19:22 -0400
> 
> In the updated comment I noted that we have a bug when we do
> 
>     (let ((file-name-coding-system 'binary))
>       (directory-files "/tmp/été/" 'full)
> 
> because we'll be concatenating the multibyte string "/tmp/été/" with
> the undecoded unibyte strings of the names of files in that directory.

I don't think file-name related stuff can work in Emacs when
file-name-coding-system is set to an arbitrary value not reflecting
the reality.  Why would we want to support such cases?

(But I don't object to the comment.)

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I don't think file-name related stuff can work in Emacs when
> file-name-coding-system is set to an arbitrary value not reflecting
> the reality.

I'd tend to agree (tho 'binary' does sound like a valid value which
should work in all cases under GNU/Linux).

I'm just a bit annoyed at the idea that ELisp code can end up
constructing a multibyte string whose bytes contain invalid utf-8
sequences, because I suspect we may end up with a core dump somewhere in
such a circumstance.


        Stefan

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 56469@debbugs.gnu.org
> Date: Sun, 10 Jul 2022 18:13:39 -0400
> 
> > I don't think file-name related stuff can work in Emacs when
> > file-name-coding-system is set to an arbitrary value not reflecting
> > the reality.
> 
> I'd tend to agree (tho 'binary' does sound like a valid value which
> should work in all cases under GNU/Linux).

'binary' exactly means that you end up with unibyte strings and with
raw bytes in multibyte strings.

> I'm just a bit annoyed at the idea that ELisp code can end up
> constructing a multibyte string whose bytes contain invalid utf-8
> sequences, because I suspect we may end up with a core dump somewhere in
> such a circumstance.

Emacs should cope with this without dumping core, but the resulting
file names might not be readable by humans nor friendly to other
programs.

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> (But I don't object to the comment.)

Skimming this bug report lightly, it seems like the proposed patch was
applied, but then the discussion continued.  It's not clear to me
whether there's more to be done here -- should this report be closed?

bug#56469: 29.0.50; Unibyte dir in directory_files_internal

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  56469@debbugs.gnu.org
> Date: Mon, 05 Sep 2022 21:21:36 +0200
> 
> Skimming this bug report lightly, it seems like the proposed patch was
> applied, but then the discussion continued.  It's not clear to me
> whether there's more to be done here -- should this report be closed?

Yes, I think so.  Done.