Bug 195918 - /bin/sh crash caused by a particular script
Summary: /bin/sh crash caused by a particular script
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 10.1-STABLE
Hardware: Any Any
: --- Affects Only Me
Assignee: Jilles Tjoelker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-12 11:08 UTC by Yuri Victorovich
Modified: 2014-12-14 18:33 UTC (History)
2 users (show)

See Also:
jilles: mfc-stable10+
jilles: mfc-stable9-
jilles: mfc-stable8-


Attachments
bin/sh with debug enable showing out of bounds array index (819 bytes, text/plain)
2014-12-13 01:16 UTC, Jason Unovitch
no flags Details
bin/sh with debug enabled and check for negative value (1.90 KB, text/plain)
2014-12-13 01:17 UTC, Jason Unovitch
no flags Details
diff showing a check for negative value (304 bytes, patch)
2014-12-13 01:20 UTC, Jason Unovitch
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuri Victorovich freebsd_committer freebsd_triage 2014-12-12 11:08:22 UTC
I accidentally hit this crash trying some (probably bash) example taken from online.

Crash is 100% reproducible.

Script might be invalid for sh, but it shouldn't cause a crash.


--- testcase ---
$ sh
$ echo b=${1985234857347568347:12:5}
Segmentation fault


(gdb) bt
#0  0x0000000000409675 in argstr ()
#1  0x0000000000408af6 in expandarg ()
#2  0x0000000000405f41 in evalcommand ()
#3  0x000000000040534f in evaltree ()
#4  0x0000000000410529 in cmdloop ()
#5  0x0000000000410392 in main ()
Comment 1 Jason Unovitch freebsd_committer freebsd_triage 2014-12-13 01:16:25 UTC
Created attachment 150520 [details]
bin/sh with debug enable showing out of bounds array index
Comment 2 Jason Unovitch freebsd_committer freebsd_triage 2014-12-13 01:17:58 UTC
Created attachment 150521 [details]
bin/sh with debug enabled and check for negative value
Comment 3 Jason Unovitch freebsd_committer freebsd_triage 2014-12-13 01:20:48 UTC
Created attachment 150522 [details]
diff showing a check for negative value
Comment 4 Jason Unovitch freebsd_committer freebsd_triage 2014-12-13 01:23:10 UTC
An interesting observation to add, I can trigger this on my amd64 box but not on my i386 router.  After further investigation, I found through using GDB on an old 9.1 VM with bin/sh compiled with debuging that expand.c runs atoi and uses the negative number it receives to read from an array index.  I've attached the diff but it's crude and I don't think this is the "right" solution but does prevent any seg faults and errors out cleanly with the bad substitution.

64 bit:

FreeBSD xts-bsd 10.1-RELEASE FreeBSD 10.1-RELEASE #0 r274401: Tue Nov 11 21:02:49 UTC 2014     root@releng1.nyi.freebsd.org:/usr/obj/usr/src/sys/GENERIC  amd64
jason@xts-bsd:/usr/src/bin/sh % sh
$ echo b=${1985234857347568347:12:5}
Segmentation fault

32 bit:

FreeBSD xts-rtr 10.1-RELEASE FreeBSD 10.1-RELEASE #0 r274562M: Sun Nov 16 07:37:32 UTC 2014     root@xts-bsd:/usr/obj/nanobsd.soekris/i386.i386/usr/src/sys/GENERIC  i386
jason@xts-rtr:~ % sh
$ echo b=${1985234857347568347:12:5}
${1985234857347568347:1...}: Bad substitution
Comment 5 Jilles Tjoelker freebsd_committer freebsd_triage 2014-12-13 15:02:54 UTC
I fixed this in 11-current a few months ago, in Subversion r268576, by using strtol() instead of atoi().
Comment 6 commit-hook freebsd_committer freebsd_triage 2014-12-14 18:31:12 UTC
A commit references this bug:

Author: jilles
Date: Sun Dec 14 18:30:31 UTC 2014
New revision: 275777
URL: https://svnweb.freebsd.org/changeset/base/275777

Log:
  MFC r268576: sh: Correctly handle positional parameters beyond INT_MAX on
  64-bit systems.

  Currently, there can be no more than INT_MAX positional parameters. Make
  sure to treat all higher ones as unset to avoid incorrect results and
  crashes.

  On 64-bit systems, our atoi() takes the low 32 bits of the strtol() and
  sign-extends them.

  On 32-bit systems, the call to atoi() returned INT_MAX for too high values
  and there is not enough address space for so many positional parameters, so
  there was no issue.

  PR:		195918

Changes:
_U  stable/10/
  stable/10/bin/sh/expand.c
  stable/10/bin/sh/tests/parameters/Makefile
  stable/10/bin/sh/tests/parameters/positional5.0