>From c4863b9ab18fd4a25679182d8045f8ef18efccd1 Mon Sep 17 00:00:00 2001 From: Assaf Gordon Date: Fri, 27 Jul 2018 02:19:41 -0600 Subject: [PATCH 2/2] sed: fix heap buffer overflow from multiline EOL regex optimization sed would access invalid memory when matching EOF combined with s///n flag: $ yes 0 | fmt -w 40 | head -n2 | valgrind sed 'N;s/$//2m' ==13131== Conditional jump or move depends on uninitialised value(s) ==13131== at 0x4C3002B: memchr (vg_replace_strmem.c:883) ==13131== by 0x1120BD: match_regex (regexp.c:286) ==13131== by 0x110736: do_subst (execute.c:1101) ==13131== by 0x1115D3: execute_program (execute.c:1591) ==13131== by 0x111A4C: process_files (execute.c:1774) ==13131== by 0x112E1C: main (sed.c:405) ==13131== ==13131== Invalid read of size 1 ==13131== at 0x4C30027: memchr (vg_replace_strmem.c:883) ==13131== by 0x1120BD: match_regex (regexp.c:286) ==13131== by 0x110736: do_subst (execute.c:1101) ==13131== by 0x1115D3: execute_program (execute.c:1591) ==13131== by 0x111A4C: process_files (execute.c:1774) ==13131== by 0x112E1C: main (sed.c:405) ==13131== Address 0x55ec765 is 0 bytes after a block of size 101 alloc'd ==13131== at 0x4C2DDCF: realloc (vg_replace_malloc.c:785) ==13131== by 0x113BA2: ck_realloc (utils.c:418) ==13131== by 0x10E682: resize_line (execute.c:154) ==13131== by 0x10E6F0: str_append (execute.c:165) ==13131== by 0x110779: do_subst (execute.c:1106) ==13131== by 0x1115D3: execute_program (execute.c:1591) ==13131== by 0x111A4C: process_files (execute.c:1774) ==13131== by 0x112E1C: main (sed.c:405) ==13131== The ^/$ optimization code added in v4.2.2-161-g6dea75e called memchr() using 'buflen', ignoring the value of 'buf_start_offset' (which, if not zero, reduces the number of bytes available for the search). Reported by address@hidden (bug#32271) in https://lists.gnu.org/r/bug-sed/2018-07/msg00018.html . * NEWS: Mention the fix. * sed/regexp.c (match_regex): Use correct buffer length in memchr(). * testsuite/bug-32271-2.sh: Test using valgrind. * testsuite/local.mk (T): Add new test. --- NEWS | 3 ++ sed/regexp.c | 3 +- testsuite/bug32271-2.sh | 75 +++++++++++++++++++++++++++++++++++++++++++++++++ testsuite/local.mk | 1 + 4 files changed, 81 insertions(+), 1 deletion(-) create mode 100755 testsuite/bug32271-2.sh diff --git a/NEWS b/NEWS index f9293f3..8f5c37c 100644 --- a/NEWS +++ b/NEWS @@ -10,6 +10,9 @@ GNU sed NEWS -*- outline -*- sed no longer adds extraneous NUL when given s/$//n command. [related to bug#32271, present since sed-4.0.7] + sed no longer accesses invalid memory (heap overflow) with s/$//n regexes. + [bug#32271, present since sed-4.3]. + * Noteworthy changes in release 4.5 (2018-03-31) [stable] diff --git a/sed/regexp.c b/sed/regexp.c index f7c2851..323898a 100644 --- a/sed/regexp.c +++ b/sed/regexp.c @@ -283,7 +283,8 @@ match_regex(struct regex *regex, char *buf, size_t buflen, const char *p = NULL; if (regex->flags & REG_NEWLINE) - p = memchr (buf + buf_start_offset, buffer_delimiter, buflen); + p = memchr (buf + buf_start_offset, buffer_delimiter, + buflen - buf_start_offset); offset = p ? p - buf : buflen; } diff --git a/testsuite/bug32271-2.sh b/testsuite/bug32271-2.sh new file mode 100755 index 0000000..d6e50ce --- /dev/null +++ b/testsuite/bug32271-2.sh @@ -0,0 +1,75 @@ +#!/bin/sh +# sed would access uninitialized memory for certain regexes. +# Before sed 4.6 these would result in "Conditional jump or move depends on +# uninitialised value(s)" and "Invalid read of size 1" +# by valgrind from regexp.c:286 + +# Copyright (C) 2018 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +. "${srcdir=.}/testsuite/init.sh"; path_prepend_ ./sed +print_ver_ sed + +require_valgrind_ + +# 40 characters ensures valgrind detects the bug +# (with less than 25 - it does not). +z=0000000000000000000000000000000000000000 + +printf '%s\n' $z $z > in || framework_failure_ +printf '%s\n' $z $z > exp || framework_failure_ + +# Before sed-4.6, this would fail with: +# [...] +# ==13131== Conditional jump or move depends on uninitialised value(s) +# ==13131== at 0x4C3002B: memchr (vg_replace_strmem.c:883) +# ==13131== by 0x1120BD: match_regex (regexp.c:286) +# ==13131== by 0x110736: do_subst (execute.c:1101) +# ==13131== by 0x1115D3: execute_program (execute.c:1591) +# ==13131== by 0x111A4C: process_files (execute.c:1774) +# ==13131== by 0x112E1C: main (sed.c:405) +# ==13131== +# ==13131== Invalid read of size 1 +# ==13131== at 0x4C30027: memchr (vg_replace_strmem.c:883) +# ==13131== by 0x1120BD: match_regex (regexp.c:286) +# ==13131== by 0x110736: do_subst (execute.c:1101) +# ==13131== by 0x1115D3: execute_program (execute.c:1591) +# ==13131== by 0x111A4C: process_files (execute.c:1774) +# ==13131== by 0x112E1C: main (sed.c:405) +# ==13131== Address 0x55ec765 is 0 bytes after a block of size 101 alloc'd +# ==13131== at 0x4C2DDCF: realloc (vg_replace_malloc.c:785) +# ==13131== by 0x113BA2: ck_realloc (utils.c:418) +# ==13131== by 0x10E682: resize_line (execute.c:154) +# ==13131== by 0x10E6F0: str_append (execute.c:165) +# ==13131== by 0x110779: do_subst (execute.c:1106) +# ==13131== by 0x1115D3: execute_program (execute.c:1591) +# ==13131== by 0x111A4C: process_files (execute.c:1774) +# ==13131== by 0x112E1C: main (sed.c:405) +valgrind --quiet --error-exitcode=1 \ + sed -e 'N; s/$//m2' in > out 2> err || fail=1 + +# Work around a bug in CentOS 5.10's valgrind +# FIXME: remove in 2018 or when CentOS 5 is no longer officially supported +grep 'valgrind: .*Assertion.*failed' err-no-posix > /dev/null \ + && skip_ 'you seem to have a buggy version of valgrind' + +compare exp out || fail=1 +compare /dev/null err || fail=1 + +echo "valgrind report:" +echo "==================================" +cat err +echo "==================================" + +exit $fail diff --git a/testsuite/local.mk b/testsuite/local.mk index 1181a0c..6d0a74d 100644 --- a/testsuite/local.mk +++ b/testsuite/local.mk @@ -45,6 +45,7 @@ T = \ testsuite/misc.pl \ testsuite/bug32082.sh \ testsuite/bug32271-1.sh \ + testsuite/bug32271-2.sh \ testsuite/cmd-l.sh \ testsuite/cmd-R.sh \ testsuite/colon-with-no-label.sh \ -- 2.11.0