Bug 233343 - [libedit] bind command reads out of bounds and sends output to stderr
Summary: [libedit] bind command reads out of bounds and sends output to stderr
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 12.0-STABLE
Hardware: amd64 Any
: --- Affects Many People
Assignee: Jilles Tjoelker
URL:
Keywords: patch
: 234690 240941 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-11-20 01:05 UTC by Mason Loring Bliss
Modified: 2019-10-05 14:10 UTC (History)
7 users (show)

See Also:


Attachments
lightly redacted core file from sh(1) (30.69 KB, application/x-xz)
2018-11-20 01:05 UTC, Mason Loring Bliss
no flags Details
libedit.patch (1.90 KB, patch)
2019-01-08 06:11 UTC, Yuichiro NAITO
no flags Details | Diff
sh.patch (1.98 KB, patch)
2019-01-08 06:12 UTC, Yuichiro NAITO
no flags Details | Diff
libedit.patch (618 bytes, patch)
2019-01-15 05:09 UTC, Yuichiro NAITO
no flags Details | Diff
sh.patch (984 bytes, patch)
2019-01-23 01:56 UTC, Yuichiro NAITO
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mason Loring Bliss 2018-11-20 01:05:44 UTC
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.
Comment 1 Kyle Evans freebsd_committer 2018-11-26 15:55:29 UTC
CC'ing jilles@ for some triage as behavior is observed in sh(1).
Comment 2 Mason Loring Bliss 2018-11-26 16:00:47 UTC
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
Comment 3 Jilles Tjoelker freebsd_committer 2018-11-26 23:31:23 UTC
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.
Comment 4 Jilles Tjoelker freebsd_committer 2019-01-07 13:15:16 UTC
*** Bug 234690 has been marked as a duplicate of this bug. ***
Comment 5 Edward Tomasz Napierala freebsd_committer 2019-01-07 13:21:47 UTC
FWIW, an easy way to reproduce seems to be:

$ bind "\e[3~" ed-delete-next-char
$ bind
Comment 6 Yuichiro NAITO 2019-01-08 06:11:23 UTC
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.
Comment 7 Yuichiro NAITO 2019-01-08 06:11:52 UTC
Created attachment 200904 [details]
libedit.patch
Comment 8 Yuichiro NAITO 2019-01-08 06:12:25 UTC
Created attachment 200905 [details]
sh.patch
Comment 9 Yuichiro NAITO 2019-01-15 05:09:17 UTC
Created attachment 201153 [details]
libedit.patch
Comment 10 Yuichiro NAITO 2019-01-15 05:09:38 UTC
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.
Comment 11 commit-hook freebsd_committer 2019-01-16 21:59:58 UTC
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
Comment 12 Jilles Tjoelker freebsd_committer 2019-01-16 22:23:18 UTC
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.
Comment 13 Yuichiro NAITO 2019-01-23 01:56:14 UTC
Created attachment 201348 [details]
sh.patch
Comment 14 Yuichiro NAITO 2019-01-23 01:58:19 UTC
(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.
Comment 15 commit-hook freebsd_committer 2019-01-24 22:35:31 UTC
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
Comment 16 commit-hook freebsd_committer 2019-01-25 22:53:38 UTC
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
Comment 17 commit-hook freebsd_committer 2019-02-19 21:28:34 UTC
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
Comment 18 commit-hook freebsd_committer 2019-03-27 21:54:19 UTC
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
Comment 19 commit-hook freebsd_committer 2019-03-27 22:10:35 UTC
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
Comment 20 Jilles Tjoelker freebsd_committer 2019-10-05 14:10:00 UTC
*** Bug 240941 has been marked as a duplicate of this bug. ***