Summary: | [libedit] bind command reads out of bounds and sends output to stderr | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | Base System | Reporter: | Mason Loring Bliss <mason> | ||||||||||||
Component: | bin | Assignee: | Jilles Tjoelker <jilles> | ||||||||||||
Status: | Closed FIXED | ||||||||||||||
Severity: | Affects Many People | CC: | bugs, corvid, jilles, kevans, naito.yuichiro, re, trasz | ||||||||||||
Priority: | --- | Keywords: | patch | ||||||||||||
Version: | 12.0-STABLE | ||||||||||||||
Hardware: | amd64 | ||||||||||||||
OS: | Any | ||||||||||||||
Attachments: |
|
CC'ing jilles@ for some triage as behavior is observed in sh(1). For added flavour: 10:58 < kevans91> It's a multi-release bug at this point 10:58 < mason> Not just 12? 10:58 < kevans91> repro's on this 11.2 box that's restoring stuff I notice a bug in libedit. The function map_bind() in lib/libedit/map.c assumes that the argv array ends with a NULL string pointer, but the documentation for el_parse() does not say this is required and the implementation of ct_decode_argv() does not make it such. As a result, memory out of bounds of the allocation is accessed. Since applications calling el_wparse() cannot be assumed to add the NULL sentinel, functions like map_bind() should be adjusted (there may be more places making this incorrect assumption). By the way, I don't think the approach of converting strings into wchar_t strings (that is, UTF-32, most of the time) should be repeated in new code. *** Bug 234690 has been marked as a duplicate of this bug. *** FWIW, an easy way to reproduce seems to be: $ bind "\e[3~" ed-delete-next-char $ bind Hi, I wrote a a patch that fixes the problem mentioned in comment #3. Just fixed to check the end of argv list by argc counts. The attached `libedit.patch` is a patch file for this. And I found another problem that built in 'bind' never redirects to a pipe and/or a file. The `bind` command always outputs messages to the terminal. This is because built in `bind` calls el_parse(3) and its outout file descriptor never be changed by pipe and/or redirects. The attached `sh.patch` fixes this problem. Could you see my patches and try them? I checked my patches work on FreeBSD-12.0R. Created attachment 200904 [details]
libedit.patch
Created attachment 200905 [details]
sh.patch
Created attachment 201153 [details]
libedit.patch
I found that this problem has already been fixed in NetBSD. See the following URL. http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libedit/chartype.c.diff?r1=1.30&r2=1.31&only_with_tag=MAIN&f=h I updated `libedit.patch` just regenerated for FreeBSD src tree. A commit references this bug: Author: jilles Date: Wed Jan 16 21:59:19 UTC 2019 New revision: 343105 URL: https://svnweb.freebsd.org/changeset/base/343105 Log: libedit: Avoid out of bounds read in 'bind' command This is CVS revision 1.31 from NetBSD lib/libedit/chartype.c: Make sure that argv is NULL terminated since functions like tty_stty rely on it to be so (Gerry Swinslow) This broke when the wide-character support was enabled in libedit. The conversion from multibyte to wide-character did not supply the apparently expected terminating NULL in the new argv array. PR: 233343 Submitted by: Yuichiro NAITO Obtained from: NetBSD MFC after: 1 week Changes: head/lib/libedit/chartype.c As for the sh(1) part, it looks like it should work but I don't really like that it's a workaround for a workaround: sh supplies its standard error as standard output to libedit, so that prompts are written to standard error as required by POSIX. The bind builtin would then temporarily undo this again. However, something is definitely required to prevent bind's output from staying in a stdio buffer indefinitely. It is also useful to report write errors like other builtins can. I will think about it for a while. Created attachment 201348 [details]
sh.patch
(In reply to Jilles Tjoelker from comment #12) Thanks for the comment. My understanding was a little wrong. I saw `bind` messages were always output to STDERR. So my patch can be reduced to just swap el's stderr and stdout temporarily. I updated 'sh.patch'. It seems ad hoc way, but I have no idea about better than this. A commit references this bug: Author: jilles Date: Thu Jan 24 22:34:30 UTC 2019 New revision: 343415 URL: https://svnweb.freebsd.org/changeset/base/343415 Log: MFC r343105: libedit: Avoid out of bounds read in 'bind' command This is CVS revision 1.31 from NetBSD lib/libedit/chartype.c: Make sure that argv is NULL terminated since functions like tty_stty rely on it to be so (Gerry Swinslow) This broke when the wide-character support was enabled in libedit. The conversion from multibyte to wide-character did not supply the apparently expected terminating NULL in the new argv array. PR: 233343 Changes: _U stable/12/ stable/12/lib/libedit/chartype.c A commit references this bug: Author: jilles Date: Fri Jan 25 22:52:49 UTC 2019 New revision: 343460 URL: https://svnweb.freebsd.org/changeset/base/343460 Log: MFC r343105: libedit: Avoid out of bounds read in 'bind' command This is CVS revision 1.31 from NetBSD lib/libedit/chartype.c: Make sure that argv is NULL terminated since functions like tty_stty rely on it to be so (Gerry Swinslow) This broke when the wide-character support was enabled in libedit. The conversion from multibyte to wide-character did not supply the apparently expected terminating NULL in the new argv array. PR: 233343 Changes: _U stable/11/ stable/11/lib/libedit/chartype.c A commit references this bug: Author: jilles Date: Tue Feb 19 21:27:31 UTC 2019 New revision: 344306 URL: https://svnweb.freebsd.org/changeset/base/344306 Log: sh: Send normal output from bind builtin to stdout PR: 233343 Submitted by: Yuichiro NAITO (original version) Changes: head/bin/sh/histedit.c head/bin/sh/output.c head/bin/sh/output.h A commit references this bug: Author: jilles Date: Wed Mar 27 21:53:45 UTC 2019 New revision: 345613 URL: https://svnweb.freebsd.org/changeset/base/345613 Log: MFC r344306: sh: Send normal output from bind builtin to stdout PR: 233343 Changes: _U stable/12/ stable/12/bin/sh/histedit.c stable/12/bin/sh/output.c stable/12/bin/sh/output.h A commit references this bug: Author: jilles Date: Wed Mar 27 22:09:35 UTC 2019 New revision: 345617 URL: https://svnweb.freebsd.org/changeset/base/345617 Log: MFC r344306: sh: Send normal output from bind builtin to stdout PR: 233343 Changes: _U stable/11/ stable/11/bin/sh/histedit.c stable/11/bin/sh/output.c stable/11/bin/sh/output.h *** Bug 240941 has been marked as a duplicate of this bug. *** |
Created attachment 199366 [details] lightly redacted core file from sh(1) bleb on Freenode #freebsd noted that he saw the sh(1) bind(1) when piping it into less(1). I could reproduce it, although it's sporadic. I've also seen it piping into more(1) and redirecting to a file. I see it both both running sh(1) and exec'ing it from my current shell. $ bind | more Segmentation fault (core dumped) I'm attaching the core, with some string subs in place to help obscure the system identity.