* bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte @ 2018-01-06 11:39 Philipp 2018-01-20 12:02 ` Eli Zaretskii 0 siblings, 1 reply; 6+ messages in thread From: Philipp @ 2018-01-06 11:39 UTC (permalink / raw) To: 30005 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)) ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte 2018-01-06 11:39 bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte Philipp @ 2018-01-20 12:02 ` Eli Zaretskii 2018-01-22 22:25 ` Philipp Stephani 0 siblings, 1 reply; 6+ messages in thread From: Eli Zaretskii @ 2018-01-20 12:02 UTC (permalink / raw) To: Philipp; +Cc: 30005 > 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); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte 2018-01-20 12:02 ` Eli Zaretskii @ 2018-01-22 22:25 ` Philipp Stephani 2018-01-23 15:55 ` Eli Zaretskii 0 siblings, 1 reply; 6+ messages in thread From: Philipp Stephani @ 2018-01-22 22:25 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 30005 [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte 2018-01-22 22:25 ` Philipp Stephani @ 2018-01-23 15:55 ` Eli Zaretskii 2018-01-23 18:06 ` Richard Stallman 2018-02-03 21:48 ` Noam Postavsky 0 siblings, 2 replies; 6+ messages in thread From: Eli Zaretskii @ 2018-01-23 15:55 UTC (permalink / raw) To: Philipp Stephani; +Cc: 30005 > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte 2018-01-23 15:55 ` Eli Zaretskii @ 2018-01-23 18:06 ` Richard Stallman 2018-02-03 21:48 ` Noam Postavsky 1 sibling, 0 replies; 6+ messages in thread From: Richard Stallman @ 2018-01-23 18:06 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 30005, p.stephani2 [[[ 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte 2018-01-23 15:55 ` Eli Zaretskii 2018-01-23 18:06 ` Richard Stallman @ 2018-02-03 21:48 ` Noam Postavsky 1 sibling, 0 replies; 6+ messages in thread From: Noam Postavsky @ 2018-02-03 21:48 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 30005, Philipp Stephani 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-03 21:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-01-06 11:39 bug#30005: 27.0.50; call-interactively doesn't work correctly if the interactive specification has an embedded null byte Philipp 2018-01-20 12:02 ` Eli Zaretskii 2018-01-22 22:25 ` Philipp Stephani 2018-01-23 15:55 ` Eli Zaretskii 2018-01-23 18:06 ` Richard Stallman 2018-02-03 21:48 ` Noam Postavsky
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.