Bug 233343 - [libedit] bind command reads out of bounds
Summary: [libedit] bind command reads out of bounds
Status: Open
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
Keywords: patch
: 234690 (view as bug list)
Depends on:
Reported: 2018-11-20 01:05 UTC by Mason Loring Bliss
Modified: 2019-01-16 22:23 UTC (History)
6 users (show)

See Also:

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

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
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]
Comment 8 Yuichiro NAITO 2019-01-08 06:12:25 UTC
Created attachment 200905 [details]
Comment 9 Yuichiro NAITO 2019-01-15 05:09:17 UTC
Created attachment 201153 [details]
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.


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

  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

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.