NetBSD Problem Report #57573
From www@netbsd.org Tue Aug 8 17:09:20 2023
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id 72A301A9238
for <gnats-bugs@gnats.NetBSD.org>; Tue, 8 Aug 2023 17:09:20 +0000 (UTC)
Message-Id: <20230808170849.9936C1A923A@mollari.NetBSD.org>
Date: Tue, 8 Aug 2023 17:08:49 +0000 (UTC)
From: kevans@FreeBSD.org
Reply-To: kevans@FreeBSD.org
To: gnats-bugs@NetBSD.org
Subject: Overflow possibilities in vis(3)
X-Send-Pr-Version: www-1.0
>Number: 57573
>Category: lib
>Synopsis: Overflow possibilities in vis(3)
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: lib-bug-people
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Tue Aug 08 17:10:01 +0000 2023
>Closed-Date: Sat Dec 09 14:16:39 +0000 2023
>Last-Modified: Sat Dec 09 14:16:39 +0000 2023
>Originator: Kyle Evans
>Release: N/A
>Organization:
Klara, Inc.
>Environment:
FreeBSD
>Description:
We identified some overflow possibilities in vis(3) downstream in FreeBSD and wanted to alert you folks of the patch that we came up with.
Generally, there's two separate isuses:
1.) We wcrtomb() into mbst and only check that it didn't overflow *after* we already overflowed (+ same issue in the byte-by-byte fallback)
2.) The overflow check didn't account for the fact that `maxolen` includes the NUL terminator, so we could overflow when writing the NUL terminator out
>How-To-Repeat:
See tests added in referenced patch; currently we would observe that both the strnvis(3) call wouldn't fail and one byte past the end is overwritten in both of these cases, when ideally we should at the very least not write past the end of the known bounds and ultimately fail instead of simply truncating the result.
>Fix:
Patch with test included here: https://cgit.freebsd.org/src/patch/?id=2f489a509e615c46be6f7c6aa7cea161f50f18af
>Release-Note:
>Audit-Trail:
From: Taylor R Campbell <riastradh@NetBSD.org>
To: kevans@FreeBSD.org
Cc: gnats-bugs@NetBSD.org
Subject: Re: lib/57573: Overflow possibilities in vis(3)
Date: Sat, 12 Aug 2023 08:48:21 +0000
Thanks, taking a look at this. I think this code needs a little more
reworking, and a lot more automatic testing for something that is
supposed to make unprintable things safe.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src/tests/lib/libc/gen
Date: Sat, 12 Aug 2023 12:43:26 +0000
Module Name: src
Committed By: riastradh
Date: Sat Aug 12 12:43:26 UTC 2023
Modified Files:
src/tests/lib/libc/gen: t_vis.c
Log Message:
vis(3) tests: Add xfail test for encoding overflow.
From Kyle Evans <kevans@FreeBSD.org>.
PR lib/57573
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.9 -r1.10 src/tests/lib/libc/gen/t_vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src/tests/lib/libc/gen
Date: Sat, 12 Aug 2023 12:45:03 +0000
Module Name: src
Committed By: riastradh
Date: Sat Aug 12 12:45:03 UTC 2023
Modified Files:
src/tests/lib/libc/gen: t_vis.c
Log Message:
vis(3) tests: Expand tests and diagnostic outputs on failure.
PR lib/57573
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.10 -r1.11 src/tests/lib/libc/gen/t_vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src/tests/lib/libc/gen
Date: Sat, 12 Aug 2023 12:46:16 +0000
Module Name: src
Committed By: riastradh
Date: Sat Aug 12 12:46:16 UTC 2023
Modified Files:
src/tests/lib/libc/gen: t_vis.c
Log Message:
vis(3) tests: Test another overflow edge case.
Related to PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.11 -r1.12 src/tests/lib/libc/gen/t_vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src/lib/libc/gen
Date: Sat, 12 Aug 2023 12:46:33 +0000
Module Name: src
Committed By: riastradh
Date: Sat Aug 12 12:46:33 UTC 2023
Modified Files:
src/lib/libc/gen: vis.c
Log Message:
vis(3): Make maxolen unsigned size_t, not ssize_t.
It is initialized once either to *dlen, which is unsigned size_t, or
to wcslen(start) * MB_MAX_LEN + 1, and wcslen returns unsigned size_t
too. So there appears to have never been any reason for this to be
signed.
Part of PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.76 -r1.77 src/lib/libc/gen/vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src/lib/libc/gen
Date: Sat, 12 Aug 2023 12:46:50 +0000
Module Name: src
Committed By: riastradh
Date: Sat Aug 12 12:46:50 UTC 2023
Modified Files:
src/lib/libc/gen: vis.c
Log Message:
vis(3): Make mbslength unsigned.
Sprinkle assertions and comments justifying the proposition that it
would never go negative if signed.
Obviates need to worry about mblength > SSIZE_MAX.
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.77 -r1.78 src/lib/libc/gen/vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src/lib/libc/gen
Date: Sat, 12 Aug 2023 12:47:17 +0000
Module Name: src
Committed By: riastradh
Date: Sat Aug 12 12:47:17 UTC 2023
Modified Files:
src/lib/libc/gen: vis.c
Log Message:
vis(3): Avoid arithmetic overflow before calloc(3).
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.78 -r1.79 src/lib/libc/gen/vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src/lib/libc/gen
Date: Sat, 12 Aug 2023 12:48:01 +0000
Module Name: src
Committed By: riastradh
Date: Sat Aug 12 12:48:01 UTC 2023
Modified Files:
src/lib/libc/gen: vis.c
Log Message:
vis(3): Call wcslen(start) only once.
It had better not change between these two times!
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.79 -r1.80 src/lib/libc/gen/vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src/lib/libc/gen
Date: Sat, 12 Aug 2023 12:48:17 +0000
Module Name: src
Committed By: riastradh
Date: Sat Aug 12 12:48:17 UTC 2023
Modified Files:
src/lib/libc/gen: vis.c
Log Message:
vis(3): Avoid potential arithmetic overflow in maxolen.
Can't easily prove that this overflow is impossible, so let's add a
check.
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.80 -r1.81 src/lib/libc/gen/vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src
Date: Sat, 12 Aug 2023 12:48:37 +0000
Module Name: src
Committed By: riastradh
Date: Sat Aug 12 12:48:37 UTC 2023
Modified Files:
src/lib/libc/gen: vis.c
src/tests/lib/libc/gen: t_vis.c
Log Message:
vis(3): Fix main part of PR lib/57573.
From Kyle Evans <kevans@FreeBSD.org>.
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.81 -r1.82 src/lib/libc/gen/vis.c
cvs rdiff -u -r1.12 -r1.13 src/tests/lib/libc/gen/t_vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src
Date: Sat, 12 Aug 2023 12:48:53 +0000
Module Name: src
Committed By: riastradh
Date: Sat Aug 12 12:48:53 UTC 2023
Modified Files:
src/lib/libc/gen: vis.c
src/tests/lib/libc/gen: t_vis.c
Log Message:
vis(3): Fix one more buffer overrun in an edge case.
PR lib/57573
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.82 -r1.83 src/lib/libc/gen/vis.c
cvs rdiff -u -r1.13 -r1.14 src/tests/lib/libc/gen/t_vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sat, 12 Aug 2023 12:53:22 +0000
State-Changed-Why:
candidate fix committed, needs pullups to 10/9/8
From: Taylor R Campbell <riastradh@NetBSD.org>
To: kevans@FreeBSD.org
Cc: gnats-bugs@NetBSD.org
Subject: Re: lib/57573: Overflow possibilities in vis(3)
Date: Sat, 12 Aug 2023 15:01:36 +0000
This is a multi-part message in MIME format.
--=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
Turns out there's another buffer overrun lurking, when you do
strnvis(dst, 0, "", ...) -- it shouldn't write to dst[0], but it does.
I added an atf test for that, a branch to prevent it, and an assertion
to catch it.
I adapted your patch for the other overruns with some minor changes:
1. `char mbbuf[MB_LEN_MAX]' instead of `char mbbuf[MB_CUR_MAX]', since
MB_CUR_MAX is not a compile-time constant.
This avoids what is formally a variable-length array, which gets in
the way of stack protectors, &c. It may not pose a practical
problem for stack overflow, because MB_CUR_MAX <= MB_LEN_MAX, but
it's easier to keep measures that detect real problems elsewhere
happy this way.
2. Changed mbslength from ssize_t to size_t, since it can't go
negative, and added some assertions to support this.
This obviates the need to worry about what happens when mblength >
SIZE_MAX -- maybe impossible but I don't need to think about
proving that now!
3. Added another bounds check upstream of the callocs, in case
16*mbslength + 1 would overflow.
4. Eliminated a duplicate call wcslen(start).
5. Kept the #ifdef VIS_NOLOCALE in t_vis.c updated. Not sure why this
is here, but easier to just keep it updated than to investigate.
Attached is the patch series I committed to NetBSD for convenience if
you want to take a look. Tried to avoid unnecessary style differences
from yours to make merging easier if you want.
(I feel like this code shouldn't need to allocate multiple temporary
copies of everything -- should be able to work incrementally in a
stream from src to dst with only a constant-size intermediate buffer
like mbbuf, but I'm not feeling inclined to rewrite this now, so...)
Thanks for the report!
--=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
Content-Type: text/plain; charset="ISO-8859-1"; name="pr57573-vis-v2"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="pr57573-vis-v2.patch"
From e6466e0119be123f98a5385ac2101a0cba121ced Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Aug 2023 07:20:03 +0000
Subject: [PATCH 01/10] vis(3) tests: Add xfail test for encoding overflow.
From Kyle Evans <kevans@FreeBSD.org>.
PR lib/57573
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
tests/lib/libc/gen/t_vis.c | 68 ++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/tests/lib/libc/gen/t_vis.c b/tests/lib/libc/gen/t_vis.c
index adb0930a300a..18162565835c 100644
--- a/tests/lib/libc/gen/t_vis.c
+++ b/tests/lib/libc/gen/t_vis.c
@@ -175,6 +175,72 @@ ATF_TC_BODY(strvis_locale, tc)
}
#endif /* VIS_NOLOCALE */
=20
+#define STRVIS_OVERFLOW_MARKER 0xff /* Arbitrary */
+
+#ifdef VIS_NOLOCALE
+ATF_TC(strvis_overflow_mb);
+ATF_TC_HEAD(strvis_overflow_mb, tc)
+{
+ atf_tc_set_md_var(tc, "descr", "Test strvis(3) multi-byte overflow");
+}
+
+ATF_TC_BODY(strvis_overflow_mb, tc)
+{
+ const char src[] =3D "\xf0\x9f\xa5\x91";
+ /* Extra byte to detect overflow */
+ char dst[sizeof(src) + 1];
+ int n;
+
+ atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
+
+ setlocale(LC_CTYPE, "en_US.UTF-8");
+
+ /* Arbitrary */
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+
+ /*
+ * If we only provide four bytes of buffer, we shouldn't be able encode
+ * a full 4-byte sequence.
+ */
+ n =3D strnvis(dst, 4, src, VIS_SAFE);
+ ATF_REQUIRE(dst[4] =3D=3D STRVIS_OVERFLOW_MARKER);
+ ATF_REQUIRE(n =3D=3D -1);
+
+ n =3D strnvis(dst, sizeof(src), src, VIS_SAFE);
+ ATF_REQUIRE(n =3D=3D sizeof(src) - 1);
+}
+#endif
+
+ATF_TC(strvis_overflow_c);
+ATF_TC_HEAD(strvis_overflow_c, tc)
+{
+ atf_tc_set_md_var(tc, "descr", "Test strvis(3) C locale overflow");
+}
+
+ATF_TC_BODY(strvis_overflow_c, tc)
+{
+ const char src[] =3D "AAAA";
+ /* Extra byte to detect overflow */
+ char dst[sizeof(src) + 1];
+ int n;
+
+ atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
+
+ /* Arbitrary */
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+
+ /*
+ * If we only provide four bytes of buffer, we shouldn't be able encode
+ * 4 bytes of input.
+ */
+ n =3D strnvis(dst, 4, src, VIS_SAFE | VIS_NOLOCALE);
+ ATF_REQUIRE(dst[4] =3D=3D STRVIS_OVERFLOW_MARKER);
+ ATF_REQUIRE(n =3D=3D -1);
+
+ n =3D strnvis(dst, sizeof(src), src, VIS_SAFE | VIS_NOLOCALE);
+ ATF_REQUIRE(n =3D=3D sizeof(src) - 1);
+}
+
ATF_TP_ADD_TCS(tp)
{
=20
@@ -184,7 +250,9 @@ ATF_TP_ADD_TCS(tp)
ATF_TP_ADD_TC(tp, strunvis_hex);
#ifdef VIS_NOLOCALE
ATF_TP_ADD_TC(tp, strvis_locale);
+ ATF_TP_ADD_TC(tp, strvis_overflow_mb);
#endif /* VIS_NOLOCALE */
+ ATF_TP_ADD_TC(tp, strvis_overflow_c);
=20
return atf_no_error();
}
From 8e44622ed3027a113d5c39b7abb436a0d4a7f0bc Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Aug 2023 07:24:15 +0000
Subject: [PATCH 02/10] vis(3) tests: Expand tests and diagnostic outputs on
failure.
PR lib/57573
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
tests/lib/libc/gen/t_vis.c | 64 +++++++++++++++++++++++---------------
1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/tests/lib/libc/gen/t_vis.c b/tests/lib/libc/gen/t_vis.c
index 18162565835c..cf29123dc992 100644
--- a/tests/lib/libc/gen/t_vis.c
+++ b/tests/lib/libc/gen/t_vis.c
@@ -175,7 +175,7 @@ ATF_TC_BODY(strvis_locale, tc)
}
#endif /* VIS_NOLOCALE */
=20
-#define STRVIS_OVERFLOW_MARKER 0xff /* Arbitrary */
+#define STRVIS_OVERFLOW_MARKER ((char)0xff) /* Arbitrary */
=20
#ifdef VIS_NOLOCALE
ATF_TC(strvis_overflow_mb);
@@ -189,25 +189,32 @@ ATF_TC_BODY(strvis_overflow_mb, tc)
const char src[] =3D "\xf0\x9f\xa5\x91";
/* Extra byte to detect overflow */
char dst[sizeof(src) + 1];
+ unsigned i;
int n;
=20
atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
=20
setlocale(LC_CTYPE, "en_US.UTF-8");
=20
- /* Arbitrary */
- memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
-
- /*
- * If we only provide four bytes of buffer, we shouldn't be able encode
- * a full 4-byte sequence.
- */
- n =3D strnvis(dst, 4, src, VIS_SAFE);
- ATF_REQUIRE(dst[4] =3D=3D STRVIS_OVERFLOW_MARKER);
- ATF_REQUIRE(n =3D=3D -1);
+ for (i =3D 0; i < sizeof(dst) - 1; i++) {
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+ n =3D strnvis(dst, i, src, VIS_SAFE);
+ ATF_CHECK_EQ_MSG(dst[i], STRVIS_OVERFLOW_MARKER,
+ "[%u] dst=3D[%02hhx %02hhx %02hhx %02hhx %02hhx]"
+ " STRVIS_OVERFLOW_MARKER=3D%02hhx",
+ i, dst[0], dst[1], dst[2], dst[3], dst[4],
+ STRVIS_OVERFLOW_MARKER);
+ ATF_CHECK_EQ_MSG(n, -1, "[%u] n=3D%d", i, n);
+ }
=20
- n =3D strnvis(dst, sizeof(src), src, VIS_SAFE);
- ATF_REQUIRE(n =3D=3D sizeof(src) - 1);
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+ n =3D strnvis(dst, sizeof(dst) - 1, src, VIS_SAFE);
+ ATF_CHECK_EQ_MSG(dst[sizeof(dst) - 1], STRVIS_OVERFLOW_MARKER,
+ "[%u] dst=3D[%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx]"
+ " STRVIS_OVERFLOW_MARKER=3D%02hhx",
+ i, dst[0], dst[1], dst[2], dst[3], dst[4], dst[5],
+ STRVIS_OVERFLOW_MARKER);
+ ATF_CHECK_EQ_MSG(n, (int)sizeof(dst) - 2, "n=3D%d", n);
}
#endif
=20
@@ -222,23 +229,30 @@ ATF_TC_BODY(strvis_overflow_c, tc)
const char src[] =3D "AAAA";
/* Extra byte to detect overflow */
char dst[sizeof(src) + 1];
+ unsigned i;
int n;
=20
atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
=20
- /* Arbitrary */
- memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
-
- /*
- * If we only provide four bytes of buffer, we shouldn't be able encode
- * 4 bytes of input.
- */
- n =3D strnvis(dst, 4, src, VIS_SAFE | VIS_NOLOCALE);
- ATF_REQUIRE(dst[4] =3D=3D STRVIS_OVERFLOW_MARKER);
- ATF_REQUIRE(n =3D=3D -1);
+ for (i =3D 0; i < sizeof(dst) - 1; i++) {
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+ n =3D strnvis(dst, i, src, VIS_SAFE | VIS_NOLOCALE);
+ ATF_CHECK_EQ_MSG(dst[i], STRVIS_OVERFLOW_MARKER,
+ "[%u] dst=3D[%02hhx %02hhx %02hhx %02hhx %02hhx]"
+ " STRVIS_OVERFLOW_MARKER=3D%02hhx",
+ i, dst[0], dst[1], dst[2], dst[3], dst[4],
+ STRVIS_OVERFLOW_MARKER);
+ ATF_CHECK_EQ_MSG(n, -1, "[%u] n=3D%d", i, n);
+ }
=20
- n =3D strnvis(dst, sizeof(src), src, VIS_SAFE | VIS_NOLOCALE);
- ATF_REQUIRE(n =3D=3D sizeof(src) - 1);
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+ n =3D strnvis(dst, sizeof(dst) - 1, src, VIS_SAFE | VIS_NOLOCALE);
+ ATF_CHECK_EQ_MSG(dst[sizeof(dst) - 1], STRVIS_OVERFLOW_MARKER,
+ "[%u] dst=3D[%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx]"
+ " STRVIS_OVERFLOW_MARKER=3D%02hhx",
+ i, dst[0], dst[1], dst[2], dst[3], dst[4], dst[5],
+ STRVIS_OVERFLOW_MARKER);
+ ATF_CHECK_EQ_MSG(n, (int)sizeof(dst) - 2, "n=3D%d", n);
}
=20
ATF_TP_ADD_TCS(tp)
From 2aff286fef53c7166eb876e5cc4798cd7ad917f7 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Aug 2023 12:26:33 +0000
Subject: [PATCH 03/10] vis(3) tests: Test another overflow edge case.
Related to PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
tests/lib/libc/gen/t_vis.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/tests/lib/libc/gen/t_vis.c b/tests/lib/libc/gen/t_vis.c
index cf29123dc992..7ae0a6bc3885 100644
--- a/tests/lib/libc/gen/t_vis.c
+++ b/tests/lib/libc/gen/t_vis.c
@@ -116,6 +116,25 @@ ATF_TC_BODY(strvis_empty, tc)
ATF_REQUIRE(dst[0] =3D=3D '\0' && dst[1] =3D=3D 'a');
}
=20
+ATF_TC(strnvis_empty_empty);
+ATF_TC_HEAD(strnvis_empty_empty, tc)
+{
+ atf_tc_set_md_var(tc, "descr",
+ "Test strnvis(3) with empty source and destination");
+}
+
+ATF_TC_BODY(strnvis_empty_empty, tc)
+{
+ char dst[] =3D "fail";
+ int n;
+
+ atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
+
+ n =3D strnvis(dst, 0, "", VIS_SAFE);
+ ATF_CHECK(memcmp(dst, "fail", sizeof(dst)) =3D=3D 0);
+ ATF_CHECK_EQ_MSG(n, -1, "n=3D%d", n);
+}
+
ATF_TC(strunvis_hex);
ATF_TC_HEAD(strunvis_hex, tc)
{
@@ -261,6 +280,7 @@ ATF_TP_ADD_TCS(tp)
ATF_TP_ADD_TC(tp, strvis_basic);
ATF_TP_ADD_TC(tp, strvis_null);
ATF_TP_ADD_TC(tp, strvis_empty);
+ ATF_TP_ADD_TC(tp, strnvis_empty_empty);
ATF_TP_ADD_TC(tp, strunvis_hex);
#ifdef VIS_NOLOCALE
ATF_TP_ADD_TC(tp, strvis_locale);
From 37b6975cd9b7df8286c00185a02a5c527ab29f9e Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Aug 2023 08:12:14 +0000
Subject: [PATCH 04/10] vis(3): Make maxolen unsigned size_t, not ssize_t.
It is initialized once either to *dlen, which is unsigned size_t, or
to wcslen(start) * MB_MAX_LEN + 1, and wcslen returns unsigned size_t
too. So there appears to have never been any reason for this to be
signed.
Part of PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index ecc304c8015c..5f7a494287dc 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -403,7 +403,8 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
visfun_t f;
int clen =3D 0, cerr, error =3D -1, i, shft;
char *mbdst, *mdst;
- ssize_t mbslength, maxolen;
+ ssize_t mbslength;
+ size_t maxolen;
mbstate_t mbstate;
=20
_DIAGASSERT(mbdstp !=3D NULL);
@@ -569,7 +570,7 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
cerr =3D 1;
}
/* If this character would exceed our output limit, stop. */
- if (olen + clen > (size_t)maxolen)
+ if (olen + clen > maxolen)
break;
/* Advance output pointer by number of bytes written. */
mbdst +=3D clen;
From 238d2917c36ff5ba5b7c3685481d321db08d1827 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Aug 2023 08:43:21 +0000
Subject: [PATCH 05/10] vis(3): Make mbslength unsigned.
Sprinkle assertions and comments justifying the proposition that it
would never go negative if signed.
Obviates need to worry about mblength > SSIZE_MAX.
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index 5f7a494287dc..5dc7c9e85907 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -403,7 +403,7 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
visfun_t f;
int clen =3D 0, cerr, error =3D -1, i, shft;
char *mbdst, *mdst;
- ssize_t mbslength;
+ size_t mbslength;
size_t maxolen;
mbstate_t mbstate;
=20
@@ -411,7 +411,7 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
_DIAGASSERT(mbsrc !=3D NULL || mblength =3D=3D 0);
_DIAGASSERT(mbextra !=3D NULL);
=20
- mbslength =3D (ssize_t)mblength;
+ mbslength =3D mblength;
/*
* When inputing a single character, must also read in the
* next character for nextc, the look-ahead character.
@@ -466,12 +466,15 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *=
mbsrc, size_t mblength,
memset(&mbstate, 0, sizeof(mbstate));
while (mbslength > 0) {
/* Convert one multibyte character to wchar_t. */
- if (!cerr)
+ if (!cerr) {
clen =3D mbrtowc(src, mbsrc,
(mbslength < MB_LEN_MAX
? mbslength
: MB_LEN_MAX),
&mbstate);
+ assert(clen < 0 || (size_t)clen <=3D mbslength);
+ assert(clen <=3D MB_LEN_MAX);
+ }
if (cerr || clen < 0) {
/* Conversion error, process as a byte instead. */
*src =3D (wint_t)(u_char)*mbsrc;
@@ -485,6 +488,20 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *m=
bsrc, size_t mblength,
*/
clen =3D 1;
}
+ /*
+ * Let n :=3D MIN(mbslength, MB_LEN_MAX). We have:
+ *
+ * mbslength >=3D 1
+ * mbrtowc(..., n, &mbstate) <=3D n,
+ * by the contract of mbrtowc
+ *
+ * clen is either
+ * (a) mbrtowc(..., n, &mbstate), in which case
+ * clen <=3D n <=3D mbslength; or
+ * (b) 1, in which case clen =3D 1 <=3D mbslength.
+ */
+ assert(clen > 0);
+ assert((size_t)clen <=3D mbslength);
/* Advance buffer character pointer. */
src++;
/* Advance input pointer by number of bytes read. */
From 598a93cb3ab010086e82ff5a6951c5f129edad06 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Aug 2023 08:46:46 +0000
Subject: [PATCH 06/10] vis(3): Avoid arithmetic overflow before calloc(3).
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index 5dc7c9e85907..5c9222f6acd0 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -432,6 +432,14 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *m=
bsrc, size_t mblength,
* return to the caller.
*/
=20
+ /*
+ * Guarantee the arithmetic on input to calloc won't overflow.
+ */
+ if (mbslength > (SIZE_MAX - 1)/16) {
+ errno =3D ENOMEM;
+ return -1;
+ }
+
/* Allocate space for the wide char strings */
psrc =3D pdst =3D extra =3D NULL;
mdst =3D NULL;
From 229f19a501dc75592f596c0260664de5493e9a47 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Aug 2023 09:37:58 +0000
Subject: [PATCH 07/10] vis(3): Call wcslen(start) only once.
It had better not change between these two times!
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index 5c9222f6acd0..d061749b1d50 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -567,7 +567,7 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
* output byte-by-byte here. Else use wctomb().
*/
len =3D wcslen(start);
- maxolen =3D dlen ? *dlen : (wcslen(start) * MB_LEN_MAX + 1);
+ maxolen =3D dlen ? *dlen : (len * MB_LEN_MAX + 1);
olen =3D 0;
memset(&mbstate, 0, sizeof(mbstate));
for (dst =3D start; len > 0; len--) {
From f442e562a74a11ebcfe166f46c8cd42146555a62 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Aug 2023 09:40:00 +0000
Subject: [PATCH 08/10] vis(3): Avoid potential arithmetic overflow in maxol=
en.
Can't easily prove that this overflow is impossible, so let's add a
check.
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index d061749b1d50..3e539fe22cd0 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -567,7 +567,15 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *m=
bsrc, size_t mblength,
* output byte-by-byte here. Else use wctomb().
*/
len =3D wcslen(start);
- maxolen =3D dlen ? *dlen : (len * MB_LEN_MAX + 1);
+ if (dlen) {
+ maxolen =3D *dlen;
+ } else {
+ if (len > (SIZE_MAX - 1)/MB_LEN_MAX) {
+ errno =3D ENOSPC;
+ goto out;
+ }
+ maxolen =3D len*MB_LEN_MAX + 1;
+ }
olen =3D 0;
memset(&mbstate, 0, sizeof(mbstate));
for (dst =3D start; len > 0; len--) {
From edea4ce6d025be7f375b9d7b6edb287d1edd250d Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Aug 2023 12:24:11 +0000
Subject: [PATCH 09/10] vis(3): Fix main part of PR lib/57573.
From Kyle Evans <kevans@FreeBSD.org>.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 51 ++++++++++++++++++++++++++++++++------
tests/lib/libc/gen/t_vis.c | 4 ---
2 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index 3e539fe22cd0..d291c80bcda4 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -396,13 +396,14 @@ static int
istrsenvisx(char **mbdstp, size_t *dlen, const char *mbsrc, size_t mblengt=
h,
int flags, const char *mbextra, int *cerr_ptr)
{
+ char mbbuf[MB_LEN_MAX];
wchar_t *dst, *src, *pdst, *psrc, *start, *extra;
size_t len, olen;
uint64_t bmsk, wmsk;
wint_t c;
visfun_t f;
int clen =3D 0, cerr, error =3D -1, i, shft;
- char *mbdst, *mdst;
+ char *mbdst, *mbwrite, *mdst;
size_t mbslength;
size_t maxolen;
mbstate_t mbstate;
@@ -579,8 +580,33 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *m=
bsrc, size_t mblength,
olen =3D 0;
memset(&mbstate, 0, sizeof(mbstate));
for (dst =3D start; len > 0; len--) {
- if (!cerr)
- clen =3D wcrtomb(mbdst, *dst, &mbstate);
+ if (!cerr) {
+ /*
+ * If we have at least MB_CUR_MAX bytes in the buffer,
+ * we'll just do the conversion in-place into mbdst. We
+ * need to be a little more conservative when we get to
+ * the end of the buffer, as we may not have MB_CUR_MAX
+ * bytes but we may not need it.
+ */
+ if (maxolen - olen > MB_CUR_MAX)
+ mbwrite =3D mbdst;
+ else
+ mbwrite =3D mbbuf;
+ clen =3D wcrtomb(mbwrite, *dst, &mbstate);
+ if (clen > 0 && mbwrite !=3D mbdst) {
+ /*
+ * Don't break past our output limit, noting
+ * that maxolen includes the nul terminator so
+ * we can't write past maxolen - 1 here.
+ */
+ if (olen + clen >=3D maxolen) {
+ errno =3D ENOSPC;
+ goto out;
+ }
+
+ memcpy(mbdst, mbwrite, clen);
+ }
+ }
if (cerr || clen < 0) {
/*
* Conversion error, process as a byte(s) instead.
@@ -595,16 +621,27 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *=
mbsrc, size_t mblength,
shft =3D i * NBBY;
bmsk =3D (uint64_t)0xffLL << shft;
wmsk |=3D bmsk;
- if ((*dst & wmsk) || i =3D=3D 0)
+ if ((*dst & wmsk) || i =3D=3D 0) {
+ if (olen + clen + 1 >=3D maxolen) {
+ errno =3D ENOSPC;
+ goto out;
+ }
+
mbdst[clen++] =3D (char)(
(uint64_t)(*dst & bmsk) >>
shft);
+ }
}
cerr =3D 1;
}
- /* If this character would exceed our output limit, stop. */
- if (olen + clen > maxolen)
- break;
+
+ /*
+ * We'll be dereferencing mbdst[clen] after this to write the
+ * nul terminator; the above paths should have checked for a
+ * possible overflow already.
+ */
+ assert(olen + clen < maxolen);
+
/* Advance output pointer by number of bytes written. */
mbdst +=3D clen;
/* Advance buffer character pointer. */
diff --git a/tests/lib/libc/gen/t_vis.c b/tests/lib/libc/gen/t_vis.c
index 7ae0a6bc3885..51b798b19f27 100644
--- a/tests/lib/libc/gen/t_vis.c
+++ b/tests/lib/libc/gen/t_vis.c
@@ -211,8 +211,6 @@ ATF_TC_BODY(strvis_overflow_mb, tc)
unsigned i;
int n;
=20
- atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
-
setlocale(LC_CTYPE, "en_US.UTF-8");
=20
for (i =3D 0; i < sizeof(dst) - 1; i++) {
@@ -251,8 +249,6 @@ ATF_TC_BODY(strvis_overflow_c, tc)
unsigned i;
int n;
=20
- atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
-
for (i =3D 0; i < sizeof(dst) - 1; i++) {
memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
n =3D strnvis(dst, i, src, VIS_SAFE | VIS_NOLOCALE);
From 724c7fa1eee212dcf3feec7debb87208aa799702 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh@NetBSD.org>
Date: Sat, 12 Aug 2023 12:28:37 +0000
Subject: [PATCH 10/10] vis(3): Fix one more buffer overrun in an edge case.
PR lib/57573
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 5 +++++
tests/lib/libc/gen/t_vis.c | 2 --
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index d291c80bcda4..a259d806d527 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -570,6 +570,10 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *m=
bsrc, size_t mblength,
len =3D wcslen(start);
if (dlen) {
maxolen =3D *dlen;
+ if (maxolen =3D=3D 0) {
+ errno =3D ENOSPC;
+ goto out;
+ }
} else {
if (len > (SIZE_MAX - 1)/MB_LEN_MAX) {
errno =3D ENOSPC;
@@ -651,6 +655,7 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
}
=20
/* Terminate the output string. */
+ assert(olen < maxolen);
*mbdst =3D '\0';
=20
if (flags & VIS_NOLOCALE) {
diff --git a/tests/lib/libc/gen/t_vis.c b/tests/lib/libc/gen/t_vis.c
index 51b798b19f27..4c085a59baf7 100644
--- a/tests/lib/libc/gen/t_vis.c
+++ b/tests/lib/libc/gen/t_vis.c
@@ -128,8 +128,6 @@ ATF_TC_BODY(strnvis_empty_empty, tc)
char dst[] =3D "fail";
int n;
=20
- atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
-
n =3D strnvis(dst, 0, "", VIS_SAFE);
ATF_CHECK(memcmp(dst, "fail", sizeof(dst)) =3D=3D 0);
ATF_CHECK_EQ_MSG(n, -1, "n=3D%d", n);
--=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG--
From: Kyle Evans <kevans@FreeBSD.org>
To: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org, gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org
Cc:
Subject: Re: lib/57573: Overflow possibilities in vis(3)
Date: Sat, 12 Aug 2023 22:35:17 -0500
On 8/12/23 10:05, Taylor R Campbell wrote:
> The following reply was made to PR lib/57573; it has been noted by GNATS.
>
> From: Taylor R Campbell <riastradh@NetBSD.org>
> To: kevans@FreeBSD.org
> Cc: gnats-bugs@NetBSD.org
> Subject: Re: lib/57573: Overflow possibilities in vis(3)
> Date: Sat, 12 Aug 2023 15:01:36 +0000
>
> This is a multi-part message in MIME format.
> --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
>
> Turns out there's another buffer overrun lurking, when you do
> strnvis(dst, 0, "", ...) -- it shouldn't write to dst[0], but it does.
> I added an atf test for that, a branch to prevent it, and an assertion
> to catch it.
>
Ahh, whoops, we didn't look quite that far up- our immediate concern was
a consumer that wasn't providing a buffer quite large enough (missing
space for a NUL byte); I convinced them to fix the size and switch to
strnvis(), but then realized that wouldn't quite be sufficient for the
scenario they were hitting in the first place where we were apparently
needing to encode every single byte in the buffer (thus, writing into a
redzone with the NUL terminator). I'm quite glad you did a more careful
analysis here.
> I adapted your patch for the other overruns with some minor changes:
>
Thanks!
> 1. `char mbbuf[MB_LEN_MAX]' instead of `char mbbuf[MB_CUR_MAX]', since
> MB_CUR_MAX is not a compile-time constant.
>
> This avoids what is formally a variable-length array, which gets in
> the way of stack protectors, &c. It may not pose a practical
> problem for stack overflow, because MB_CUR_MAX <= MB_LEN_MAX, but
> it's easier to keep measures that detect real problems elsewhere
> happy this way.
>
Ahh, good idea- I had contemplated making it MB_LEN_MAX in case someone
does something squirrely and swaps out the process locale in the middle
of a strvis(3), but hadn't yet convinced myself that that wasn't
problematic in many other ways yet.
> 2. Changed mbslength from ssize_t to size_t, since it can't go
> negative, and added some assertions to support this.
>
> This obviates the need to worry about what happens when mblength >
> SIZE_MAX -- maybe impossible but I don't need to think about
> proving that now!
>
> 3. Added another bounds check upstream of the callocs, in case
> 16*mbslength + 1 would overflow.
>
> 4. Eliminated a duplicate call wcslen(start).
>
> 5. Kept the #ifdef VIS_NOLOCALE in t_vis.c updated. Not sure why this
> is here, but easier to just keep it updated than to investigate.
>
> Attached is the patch series I committed to NetBSD for convenience if
> you want to take a look. Tried to avoid unnecessary style differences
> from yours to make merging easier if you want.
>
Perfect, thanks! I'll get this merged downstream as soon as I can.
> (I feel like this code shouldn't need to allocate multiple temporary
> copies of everything -- should be able to work incrementally in a
> stream from src to dst with only a constant-size intermediate buffer
> like mbbuf, but I'm not feeling inclined to rewrite this now, so. >
I agree with that sentiment... istrsenvisx in general kind of scares me
a bit still. :-)
> Thanks for the report!
>
Thanks,
Kyle Evans
From: Kyle Evans <kevans@FreeBSD.org>
To: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org, gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org
Cc:
Subject: Re: lib/57573: Overflow possibilities in vis(3)
Date: Sun, 13 Aug 2023 00:30:32 -0500
On 8/12/23 22:35, Kyle Evans wrote:
> On 8/12/23 10:05, Taylor R Campbell wrote:
>> The following reply was made to PR lib/57573; it has been noted by GNATS.
>>
>> From: Taylor R Campbell <riastradh@NetBSD.org>
>> To: kevans@FreeBSD.org
>> Cc: gnats-bugs@NetBSD.org
>> Subject: Re: lib/57573: Overflow possibilities in vis(3)
>> Date: Sat, 12 Aug 2023 15:01:36 +0000
>>
>> This is a multi-part message in MIME format.
>> --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
>> Turns out there's another buffer overrun lurking, when you do
>> strnvis(dst, 0, "", ...) -- it shouldn't write to dst[0], but it does.
>> I added an atf test for that, a branch to prevent it, and an assertion
>> to catch it.
>
> Ahh, whoops, we didn't look quite that far up- our immediate concern was
> a consumer that wasn't providing a buffer quite large enough (missing
> space for a NUL byte); I convinced them to fix the size and switch to
> strnvis(), but then realized that wouldn't quite be sufficient for the
> scenario they were hitting in the first place where we were apparently
> needing to encode every single byte in the buffer (thus, writing into a
> redzone with the NUL terminator). I'm quite glad you did a more careful
> analysis here.
>
>> I adapted your patch for the other overruns with some minor changes:
>
> Thanks!
>
>> 1. `char mbbuf[MB_LEN_MAX]' instead of `char mbbuf[MB_CUR_MAX]', since
>> MB_CUR_MAX is not a compile-time constant.
>> This avoids what is formally a variable-length array, which gets in
>> the way of stack protectors, &c. It may not pose a practical
>> problem for stack overflow, because MB_CUR_MAX <= MB_LEN_MAX, but
>> it's easier to keep measures that detect real problems elsewhere
>> happy this way.
>
> Ahh, good idea- I had contemplated making it MB_LEN_MAX in case someone
> does something squirrely and swaps out the process locale in the middle
> of a strvis(3), but hadn't yet convinced myself that that wasn't
> problematic in many other ways yet.
>
>> 2. Changed mbslength from ssize_t to size_t, since it can't go
>> negative, and added some assertions to support this.
>> This obviates the need to worry about what happens when mblength >
>> SIZE_MAX -- maybe impossible but I don't need to think about
>> proving that now!
>> 3. Added another bounds check upstream of the callocs, in case
>> 16*mbslength + 1 would overflow.
>> 4. Eliminated a duplicate call wcslen(start).
>> 5. Kept the #ifdef VIS_NOLOCALE in t_vis.c updated. Not sure why this
>> is here, but easier to just keep it updated than to investigate.
>> Attached is the patch series I committed to NetBSD for convenience if
>> you want to take a look. Tried to avoid unnecessary style differences
>> from yours to make merging easier if you want.
>
> Perfect, thanks! I'll get this merged downstream as soon as I can.
>
>> (I feel like this code shouldn't need to allocate multiple temporary
>> copies of everything -- should be able to work incrementally in a
>> stream from src to dst with only a constant-size intermediate buffer
>> like mbbuf, but I'm not feeling inclined to rewrite this now, so. >
>
> I agree with that sentiment... istrsenvisx in general kind of scares me
> a bit still. :-)
>
One tiny nit: SIZE_MAX is formally defined in <stdint.h>, so I think
there's some header pollution afoot in NetBSD; I've added the include to
vis.c downstream.
Thanks,
Kyle Evans
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src/lib/libc/gen
Date: Sun, 13 Aug 2023 15:19:13 +0000
Module Name: src
Committed By: riastradh
Date: Sun Aug 13 15:19:13 UTC 2023
Modified Files:
src/lib/libc/gen: vis.c
Log Message:
vis(3): Sort includes. No functional change intended.
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.83 -r1.84 src/lib/libc/gen/vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: src/lib/libc/gen
Date: Sun, 13 Aug 2023 15:19:24 +0000
Module Name: src
Committed By: riastradh
Date: Sun Aug 13 15:19:24 UTC 2023
Modified Files:
src/lib/libc/gen: vis.c
Log Message:
vis(3): Need <stdint.h> for SIZE_MAX, per C standard.
From Kyle Evans <kevans@FreeBSD.org>.
Followup to PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.84 -r1.85 src/lib/libc/gen/vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Thu, 07 Dec 2023 15:53:49 +0000
State-Changed-Why:
pullup-10 #485 https://releng.netbsd.org/cgi-bin/req-10.cgi?show=485
pullup-9 #1774 https://releng.netbsd.org/cgi-bin/req-9.cgi?show=1774
pullup-8 #1923 https://releng.netbsd.org/cgi-bin/req-8.cgi?show=1923
From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: [netbsd-10] src
Date: Sat, 9 Dec 2023 13:03:34 +0000
Module Name: src
Committed By: martin
Date: Sat Dec 9 13:03:34 UTC 2023
Modified Files:
src/lib/libc/gen [netbsd-10]: vis.c
src/tests/lib/libc/gen [netbsd-10]: t_vis.c
Log Message:
Pull up following revision(s) (requested by riastradh in ticket #485):
lib/libc/gen/vis.c: revision 1.76-1.86
tests/lib/libc/gen/t_vis.c: revision 1.10-1.14
vis(3): Avoid nonportable MIN in portable code.
vis(3) tests: Add xfail test for encoding overflow.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
PR lib/57573
vis(3) tests: Expand tests and diagnostic outputs on failure.
PR lib/57573
vis(3) tests: Test another overflow edge case.
Related to PR lib/57573.
vis(3): Make maxolen unsigned size_t, not ssize_t.
It is initialized once either to *dlen, which is unsigned size_t, or
to wcslen(start) * MB_MAX_LEN + 1, and wcslen returns unsigned size_t
too. So there appears to have never been any reason for this to be
signed.
Part of PR lib/57573.
vis(3): Make mbslength unsigned.
Sprinkle assertions and comments justifying the proposition that it
would never go negative if signed.
Obviates need to worry about mblength > SSIZE_MAX.
Prompted by PR lib/57573.
vis(3): Avoid arithmetic overflow before calloc(3).
Prompted by PR lib/57573.
vis(3): Call wcslen(start) only once.
It had better not change between these two times!
Prompted by PR lib/57573.
vis(3): Avoid potential arithmetic overflow in maxolen.
Can't easily prove that this overflow is impossible, so let's add a
check.
Prompted by PR lib/57573.
vis(3): Fix main part of PR lib/57573.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
vis(3): Fix one more buffer overrun in an edge case.
PR lib/57573
vis(3): Sort includes. No functional change intended.
Prompted by PR lib/57573.
vis(3): Need <stdint.h> for SIZE_MAX, per C standard.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
Followup to PR lib/57573.
vis(3): Per KNF, sys/param.h comes before sys/types.h.
Which is nice because that's also lexicographic.
To generate a diff of this commit:
cvs rdiff -u -r1.75 -r1.75.2.1 src/lib/libc/gen/vis.c
cvs rdiff -u -r1.9 -r1.9.24.1 src/tests/lib/libc/gen/t_vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: [netbsd-9] src
Date: Sat, 9 Dec 2023 13:09:04 +0000
Module Name: src
Committed By: martin
Date: Sat Dec 9 13:09:04 UTC 2023
Modified Files:
src/lib/libc/gen [netbsd-9]: vis.c
src/tests/lib/libc/gen [netbsd-9]: t_vis.c
Log Message:
Pull up following revision(s) (requested by riastradh in ticket #1774):
lib/libc/gen/vis.c: revision 1.75-1.86
tests/lib/libc/gen/t_vis.c: revision 1.10-1.14
PR 56260: fix out-of-bounds stack read.
vis(3): Avoid nonportable MIN in portable code.
vis(3) tests: Add xfail test for encoding overflow.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
PR lib/57573
vis(3) tests: Expand tests and diagnostic outputs on failure.
PR lib/57573
vis(3) tests: Test another overflow edge case.
Related to PR lib/57573.
vis(3): Make maxolen unsigned size_t, not ssize_t.
It is initialized once either to *dlen, which is unsigned size_t, or
to wcslen(start) * MB_MAX_LEN + 1, and wcslen returns unsigned size_t
too. So there appears to have never been any reason for this to be
signed.
Part of PR lib/57573.
vis(3): Make mbslength unsigned.
Sprinkle assertions and comments justifying the proposition that it
would never go negative if signed.
Obviates need to worry about mblength > SSIZE_MAX.
Prompted by PR lib/57573.
vis(3): Avoid arithmetic overflow before calloc(3).
Prompted by PR lib/57573.
vis(3): Call wcslen(start) only once.
It had better not change between these two times!
Prompted by PR lib/57573.
vis(3): Avoid potential arithmetic overflow in maxolen.
Can't easily prove that this overflow is impossible, so let's add a
check.
Prompted by PR lib/57573.
vis(3): Fix main part of PR lib/57573.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
vis(3): Fix one more buffer overrun in an edge case.
PR lib/57573
vis(3): Sort includes. No functional change intended.
Prompted by PR lib/57573.
vis(3): Need <stdint.h> for SIZE_MAX, per C standard.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
Followup to PR lib/57573.
vis(3): Per KNF, sys/param.h comes before sys/types.h.
Which is nice because that's also lexicographic.
To generate a diff of this commit:
cvs rdiff -u -r1.74 -r1.74.6.1 src/lib/libc/gen/vis.c
cvs rdiff -u -r1.9 -r1.9.16.1 src/tests/lib/libc/gen/t_vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/57573 CVS commit: [netbsd-8] src
Date: Sat, 9 Dec 2023 13:10:16 +0000
Module Name: src
Committed By: martin
Date: Sat Dec 9 13:10:16 UTC 2023
Modified Files:
src/lib/libc/gen [netbsd-8]: vis.c
src/tests/lib/libc/gen [netbsd-8]: t_vis.c
Log Message:
Pull up following revision(s) (requested by riastradh in ticket #1923):
lib/libc/gen/vis.c: revision 1.75-1.86
tests/lib/libc/gen/t_vis.c: revision 1.10-1.14
PR 56260: fix out-of-bounds stack read.
vis(3): Avoid nonportable MIN in portable code.
vis(3) tests: Add xfail test for encoding overflow.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
PR lib/57573
vis(3) tests: Expand tests and diagnostic outputs on failure.
PR lib/57573
vis(3) tests: Test another overflow edge case.
Related to PR lib/57573.
vis(3): Make maxolen unsigned size_t, not ssize_t.
It is initialized once either to *dlen, which is unsigned size_t, or
to wcslen(start) * MB_MAX_LEN + 1, and wcslen returns unsigned size_t
too. So there appears to have never been any reason for this to be
signed.
Part of PR lib/57573.
vis(3): Make mbslength unsigned.
Sprinkle assertions and comments justifying the proposition that it
would never go negative if signed.
Obviates need to worry about mblength > SSIZE_MAX.
Prompted by PR lib/57573.
vis(3): Avoid arithmetic overflow before calloc(3).
Prompted by PR lib/57573.
vis(3): Call wcslen(start) only once.
It had better not change between these two times!
Prompted by PR lib/57573.
vis(3): Avoid potential arithmetic overflow in maxolen.
Can't easily prove that this overflow is impossible, so let's add a
check.
Prompted by PR lib/57573.
vis(3): Fix main part of PR lib/57573.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
vis(3): Fix one more buffer overrun in an edge case.
PR lib/57573
vis(3): Sort includes. No functional change intended.
Prompted by PR lib/57573.
vis(3): Need <stdint.h> for SIZE_MAX, per C standard.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
Followup to PR lib/57573.
vis(3): Per KNF, sys/param.h comes before sys/types.h.
Which is nice because that's also lexicographic.
To generate a diff of this commit:
cvs rdiff -u -r1.73.4.1 -r1.73.4.2 src/lib/libc/gen/vis.c
cvs rdiff -u -r1.9 -r1.9.6.1 src/tests/lib/libc/gen/t_vis.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: pending-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sat, 09 Dec 2023 14:16:39 +0000
State-Changed-Why:
fixed and pulled up to 10, 9, 8
(with caveat something else is broken in 8 but probably not this)
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2023
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home