[src/trunk]: src/usr.bin/make make(1): fix assertion failure for files withou...

[rillig <rillig%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/d67c87bb756e
branches:  trunk
changeset: 948240:d67c87bb756e
user:      rillig <rillig%NetBSD.org@localhost>
date:      Tue Dec 22 08:05:08 2020 +0000

description:
make(1): fix assertion failure for files without trailing newline

Previously, mmapped files didn't always have the final newline added.
Only those that ended at a page boundary did.

This confused ParseRawLine, which assumed (and since parse.c 1.510 from
moments ago also asserted) that every line ends with a newline, which
allows the code to assume that after a backslash, there is at least one
other character in the buffer, thereby preventing an out-of-bounds read.

This bug had been there at least since parse.c 1.170 from 2010-12-25
04:57:07, maybe even earlier, I didn't check.

Now line_end always points to the trailing newline, which allows
ParseGetLine to overwrite that character to end the string.

diffstat:

 usr.bin/make/parse.c                 |  47 +++++++++++++++--------------------
 usr.bin/make/unit-tests/opt-file.exp |   1 +
 usr.bin/make/unit-tests/opt-file.mk  |  11 +++++++-
 3 files changed, 31 insertions(+), 28 deletions(-)

diffs (123 lines):

diff -r ab4e3c4e294e -r d67c87bb756e usr.bin/make/parse.c
--- a/usr.bin/make/parse.c      Tue Dec 22 07:22:39 2020 +0000
+++ b/usr.bin/make/parse.c      Tue Dec 22 08:05:08 2020 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: parse.c,v 1.510 2020/12/22 06:48:33 rillig Exp $       */
+/*     $NetBSD: parse.c,v 1.511 2020/12/22 08:05:08 rillig Exp $       */
 
 /*
  * Copyright (c) 1988, 1989, 1990, 1993
@@ -117,7 +117,7 @@
 #include "pathnames.h"
 
 /*     "@(#)parse.c    8.3 (Berkeley) 3/19/94" */
-MAKE_RCSID("$NetBSD: parse.c,v 1.510 2020/12/22 06:48:33 rillig Exp $");
+MAKE_RCSID("$NetBSD: parse.c,v 1.511 2020/12/22 08:05:08 rillig Exp $");
 
 /* types and constants */
 
@@ -466,13 +466,14 @@
        if (lf->buf == MAP_FAILED)
                return FALSE;
 
-       if (lf->len == lf->maplen && lf->buf[lf->len - 1] != '\n') {
-               char *b = bmake_malloc(lf->len + 1);
-               b[lf->len] = '\n';
-               memcpy(b, lf->buf, lf->len++);
-               munmap(lf->buf, lf->maplen);
-               lf->maplen = 0;
-               lf->buf = b;
+       if (lf->len > 0 && lf->buf[lf->len - 1] != '\n') {
+               if (lf->len == lf->maplen) {
+                       char *b = bmake_malloc(lf->len + 1);
+                       memcpy(b, lf->buf, lf->len);
+                       munmap(lf->buf, lf->maplen);
+                       lf->maplen = 0;
+               }
+               lf->buf[lf->len++] = '\n';
        }
 
        return TRUE;
@@ -2687,19 +2688,15 @@
                if (ch == '\\') {
                        if (firstBackslash == NULL)
                                firstBackslash = p;
-                       /*
-                        * FIXME: In opt-file.mk, this command succeeds:
-                        *      printf '%s' 'V=v\' | make -r -f -
-                        * Using an intermediate file fails though:
-                        *      printf '%s' 'V=v\' > backslash
-                        *      make -r -f backslash
-                        *
-                        * In loadedfile_mmap, the trailing newline is not
-                        * added in every case, only if the file ends at a
-                        * page boundary.
-                        */
-                       if (p[1] == '\n')
+                       if (p[1] == '\n') {
                                curFile->lineno++;
+                               if (p + 2 == curFile->buf_end) {
+                                       line_end = p;
+                                       *line_end = '\n';
+                                       p += 2;
+                                       continue;
+                               }
+                       }
                        p += 2;
                        line_end = p;
                        assert(p <= curFile->buf_end);
@@ -2843,12 +2840,8 @@
                }
 
                /* We now have a line of data */
-               /*
-                * FIXME: undefined behavior since line_end points right
-                * after the allocated buffer. This becomes apparent when
-                * using a strict malloc implementation that adds canaries
-                * before and after the allocated space.
-                */
+               /* TODO: Remove line_end, it's not necessary here. */
+               assert(*line_end == '\n');
                *line_end = '\0';
 
                if (mode == GLM_FOR_BODY)
diff -r ab4e3c4e294e -r d67c87bb756e usr.bin/make/unit-tests/opt-file.exp
--- a/usr.bin/make/unit-tests/opt-file.exp      Tue Dec 22 07:22:39 2020 +0000
+++ b/usr.bin/make/unit-tests/opt-file.exp      Tue Dec 22 08:05:08 2020 +0000
@@ -1,3 +1,4 @@
+value
 value
 make: "(stdin)" line 1: Zero byte read from file
 make: Fatal errors encountered -- cannot continue
diff -r ab4e3c4e294e -r d67c87bb756e usr.bin/make/unit-tests/opt-file.mk
--- a/usr.bin/make/unit-tests/opt-file.mk       Tue Dec 22 07:22:39 2020 +0000
+++ b/usr.bin/make/unit-tests/opt-file.mk       Tue Dec 22 08:05:08 2020 +0000
@@ -1,4 +1,4 @@
-# $NetBSD: opt-file.mk,v 1.7 2020/12/06 20:55:30 rillig Exp $
+# $NetBSD: opt-file.mk,v 1.8 2020/12/22 08:05:08 rillig Exp $
 #
 # Tests for the -f command line option.
 
@@ -6,6 +6,7 @@
 
 all: .PHONY
 all: file-ending-in-backslash
+all: file-ending-in-backslash-mmap
 all: file-containing-null-byte
 
 # Passing '-' as the filename reads from stdin.  This is unusual but possible.
@@ -35,6 +36,14 @@
        @printf '%s' 'VAR=value\' \
        | ${MAKE} -r -f - -V VAR
 
+# Between parse.c 1.170 from 2010-12-25 and parse.c 1.511 from 2020-12-22,
+# there was an out-of-bounds write in ParseGetLine, where line_end pointed at
+# the end of the allocated buffer, in the special case where loadedfile_mmap
+# had not added the final newline character.
+file-ending-in-backslash-mmap: .PHONY
+       @printf '%s' 'VAR=value\' > opt-file-backslash
+       @${MAKE} -r -f opt-file-backslash -V VAR
+
 # If a file contains null bytes, the rest of the line is skipped, and parsing
 # continues in the next line.  Throughout the history of make, the behavior
 # has changed several times, sometimes knowingly, sometimes by accident.