bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte

bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte

[Philipp]

For example, when evaluating

(call-interactively (lambda (a b) (interactive "sa\0b\ns")))

the prompt is only "a" and a `wrong-number-of-argument' signal is
raised.  This is because `call-interactively' copies the interactive
specification to a C string, ignoring embedded nulls.


In GNU Emacs 27.0.50 (build 4, x86_64-apple-darwin17.3.0, NS appkit-1561.20 Version 10.13.2 (Build 17C88))
 of 2018-01-02 built on p
Repository revision: 1330afd3debc5a0d5d7d58a5db3f73f84b04be26
Windowing system distributor 'Apple', version 10.3.1561
System Description:  Mac OS X 10.13.2

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured features:
NOTIFY ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS JSON

Important settings:
  value of $LANG: de_DE.UTF-8
  locale-coding-system: utf-8-unix

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
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils elec-pair time-date
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel term/ns-win ns-win ucs-normalize mule-util 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
menu-bar rfn-eshadow isearch timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core term/tty-colors frame 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 minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote kqueue cocoa ns
multi-tty make-network-process emacs)

Memory information:
((conses 16 204548 10161)
 (symbols 48 20150 1)
 (miscs 40 43 166)
 (strings 32 28872 1694)
 (string-bytes 1 770691)
 (vectors 16 35128)
 (vector-slots 8 719016 13106)
 (floats 8 48 68)
 (intervals 56 199 0)
 (buffers 992 11))

bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte

[Eli Zaretskii]

> From: Philipp <p.stephani2@gmail.com>
> Date: Sat, 06 Jan 2018 12:39:43 +0100
> 
> (call-interactively (lambda (a b) (interactive "sa\0b\ns")))
> 
> the prompt is only "a" and a `wrong-number-of-argument' signal is
> raised.  This is because `call-interactively' copies the interactive
> specification to a C string, ignoring embedded nulls.

No, it copies the spec in its entirety, including embedded null bytes,
but then _processes_ the result as a C string, taking the first null
byte as the end of the string.

Does the patch below look right, and give good results?

diff --git a/src/callint.c b/src/callint.c
index 2253cdf..3d2ed00 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -288,7 +288,8 @@ invoke it.  If KEYS is omitted or nil, the return value of
   ptrdiff_t next_event;
 
   Lisp_Object prefix_arg;
-  char *string;
+  char *string, *string_end;
+  ptrdiff_t string_len;
   const char *tem;
 
   /* If varies[i] > 0, the i'th argument shouldn't just have its value
@@ -396,6 +397,8 @@ invoke it.  If KEYS is omitted or nil, the return value of
   /* SPECS is set to a string; use it as an interactive prompt.
      Copy it so that STRING will be valid even if a GC relocates SPECS.  */
   SAFE_ALLOCA_STRING (string, specs);
+  string_len = SBYTES (specs);
+  string_end = string + string_len;
 
   /* Here if function specifies a string to control parsing the defaults.  */
 
@@ -418,7 +421,7 @@ invoke it.  If KEYS is omitted or nil, the return value of
 	      if (!NILP (record_flag))
 		{
 		  char *p = string;
-		  while (*p)
+		  while (p < string_end)
 		    {
 		      if (! (*p == 'r' || *p == 'p' || *p == 'P'
 			     || *p == '\n'))
@@ -469,7 +472,7 @@ invoke it.  If KEYS is omitted or nil, the return value of
      `funcall-interactively') plus the number of arguments the interactive spec
      would have us give to the function.  */
   tem = string;
-  for (nargs = 2; *tem; )
+  for (nargs = 2; tem < string_end; )
     {
       /* 'r' specifications ("point and mark as 2 numeric args")
 	 produce *two* arguments.  */
@@ -477,7 +480,7 @@ invoke it.  If KEYS is omitted or nil, the return value of
 	nargs += 2;
       else
 	nargs++;
-      tem = strchr (tem, '\n');
+      tem = memchr (tem, '\n', string_len - (tem - string));
       if (tem)
 	++tem;
       else
@@ -503,9 +506,12 @@ invoke it.  If KEYS is omitted or nil, the return value of
     specbind (Qenable_recursive_minibuffers, Qt);
 
   tem = string;
-  for (i = 2; *tem; i++)
+  for (i = 2; tem < string_end; i++)
     {
-      visargs[1] = make_string (tem + 1, strcspn (tem + 1, "\n"));
+      char *pnl = memchr (tem + 1, '\n', string_len - (tem + 1 - string));
+      ptrdiff_t sz = pnl ? pnl - (tem + 1) : string_end - (tem + 1);
+
+      visargs[1] = make_string (tem + 1, sz);
       callint_message = Fformat_message (i - 1, visargs + 1);
 
       switch (*tem)
@@ -781,7 +787,7 @@ invoke it.  If KEYS is omitted or nil, the return value of
 	  {
 	    /* How many bytes are left unprocessed in the specs string?
 	       (Note that this excludes the trailing null byte.)  */
-	    ptrdiff_t bytes_left = SBYTES (specs) - (tem - string);
+	    ptrdiff_t bytes_left = string_len - (tem - string);
 	    unsigned letter;
 
 	    /* If we have enough bytes left to treat the sequence as a
@@ -803,9 +809,9 @@ invoke it.  If KEYS is omitted or nil, the return value of
       if (NILP (visargs[i]) && STRINGP (args[i]))
 	visargs[i] = args[i];
 
-      tem = strchr (tem, '\n');
+      tem = memchr (tem, '\n', string_len - (tem - string));
       if (tem) tem++;
-      else tem = "";
+      else tem = string_end;
     }
   unbind_to (speccount, Qnil);
 

bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Sa., 20. Jan. 2018 um 13:03 Uhr:

> > From: Philipp <p.stephani2@gmail.com>
> > Date: Sat, 06 Jan 2018 12:39:43 +0100
> >
> > (call-interactively (lambda (a b) (interactive "sa\0b\ns")))
> >
> > the prompt is only "a" and a `wrong-number-of-argument' signal is
> > raised.  This is because `call-interactively' copies the interactive
> > specification to a C string, ignoring embedded nulls.
>
> No, it copies the spec in its entirety, including embedded null bytes,
> but then _processes_ the result as a C string, taking the first null
> byte as the end of the string.
>
> Does the patch below look right, and give good results?
>

Yes, thanks. Just some minor nits inline to make the code shorter.


>
> diff --git a/src/callint.c b/src/callint.c
> index 2253cdf..3d2ed00 100644
> --- a/src/callint.c
> +++ b/src/callint.c
> @@ -288,7 +288,8 @@ invoke it.  If KEYS is omitted or nil, the return
> value of
>    ptrdiff_t next_event;
>
>    Lisp_Object prefix_arg;
> -  char *string;
> +  char *string, *string_end;
> +  ptrdiff_t string_len;
>

I think these days (where we require C99) we always declare variables when
we first use them.


>    const char *tem;
>
>    /* If varies[i] > 0, the i'th argument shouldn't just have its value
> @@ -396,6 +397,8 @@ invoke it.  If KEYS is omitted or nil, the return
> value of
>    /* SPECS is set to a string; use it as an interactive prompt.
>       Copy it so that STRING will be valid even if a GC relocates SPECS.
> */
>    SAFE_ALLOCA_STRING (string, specs);
> +  string_len = SBYTES (specs);
> +  string_end = string + string_len;
>
>    /* Here if function specifies a string to control parsing the
> defaults.  */
>
> @@ -418,7 +421,7 @@ invoke it.  If KEYS is omitted or nil, the return
> value of
>               if (!NILP (record_flag))
>                 {
>                   char *p = string;
> -                 while (*p)
> +                 while (p < string_end)
>                     {
>                       if (! (*p == 'r' || *p == 'p' || *p == 'P'
>                              || *p == '\n'))
> @@ -469,7 +472,7 @@ invoke it.  If KEYS is omitted or nil, the return
> value of
>       `funcall-interactively') plus the number of arguments the
> interactive spec
>       would have us give to the function.  */
>    tem = string;
> -  for (nargs = 2; *tem; )
> +  for (nargs = 2; tem < string_end; )
>      {
>        /* 'r' specifications ("point and mark as 2 numeric args")
>          produce *two* arguments.  */
> @@ -477,7 +480,7 @@ invoke it.  If KEYS is omitted or nil, the return
> value of
>         nargs += 2;
>        else
>         nargs++;
> -      tem = strchr (tem, '\n');
> +      tem = memchr (tem, '\n', string_len - (tem - string));
>

You can write the third argument as string_end - tem.


>        if (tem)
>         ++tem;
>        else
> @@ -503,9 +506,12 @@ invoke it.  If KEYS is omitted or nil, the return
> value of
>      specbind (Qenable_recursive_minibuffers, Qt);
>
>    tem = string;
> -  for (i = 2; *tem; i++)
> +  for (i = 2; tem < string_end; i++)
>      {
> -      visargs[1] = make_string (tem + 1, strcspn (tem + 1, "\n"));
> +      char *pnl = memchr (tem + 1, '\n', string_len - (tem + 1 - string));
>

Here you can write the third argument as string_end - (tem + 1).


> +      ptrdiff_t sz = pnl ? pnl - (tem + 1) : string_end - (tem + 1);
>

You can write the RHS as (pnl ? pnl : string_end) - (tem + 1).


> +
> +      visargs[1] = make_string (tem + 1, sz);
>        callint_message = Fformat_message (i - 1, visargs + 1);
>
>        switch (*tem)
> @@ -781,7 +787,7 @@ invoke it.  If KEYS is omitted or nil, the return
> value of
>           {
>             /* How many bytes are left unprocessed in the specs string?
>                (Note that this excludes the trailing null byte.)  */
> -           ptrdiff_t bytes_left = SBYTES (specs) - (tem - string);
> +           ptrdiff_t bytes_left = string_len - (tem - string);
>

Same (string-end - tem).


>             unsigned letter;
>
>             /* If we have enough bytes left to treat the sequence as a
> @@ -803,9 +809,9 @@ invoke it.  If KEYS is omitted or nil, the return
> value of
>        if (NILP (visargs[i]) && STRINGP (args[i]))
>         visargs[i] = args[i];
>
> -      tem = strchr (tem, '\n');
> +      tem = memchr (tem, '\n', string_len - (tem - string));
>

again, string_end - tem.


>        if (tem) tem++;
> -      else tem = "";
> +      else tem = string_end;
>      }
>    unbind_to (speccount, Qnil);
>
>

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

bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Mon, 22 Jan 2018 22:25:39 +0000
> Cc: 30005@debbugs.gnu.org
> 
>  Does the patch below look right, and give good results?
> 
> Yes, thanks. Just some minor nits inline to make the code shorter.

Thanks for the review.  I eventually pushed the changes as presented
here, for the reasons I explain below.

>     Lisp_Object prefix_arg;
>  -  char *string;
>  +  char *string, *string_end;
>  +  ptrdiff_t string_len;
> 
> I think these days (where we require C99) we always declare variables when we first use them.

That's not my understanding of the preferred style.  My understanding,
and what I do in practice, is that variables used only in a small
portion of a function should be declared before the use, so that all
the references to those variables are localized to the fragment where
they are used.  By contrast, in this case these variables are used all
over the function, so it makes much less sense to delay their
declaration.

>  -      tem = strchr (tem, '\n');
>  +      tem = memchr (tem, '\n', string_len - (tem - string));
> 
> You can write the third argument as string_end - tem.

Yes, but IMO the above is easier to convince the reader that the code
is correct, and an optimizing compiler will produce the same code from
both.

>  -      visargs[1] = make_string (tem + 1, strcspn (tem + 1, "\n"));
>  +      char *pnl = memchr (tem + 1, '\n', string_len - (tem + 1 - string));
> 
> Here you can write the third argument as string_end - (tem + 1).

Same here: I find the code I used easier to understand and verify its
correctness.

>  +      ptrdiff_t sz = pnl ? pnl - (tem + 1) : string_end - (tem + 1);
> 
> You can write the RHS as (pnl ? pnl : string_end) - (tem + 1).

Same here.

Thanks.

bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > That's not my understanding of the preferred style.  My understanding,
  > and what I do in practice, is that variables used only in a small
  > portion of a function should be declared before the use, so that all
  > the references to those variables are localized to the fragment where
  > they are used.  By contrast, in this case these variables are used all
  > over the function, so it makes much less sense to delay their
  > declaration.

I agree.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte

[Noam Postavsky]

tags 30005 fixed
close 30005 27.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philipp Stephani <p.stephani2@gmail.com>
>> Date: Mon, 22 Jan 2018 22:25:39 +0000
>> Cc: 30005@debbugs.gnu.org
>> 
>>  Does the patch below look right, and give good results?
>> 
>> Yes, thanks. Just some minor nits inline to make the code shorter.
>
> Thanks for the review.  I eventually pushed the changes

Closing.

[1: 6d836771da]: 2018-01-23 17:48:08 +0200
  Support null characters in interactive specs
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=6d836771da7e9a6a67fcd18e52dd16de1cdc154e